Skip to content

1133.md

Latest commit

 

History

History
308 lines (242 loc) · 16.9 KB

1133.md

File metadata and controls

308 lines (242 loc) · 16.9 KB
reconciled_at 2026-05-09 12:46:54 UTC
current_state closed
review_comment_synced_at 2026-05-08 15:51:19 UTC
number 1133
repository openclaw/clawhub
type pull_request
title feat: add liftModerationHold admin mutation for false-positive recovery
url openclaw/clawhub#1133
state_at_review open
item_created_at 2026-03-22 04:52:18 UTC
item_updated_at 2026-05-07 15:41:36 UTC
author Justincredible-tech
author_association NONE
labels
reviewed_at 2026-05-08 15:48:41 UTC
main_sha 38c21345906ab1f107a91b33bb86b63667d96643
pull_head_sha c895190d2d04134b07a96d7a8b5e6cf97b72a25a
latest_release v0.1.0
latest_release_sha 8216c73c9bd4e7befe12081e3e38a8333fbce46b
fixed_release unknown
fixed_sha unknown
fixed_at unknown
fixed_pr_url unknown
fixed_pr_number unknown
fixed_pr_title unknown
fixed_pr_merged_at unknown
fixed_pr_sha unknown
fixed_pr_confidence unknown
fixed_pr_source unknown
review_policy 9f214239696c0cc2
review_model gpt-5.5
review_reasoning_effort high
review_sandbox danger-full-access
review_service_tier default
review_prompt_chars 131417
review_static_prompt_chars 33412
review_context_chars 96719
review_schema_chars 14081
review_additional_prompt_chars 0
review_context_elapsed_ms 6343
review_codex_elapsed_ms 253657
review_mode propose
review_status complete
local_checkout_access verified
item_snapshot_hash 34bd2d64ebbfbfd977fe27cadf2214c7a826725af474bce077a2d7e692c7d721
close_comment_sha256 none
review_comment_sha256 951a1a1cbcd268e04cb37bdb7a38180224481db2bba048b89b24caaecb06dd4b
review_comment_id 4332438654
review_comment_url openclaw/clawhub#1133 (comment)
decision keep_open
close_reason none
confidence high
action_taken kept_open
work_candidate manual_review
work_confidence high
work_priority high
work_status manual_review
work_reason_sha256 a96b0113ba6ed107ccd0b261f1476c6039549d294bc843c12f8854df16fe1f9b
work_prompt_sha256 none
work_cluster_refs
work_validation
bun run format:check
bun run lint
bun run test -- convex/users.test.ts
bun run test -- convex/skills.pendingScanQueue.test.ts
bun run test -- convex/skills.rateLimit.test.ts
Contributor-provided redacted real behavior proof of a real moderation-hold lift
work_likely_files
convex/users.ts
convex/skills.ts
convex/users.test.ts
convex/skills.pendingScanQueue.test.ts
convex/skills.rateLimit.test.ts
docs/security.md
specs/security-moderation.md
item_category feature
reproduction_status source_reproducible
reproduction_confidence high
requires_new_feature true
requires_new_config_option false
requires_product_decision true
real_behavior_proof_status missing
real_behavior_proof_evidence_kind none
real_behavior_proof_needs_contributor_action true

#1133: feat: add liftModerationHold admin mutation for false-positive recovery

Type: pull_request

URL: openclaw/clawhub#1133

Author: Justincredible-tech

Author association: NONE

Labels: none

Created at: Mar 22, 2026, 04:52 UTC

Updated at: May 7, 2026, 15:41 UTC

Reviewed against: 38c21345906a

Codex review: model gpt-5.5, reasoning high

Latest release at review time: v0.1.0 (8216c73c9bd4)

Fixed in: not determined

Decision

Keep open: kept open

Confidence: high

Action taken: kept_open

Summary

Keep open: current main still lacks the moderation-hold lift surface, and this PR remains an active security-sensitive implementation under maintainer reinvestigation with blocking restore-policy findings and missing real behavior proof.

What This Changes

The PR adds public and internal Convex admin mutations to clear user moderation holds, batch-restore eligible owned skills, audit the lift, add tests, and document the admin CLI flow.

Best Possible Solution

Land a maintainer-reviewed hold-lift implementation that clears user hold fields, restores only skills safely attributable to the lifted hold, preserves unrelated moderation states, keeps unresolved scans queued, records complete audit data, and documents the operator path.

Reproduction Assessment

