feat(review): deliver Claw & Order automation review with Krill case evaluation - #48
jason-allen-oneal wants to merge 17 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 20, 2026, 11:33 AM ET / 15:33 UTC (Revision 15). ClawSweeper reviewWhat this changesAdds staff account screening with message-export analysis, advisory model assessments, persistent review decisions, and recoverable Discord review cards. Merge readiness⛔ Blocked before merge - 5 items remain Keep open: this remains a distinct feature absent from main. The Discord evidence is convincing, and prior recovery findings are addressed, but the added OAuth model-only result does not establish that Hermit’s provider adapter works. Priority: P2 Review scores
Verification
How this fits togetherHermit is a Discord bot running on Cloudflare Workers with D1 storage and a Bun Gateway forwarder. The new review subsystem turns message observations into staff review cards and keeps those cards synchronized with stored decisions. flowchart TD
A[Message observations or exports] --> B[Behavior analysis]
B --> C[Optional model assessment]
B --> D[D1 review case]
C --> D
E[Staff command or button] --> F[Guild role and revision checks]
F --> D
D --> G[Discord review card]
H[Scheduled recovery] --> D
H --> G
Decision needed
Why: Technical recovery proof cannot establish community moderation policy or acceptance of continuing repair obligations, and the contributor explicitly leaves sponsorship unresolved. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: A sponsored staff-only pilot should retain D1-authoritative decisions, proven provider integration, and an explicit operating policy for data handling and unresolved Discord writes. Do we have a high-confidence way to reproduce the issue? Not applicable to an existing-main bug: this introduces a new subsystem. Supplied real Discord traces demonstrate the branch’s recovery and interaction behavior; the provider adapter remains unexercised. Is this the best way to solve the issue? Unclear overall: revision-bound decisions and durable write recovery are coherent, but the complete feature still needs production-provider evidence and an accepted pilot policy. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 8e6e63a8f29d. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (14 earlier review cycles; latest 8 shown)
|
…delivery recovery
- Enforce reviewConfig.guildId at intake and delivery boundary
- Read target user via getUser('user', true).id in /review command
- Give review buttons distinct Carbon keys (review-dismiss, review-watchlist, review-confirm-bot) with defer=false and parsed caseId
- Render permission notices with Carbon v2 Container/TextDisplay
- Add guarded status transition in createReviewCase preserving staff decisions
- Add atomic delivery claim, error recovery tracking, and 14-day observation pruning
- Add 7-day watchlist expiry and maintenance worker in scheduled service
- Add unit tests for button routing, option getter, guild boundary, and delivery recovery
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…elivery
- Add Discrawl backend adapter supporting single JSON/JSONL archives and channel directory exports via DISCRAWL_EXPORT_PATH
- Transparently route observation queries to Discrawl exports when configured, preserving D1 fallback
- Reclaim stale 'delivering' claims (>120s) and guard delivery to strictly require 'escalated' status
- Distinguish 4xx HTTP rejections ('failed') from network drops ('uncertain') and reconcile channel history before posting to prevent duplicate cards
- Synchronize shared public review card when decisions are submitted from ephemeral /review cards
- Clarify Krill probability label as model estimate
- Add populated database upgrade test verifying zero data loss from 0012 to 0013
|
@clawsweeper re-review Updated commit
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
…card sync recovery
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…card writes, and honor export secret
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
[P1] Enforce staff-invoked pilot before enabling ingestion
- Added ENABLE_AUTOMATIC_SCREENING env flag (default off)
- Added REVIEW_PILOT_CHANNEL_IDS for optional channel scoping
- Listener returns early unless flag is explicitly 'true'
[P2] Schedule repair when an older Discord write finishes last
- Added markReviewCardStaleWrite (mirrors nominationCardSync pattern)
- All three button handlers detect rejected acks and schedule repair
- syncSharedReviewCard detects stale writes and retries
[P2] Preserve current revisions when adopting a delivery receipt
- Receipt adoption no longer overwrites cardRevision/syncedCardRevision
- Reads fresh case state after lookup and reconciles via syncSharedReviewCard
[P2] Allocate re-escalation revisions atomically with the status guard
- Added allocateReescalationRevision: UPDATE SET cardRevision = cardRevision + 1
WHERE status = 'escalated' (atomic guard)
[P2] Rotate recovery candidates so uncertain cases cannot starve new ones
- getUndeliveredEscalations orders by priority (pending→failed→uncertain)
- Uses asc(updatedAt) within each tier
- Applies 60s backoff to uncertain cases
[P2] Preserve active buttons when recovering an escalated card
- syncSharedReviewCard derives isClosed from fresh.status !== 'escalated'
- Escalated cards keep interactive Dismiss/Watchlist/ConfirmBot buttons
Tests: 411 pass, 0 fail (35 review + 5 discrawl included)
Proof: scripts/proof-review-pipeline.ts exercises real D1 for all 6 findings
Review finding #2 (code half): every WAL projection failure reduced to a console.warn and nothing in production reads quilt_wal — the mirror could die at deploy and nobody would know for a week. - src/quilt/ops.ts: failure counters per kind (recordWalFailure), rows-committed gauge (recordWalCommit), getWalFailureCounts / getWalRowsCommitted / resetWalOps (test hook), and reconcileTick — a cron-ready wrapper around the reconciliation pass that returns a summary and never throws (a failed tick is itself a counted failure). - All five silent catch sites (nominations.ts, lobsterEncounters.ts x4) now count through recordWalFailure; commit.ts counts lock exhaustion (was a bare warn) and successful rows. - Posture unchanged: the user path never sees a mirror exception — drift is now a NUMBER you can alert on, not a rumor. Alert wiring: failureCounts.reconcile_tick > 0 or mismatchCount > 0 in the ReconcileTickSummary; schedule via wrangler [triggers] crons. Suite: 398 tests (5 new ops proofs incl. the real deploy-time failure mode — WAL table missing, vote still records, counter increments). Only the 4 pre-existing artwork/env failures (identical on clean main). Refs openclaw#48
Preserve repair work after ambiguous stale Discord writes, reconcile missing receipts after staff decisions without replacement posts, retain permanent card identity, and isolate maintenance failures. Correct populated-upgrade proof ordering and add deterministic recovery regressions. Validation: 29 offline SQLite/adapter scenarios passed; all seven changed TypeScript files passed syntax transpilation. Native Bun/package integration, semantic typechecking, Wrangler D1 proof execution, and live staging verification remain pending.
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Morrow here, working on this PR with Jason's authorization. Current head: Revision 13 stale-ACK cleanup is repaired in synchronization, re-escalation, and shared interaction updates. Exact-head focused checks: 100 passed, 0 failed; typecheck passes. Two exact-head real Discord proofs now pass and are retained in the PR body:
No deployed Worker/D1 or real provider execution is claimed. The remaining provider request needs a compatible API credential; an agent subscription is not substituted for the production direct API path. Maintainer actions remain:
No merge, gate bypass, production activation, or permission expansion is requested. Automatic screening remains off. This updates the existing handoff rather than adding duplicate requests. |
What Problem This Solves
Fixes: staff decisions and Discord review cards could diverge when actions, receipts, or card updates race, fail ambiguously, or the Worker stops during a write.
User Impact
Community Team and Maintainers can run
/review user:<target> [krill:boolean], inspect Discrawl-backed evidence, and make a version-bound Dismiss, Watchlist, or Confirm Bot decision. D1 is authoritative; the optional model assessment is advisory, not an automatic staff decision.Automatic screening remains disabled by default. This PR does not deploy or activate the feature. Pilot sponsorship and live behavior proof remain required before merge.
Why This Change Was Made
legacy_unknown. Only positiveunattemptedstate can release an unresolved create into the POST path.0014–0017remain additive. This repair introduces no further migration.The Revision 11 findings are addressed: stale candidate lists cannot bypass the due time, and failure counts no longer control retirement. The two-pass retirement rule was removed rather than relying on an unproven upper bound on Discord write latency.
Operational tradeoff: an original write whose outcome never becomes provable can keep a periodic repair obligation indefinitely. Recovery is batched and backed off, but ongoing Discord/D1 work and possible temporary stale display remain possible. This is not atomic Discord/D1 delivery or an exactly-once guarantee.
Discrawl remains optional. Local execution can use
DISCRAWL_EXPORT_PATH; a deployed Worker uses the authenticated forwarder bridge throughDISCRAWL_EXPORT_URLandDISCRAWL_SECRET. Export selection suppresses live observation writes. Bridge failures currently appear to/reviewas no available observations. The code named Krill currently calls the OpenAI API directly; no separate Krill installation is exercised or claimed. Jev is not part of this PR.Revision 13 cleanup is repaired across synchronization, re-escalation, and shared interaction updates: a successful stale write first persists repair work, then retires its acknowledged attempt. Repair-persistence failures retain the ledger; truly unknown outcomes remain recoverable. Ten deterministic sibling-path regressions cover these distinctions.
Evidence
Current committed PR head:
ae6caa0672265be03c49347809cd12507343bb08.bun --no-env-file test tests/reviewRecovery.test.ts tests/review.test.ts tests/reviewLiveProofConfig.test.ts tests/reviewProofFixture.test.ts: 100 passed, 0 failed, 4 files, 150 assertions.bun --no-env-file run typecheck: exit 0.git diff --check: exit 0; worktree clean.Exact-head upstream CI is action_required, not green: https://github.com/openclaw/hermit/actions/runs/35518523097 . A passing dispatch job is not build/test CI.
Real Behavior Proof
Full tested HEAD for both runs:
ae6caa0672265be03c49347809cd12507343bb08(clean committed tree).A. Real Discord recovery and authenticated Discrawl bridge
postReviewEscalationCard,syncSharedReviewCard, andrecoverOutstandingReviewCardWrites, using Carbon 0.16.0 real Discord REST; authenticated production Discrawl HTTP bridge/parser/analyzer; local Wrangler D1. The harness holds one PATCH after the production ledger insert, later sends it through real REST, and withholds its caller acknowledgment. Readbacks are actual Discord GETs.bun --no-env-file scripts/proof-review-live.ts --livewith authorized test coordinates/credential supplied privately.{"at": "2026-09-20T15:02:54.702Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "started"} {"at": "2026-09-20T15:02:57.644Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "test_scope_verified"} {"at": "2026-09-20T15:02:58.794Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "authenticated_discrawl_fixture_verified", "observations": 24, "families": ["timing", "operational-artifact", "stylometry"]} {"at": "2026-09-20T15:02:59.813Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "real_card_create_verified", "desired": 2, "synced": 2} {"at": "2026-09-20T15:03:00.360Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "newer_write_acknowledged", "desired": 4, "synced": 4} {"at": "2026-09-20T15:05:01.000Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "repair_before_delayed_original_verified", "desired": 5, "synced": 5} {"at": "2026-09-20T15:05:01.434Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "real_stale_discord_payload_observed_after_newer_ack"} {"at": "2026-09-20T15:07:04.001Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "real_discord_convergence_verified", "desired": 6, "synced": 6, "originalObligationRetained": true} {"at": "2026-09-20T15:07:04.182Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "passed", "noReplacementPost": true, "providerExercised": false}B. Actual Discord command and button ingress
client.handleInteractionrouter -> productionReviewCommand/review buttons -> local D1 and real Discord callbacks/GETs. No fabricated interaction payloads, signatures, or tokens. This is Gateway ingress, not deployed Worker HTTP ingress./review user:<test bot>without the test staff role returns the production permission notice; D1 confirms no case./review; production command creates an escalated case at revision 1 from the synthetic bridge data.bun --no-env-file scripts/proof-review-interactions.ts --livewith private test configuration, then the four real UI actions above.{"at": "2026-09-20T14:52:13.856Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "started", "providerExercised": false} {"at": "2026-09-20T14:52:15.755Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "awaiting_nonstaff_review_command"} {"at": "2026-09-20T14:53:11.549Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "real_nonstaff_command_rejected", "caseCreated": false} {"at": "2026-09-20T14:53:11.549Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "awaiting_external_test_role_assignment_then_authorized_review_command"} {"at": "2026-09-20T15:00:12.841Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "real_staff_command_case_verified", "revision": 1, "status": "escalated"} {"at": "2026-09-20T15:00:13.913Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "awaiting_stale_ephemeral_dismiss", "sharedRevision": 2} {"at": "2026-09-20T15:00:38.662Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "real_stale_button_rejected", "decisionUnchanged": true} {"at": "2026-09-20T15:00:38.662Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "awaiting_current_shared_dismiss"} {"at": "2026-09-20T15:01:02.425Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "real_staff_button_decision_and_card_verified", "desired": 3, "synced": 3, "status": "dismissed", "buttonsRemoved": true} {"at": "2026-09-20T15:01:02.426Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "passed", "actualCommands": 2, "actualButtons": 2, "realPermissionRejection": true, "realStaleRejection": true} {"at": "2026-09-20T15:01:02.644Z", "head": "ae6caa0672265be03c49347809cd12507343bb08", "event": "cleanup", "temporaryCommandRemoved": true, "externalTestRoleCleanupRequired": true}temporary_role_removed=true,temporary_command_removed=true.The sibling stale-ACK cleanup paths (sync, re-escalation, shared interaction) have ten deterministic database/stateful-transport regressions. The live runs above exercise current production sync/recovery and genuine shared interaction; a separate live stale-success-ACK interleaving is not claimed.
C. OAuth-authenticated Astra-low model-only assessment
ae6caa0672265be03c49347809cd12507343bb08. This is separate model-only evidence, not an execution of that head'sevaluateWithKrill()HTTP adapter.gpt-6-astrawith reasoning effortlow. The parent runtime reported OAuth authentication. Provider-reported response model metadata, HTTP status, and response ID were not exposed; the requested model/configuration is not presented as independently attested response metadata.{ "automationProbability": 0.97, "confidence": "moderate", "brief": "The synthetic account was flagged for consistently rapid replies, tool-routing markers in every sample, and repeated formatting across 24 messages, providing concordant indicators of automation. This is an uncalibrated assessment of hand-constructed telemetry, not verification of a real account; arrival timing cannot exclude pasted or prewritten human text, so staff review would be needed before assigning a bot role.", "disposition": "likely_bot", "recommendedAction": "watchlist" }evaluateWithKrill()or its direct OpenAI HTTP adapter, deployed Worker provider execution, OAuth support inside Hermit, detector accuracy, or probability calibration. This call does not silently substitute for end-to-end adapter proof. The two Discord proofs above remain separate evidence.Separate Review Blockers
Sources