feat(review): deliver Claw & Order automation review with Krill case evaluation - #48
Open
jason-allen-oneal wants to merge 7 commits into
Open
jason-allen-oneal wants to merge 7 commits into
jason-allen-oneal wants to merge 7 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. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 16, 2026, 11:49 PM ET / September 17, 2026, 03:49 UTC (Revision 6). ClawSweeper reviewWhat this changesAdds staff-invoked Discord account screening, optional model assessments, persistent review cases, an authenticated message-export bridge, and recoverable review cards. Merge readiness⛔ Blocked before merge - 9 items remain This remains a distinct contribution absent from main. Five prior concerns are addressed, but interrupted delivery and ambiguous stale writes still escape recovery, and real Discord proof remains incomplete. Priority: P2 Review scores
Verification
How this fits togetherHermit runs Discord commands in a Cloudflare Worker, with a Bun forwarder supplying gateway events and optional message exports. The new review pipeline turns message telemetry into staff assessments and stores decisions in D1 before updating Discord cards. flowchart TD
A[Message exports or enabled intake] --> B[Behavioral analysis]
C[Staff review command] --> B
B --> D[Optional model assessment]
B --> E[D1 review cases]
D --> E
E --> F[Discord review cards]
F --> G[Authorized staff decisions]
G --> E
E --> H[Scheduled delivery recovery]
H --> F
Decision needed
Why: The contributor adopted the suggested pilot, but the available discussion contains no maintainer approval of classification policy, data processing, or rollout scope. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Deliver a maintainer-approved staff pilot with durable receipt reconciliation independent of case disposition, convergent card synchronization, and verified fresh and populated D1 operation. Do we have a high-confidence way to reproduce the issue? Yes for the patch defects: source establishes the failing interruption schedules and the incorrect upgrade-proof ordering. No runtime reproduction was executed during this read-only review. Is this the best way to solve the issue? Partly: a default-off staff pilot fits the stated scope, but the recovery design must preserve work across ambiguous responses and terminal decisions; the existing nomination synchronization offers a useful failure-handling precedent. Full review comments:
Overall correctness: patch is incorrect 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 (5 earlier review cycles)
|
…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
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
removed
the
merge-risk: 🚨 security-boundary
…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
Author
|
@clawsweeper re-review Updated commit
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
…card sync recovery
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
…card writes, and honor export secret
Author
|
@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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
High-volume public community channels face automated bot and AI agent activity that evades simple filters, live message observation writes overload transactional databases (Cloudflare D1), and unhardened escalation delivery risks duplicate cards, foreign bot receipt spoofing, or stale staff actions during worker interruptions.
User Impact
User impact: Community Team and Maintainers can inspect suspicious accounts via
/review user:<target> [krill:boolean]with model estimates from Krill (gpt-6-astra low-thinking), while high-confidence agent activity automatically escalates review cards to#ct-generalwith actionable Carbon buttons (Dismiss, Watchlist 7d, Confirm Bot).Operators can configure
DISCRAWL_EXPORT_PATHon the forwarder host orDISCRAWL_EXPORT_URLon the Worker to serve message observation features and timing telemetry directly from Discrawl exports, eliminating live D1 observation writes. Shared review cards in#ct-generalautomatically synchronize when staff take action from ephemeral cards, and delivery recovery guarantees exact-bot receipt verification without duplicate sends or lost escalations.Why This Change Was Made
src/services/discrawl.ts,forwarder/src/discrawlServer.ts,forwarder/src/index.ts):authorIdusing file modification times (mtime).startDiscrawlServer) exposing/api/discrawl/observationsand/api/discrawl/countwith Bearer token authentication (DISCRAWL_SECRET || DEPLOY_SECRET). The Cloudflare Worker queries this bridge viaDISCRAWL_EXPORT_URL, with filesystem fallback for Node/Bun local runtimes.reviewIngestMessageCreate.ts) andrecordObservation(src/data/review.ts) skip live D1 writes entirely when export mode is enabled.src/services/reviewNotifier.ts,src/data/review.ts):findExistingReviewCardrequiresmessage.author?.id === botId && message.author?.bot === trueusingprocess.env.DISCORD_CLIENT_IDAND requires case-specific markercaseId=${caseId}, rejecting foreign bot messages in#ct-general.claimReviewCaseDeliveryrecordsprevious_delivery_statusatomically in D1. Empty or inconclusive history lookups preserveuncertainand suppress duplicate sends. Stale claims (>120s) are treated conservatively as uncertain.review-escalate:${caseId}:${cardRevision}withenforce_nonce: trueon Discord POST to prevent gateway double-delivery.src/data/review.ts,src/components/reviewButtons.ts,src/services/reviewNotifier.ts):recordReviewCaseDecisionpersists the staff decision and incrementscardRevisionatomically in D1 before any Discord I/O occurs, guaranteeingcardRevision > syncedCardRevisionexists durably in D1 to allow maintenance recovery to discover and heal dropped interactions.markReviewCardSynced(caseId, revision)enforces optimistic concurrency withWHERE case_id = ? AND card_revision = ?(identical to Hermit's nomination sync pattern insrc/data/nominations.ts), rejecting stale writes from concurrent older actions.src/commands/review.ts):openor expired watchlist cases toescalatedwhen manual review reachesreview-recommended, settingdeliveryStatus: "pending"for recovery while preserving terminal staff decisions (dismissed,confirmed_bot).src/review/krillEvaluator.ts,src/review/analyzer.ts):interaction.options.getUser("user", true).idfor type-safe User resolution.review-dismiss:caseId=${caseId},review-watchlist:caseId=${caseId},review-confirm-bot:caseId=${caseId}) withdefer = falsefor immediate Carbon v2 updates.Decision Resolution
/reviewcommand invoked by Community Team / Maintainers on suspicious accounts, with observations sourced from Discrawl exports or agreed test channels). Full automated guild-wide background escalation will be evaluated following operational experience and false-positive baseline verification.Evidence
DISCRAWL_EXPORT_PATH/DISCRAWL_EXPORT_URL) serves observations with zero live D1 writesstartDiscrawlServer) exposes authenticated endpoints consumed viaDISCRAWL_EXPORT_URLDEPLOY_SECRETon startupDISCORD_CLIENT_ID,bot === true, andcaseId=${caseId}; foreign bot receipts rejecteduncertain/ staledeliveringtodeliveringprevious_delivery_statusdurably preserved across atomic claims; stale claims (>120s) treated conservatively as uncertainuncertainwithout sending duplicate cardsupdatedAtcaseIdandcardRevisionmarkReviewCardSyncedusesWHERE case_id = ? AND card_revision = ?to reject stale writesreviewMessageIdwith fresh monotonic revision to reopen active review buttonsreview-recommendedescalatedand schedules deliverycard_revision,synced_card_revision) repaired by periodic maintenancebun test): Passed all 405 tests across 42 files (185,668 assertions) with 0 failures.tests/discrawl.test.ts): 5 tests passing:getRecentUserObservationsintegration withDISCRAWL_EXPORT_PATH.startDiscrawlServerwithDISCRAWL_EXPORT_URLand Bearer token auth.DISCRAWL_SECRET || DEPLOY_SECRETand verifies unauthorized calls return 401.tests/review.test.ts): 29 tests passing:defer = falseacross all review buttons.caseIdmarker; strictly rejects foreign bot messages.recoverSharedCardSync.card_revision,synced_card_revision,previous_delivery_status), and verifies zero data loss.tests/reviewBenchmark.test.ts): 2 tests passing:1531171766179856496): 100/100 High concordance across 3 families ->review-recommended.958510681928400920): Low concordance -> gated atno-strong-indicators.bun run typecheck,bun run --cwd forwarder typecheck, andbun run deploy:dry-runpassed with 0 errors.Review Disposition (Revision 4 Findings)
All 5 actionable findings from ClawSweeper's Revision 4 review are addressed and verified:
Preserve uncertain delivery state across the atomic claim (
src/services/reviewNotifier.ts:120-123,src/data/review.ts:199):claimReviewCaseDeliverynow writesprevious_delivery_status: reviewCases.deliveryStatusanddeliveryStatus: "delivering"atomically.postReviewEscalationCardevaluateswasUncertain = claimedCase.previousDeliveryStatus === "uncertain" || claimedCase.previousDeliveryStatus === "delivering". If history reconciliation is inconclusive or empty,uncertainis preserved in D1 and duplicate sends are suppressed.updatedAtin nonce with deterministicreview-escalate:${caseId}:${cardRevision}.preserves uncertain delivery state across real atomic claim and prevents duplicate send on empty history.Persist synchronization work with the staff decision (
src/components/reviewButtons.ts:145-152,src/data/review.ts):recordReviewCaseDecisionatomically updates the staff decision (status,expiresAt,decidedById,decisionReason) AND incrementscardRevision: sql${reviewCases.cardRevision} + 1`` before any Discord I/O.cardRevision > syncedCardRevisionis already persisted in D1, enabling periodic maintenance (recoverSharedCardSync) to discover and heal the card.Reject stale card writes using monotonic persisted revisions (
src/services/reviewNotifier.ts:236-239,src/data/review.ts):markReviewCardSynced(caseId, revision)which performsWHERE case_id = ? AND card_revision = ?. If a newer concurrent decision has bumpedcardRevision, the update returnsnull, preventing stale writes from overwritingsyncedCardRevision.rejects stale card writes using monotonic persisted revisions.Honor the configured export secret in forwarder startup (
forwarder/src/index.ts:54-59):forwarder/src/index.tsextractsDISCRAWL_SECRETfromBun.envwith fallback toDEPLOY_SECRET.startDiscrawlServerreceivessecret: DISCRAWL_SECRET || DEPLOY_SECRET, eliminating 401s whenDISCRAWL_SECRETis set.authenticates discrawl server requests and rejects unauthorized callsintests/discrawl.test.ts.Promote existing open cases when manual review recommends escalation (
src/commands/review.ts:145-149):ReviewCommandpromotesopenor expired watchlist cases toescalatedwhenreport.priority === "review-recommended", resettingdeliveryStatusto"pending"for delivery while preserving terminal staff decisions (dismissed,confirmed_bot).promotes existing open cases when manual review reaches review-recommended.