Yes, for the PR review findings via source inspection: current main treats VT not_found as pending and the hold batch overwrites prior moderation reasons, while the PR diff restores from those states and records only first-batch counts. I did not run a live Convex reproduction because this was a read-only review.

Solution Assessment

No. The admin recovery capability is a reasonable boundary, but this PR needs safer restore eligibility, unresolved-scan handling, complete audit accounting, maintainer/security review, and real behavior proof before merge.

Review Findings

Overall correctness: patch is incorrect

Overall confidence: 0.88

Full review comments:

  • [P1] Treat not_found VT results as unresolved: convex/skills.ts:4147-4148
    • body: The restore path only keeps missing, pending, or loading VT results in the scan queue. Current moderation logic maps vtAnalysis.status === "not_found" to scanner.vt.pending, and the queue retries non-final VT statuses, so a lifted skill with not_found can become active/restored.moderation_lift and drop out of scanning. Include not_found or reuse the existing pending/final-status helper before making the skill public.
    • confidence: 0.92
  • [P1] Preserve pre-hold moderation locks: convex/skills.ts:4128-4129
    • body: This restore treats the post-hold user.moderation reason as proof that the skill is safe to reactivate. The hold batch rewrites every non-malicious owned skill to user.moderation, including rows already hidden for independent reasons such as quality.low, so lifting the hold can publish content quarantined before the user hold. Store the prior moderation state or skip rows that were already hidden for another reason.
    • confidence: 0.84
  • [P2] Report all restored skills in the lift audit: convex/users.ts:813
    • body: The caller records only the first batch's restoredCount; when scheduledSkills is true, continuation batches can restore additional skills without updating the user.moderation.lift metadata or returned count. Large false-positive recoveries therefore get an incomplete audit trail.
    • confidence: 0.86

Security Review

Status: needs_attention

Summary: The diff has no dependency, workflow, lockfile, or package-publishing changes, but the new admin moderation recovery path can expose unresolved or independently hidden skills and undercount recovery audit data.

Concerns:

  • [high] Unresolved VT results can be published: convex/skills.ts:4147
    • body: The PR treats VT not_found as safe to restore even though current moderation classifies that state as pending, allowing a skill to become active and leave the scan queue.
    • confidence: 0.92
  • [high] Independent moderation decisions can be cleared: convex/skills.ts:4128
    • body: Because the existing hold batch overwrites non-malicious hidden skills to user.moderation, the new restore path can reactivate rows that were hidden for manual, report, or quality reasons before the user hold.
    • confidence: 0.84
  • [medium] Recovery audits can undercount batch restores: convex/users.ts:813
    • body: Only the initial batch count is written to user.moderation.lift, so later continuation batches can omit restored skills from the security audit trail.
    • confidence: 0.86

Real Behavior Proof

Status: missing

Evidence kind: none

Needs contributor action: true

Summary: Needs real behavior proof before merge: the PR body only lists tests/lint, so the contributor should add redacted terminal output, logs, live output, screenshots, a recording, or a linked artifact showing a real moderation-hold lift after the fix.

Work Candidate

Candidate: manual_review

Confidence: high

Priority: high

Status: manual_review

Reason: Maintainer/security review is needed because this external PR changes moderation recovery semantics, lacks real behavior proof, and includes restore-policy blockers that automation should not settle for the contributor.

Cluster refs:

Likely files:

  • convex/users.ts
  • convex/skills.ts
  • convex/users.test.ts
  • convex/skills.pendingScanQueue.test.ts
  • convex/skills.rateLimit.test.ts
  • docs/security.md
  • specs/security-moderation.md

Validation:

  • bun run format:check
  • bun run lint
  • bun run test -- convex/users.test.ts
  • bun run test -- convex/skills.pendingScanQueue.test.ts
  • bun run test -- convex/skills.rateLimit.test.ts
  • Contributor-provided redacted real behavior proof of a real moderation-hold lift

Evidence

  • main_surface_missing: Current main has no liftModerationHold, restoreOwnedSkillsForModerationLiftBatchInternal, user.moderation.lift, or restored.moderation_lift symbols; only hold-setting fields and tests are present.
    • command: rg -n "requiresModeration|moderation hold|user\.moderation\.lift|liftModerationHold|restoreOwnedSkillsForModerationLift" packages convex src docs specs --glob '!convex/seedSouls.ts'
    • sha: 38c21345906a
  • main_sets_holds: Current main still places moderation holds by setting requiresModerationAt/requiresModerationReason and scheduling owned-skill hiding.
  • unban_is_not_hold_lift: unbanUserWithActor clears deletedAt and banReason but does not clear requiresModerationAt or requiresModerationReason.
  • hold_batch_overwrites_reasons: applyUserModerationToOwnedSkillsBatchInternal rewrites every non-malicious owned skill to user.moderation, which makes later restoration unable to distinguish pre-existing quality/manual hides.
  • future_publishes_remain_hidden: The publish path makes all future skills hidden with user.moderation while user.requiresModerationAt is set.
  • existing_operator_surfaces_not_equivalent: The moderator CLI and users API expose ban/unban/role/restore/reclaim flows, but no moderation-hold lift endpoint or command.
  • not_found_restore_bug: The PR restore path only treats absent, pending, and loading VT statuses as needing scan, while current main maps vtAnalysis.status === not_found to scanner.vt.pending.
  • audit_count_bug: The PR records restoredSkills from only the first restore batch, so scheduled continuation batches are omitted from the user.moderation.lift audit metadata.
  • maintainer_reinvestigation: A maintainer closed this PR, then reopened it with an explicit comment that they were reinvestigating it; GitHub reports the branch merge state as DIRTY.
    • command: GH_CONFIG_DIR=/tmp/gh-readonly gh pr view 1133 --repo openclaw/clawhub --json comments,mergeStateStatus,headRefOid
    • sha: c895190d2d04
  • proof_missing: The PR body lists tests and lint claims, but no redacted terminal output, logs, live output, screenshot, recording, or linked artifact showing a real hold lift after the change.
    • command: GH_CONFIG_DIR=/tmp/gh-readonly gh pr view 1133 --repo openclaw/clawhub --json body,comments
    • sha: c895190d2d04

Likely Related People

  • vincentkoc: recent maintainer and reviewer
    • reason: Reopened this PR for reinvestigation and has recent security/moderation work in VT failure handling, skill redaction hiding, and Convex skill read paths adjacent to the recovery policy.
    • confidence: high
    • commits: 9ea3ed896f26, cd37acadbbfe, 887e81eb8538
    • files: convex/vt.ts, convex/skills.ts, convex/lib/moderationEngine.ts
  • momothemage: recent adjacent owner
    • reason: Authored the current main fix blocking owners from overriding moderator/scanner-hidden skills, which is directly adjacent to this PR's restore eligibility and preservation of moderation provenance.
    • confidence: medium
    • commits: 38c21345906ab1f107a91b33bb86b63667d96643
    • files: convex/skills.ts, convex/skills.undeleteGate.test.ts, convex/httpApiV1/shared.ts
  • Patrick-Erichsen: recent moderation/docs maintainer
    • reason: Recent commits split moderator commands into the private CLI and updated moderation docs/report/appeal wording, which are the operator surfaces this recovery flow would join.
    • confidence: medium
    • commits: 0749f16499b4, cab18339e60d, 4592e66879f9
    • files: packages/clawhub-mod/src/commands/moderation.ts, docs/security.md, convex/skills.ts
  • deepujain: adjacent scanner moderation owner
    • reason: Recent VT Code Insight calibration changed how VT statuses and engine stats affect skill moderation, which is relevant to the PR's unresolved-VT restore decision.
    • confidence: medium
    • commits: d855d09ab0702ff7ad8f2380c790eb40227cc1da
    • files: convex/vt.ts, convex/skills.ts, convex/lib/moderationEngine.ts
  • steipete: adjacent moderation/VT maintainer
    • reason: Recent commits adjusted skill moderation reasons from CLI/API and VT normalization behavior in the same moderation surface.
    • confidence: low
    • commits: 651e54ed7c0d, 370173379786, bc06c472e40c
    • files: convex/skills.ts, convex/vt.ts, docs/security.md

Risks / Open Questions

  • The PR can publish skills with unresolved vtAnalysis.status === not_found because that state is not kept in the scan queue.
  • The restore predicate can reactivate skills whose independent moderation state was overwritten to user.moderation by the hold batch.
  • Large recoveries can undercount restored skills in user.moderation.lift audit metadata because only the first batch count is recorded.
  • The external contributor has not provided after-fix real behavior proof from a real hold-lift run.
  • The branch is currently dirty against main, so it needs maintainer attention before merge mechanics can proceed.

Close Comment

No close comment posted.

GitHub Snapshot

  • comments: 6
  • timeline events: 39
  • related items: 10
  • PR files: 4
  • PR commits: 2

Review Telemetry

  • prompt chars: 131417
  • static prompt chars: 33412
  • context chars: 96719
  • schema chars: 14081
  • additional prompt chars: 0
  • context collection ms: 6343
  • Codex review ms: 253657