Repository navigation
feat(bot): label automod deletes instead of a later plain delete - #382
Conversation
Track in-flight automod REST deletes with a bounded TTL map so gateway message_delete events emit automod_delete (with rule context) instead of a plain delete. Falls back to the message_delete channel when dedicated routing is unset; documents restart limitation in TEMPLATES.md.
|
@pullfrog review |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new event breaks two existing regression tests, and the correlation and snapshot paths have correctness gaps that should be fixed before merging.
Reviewed changes Reviewed the complete PR against #159, including automod dispatch, gateway/snapshot delete handling, event registration, routing, templates, and regression coverage.
- Delete correlation: Adds bounded, expiring process-local marks carrying the triggering rule and consumes them when gateway deletions are reported.
- Report handling: Adds
automod_deleterendering, a dedicated destination withmessage_deletefallback, and separation of marked messages from bulk transcripts. - Configuration and documentation: Registers the event as a core activity-log type without expanding the guild-store event schema and documents its restart limitation.
- Validation: All 18 tests selected by
cargo test -p jmodguard-bot automod_delete --libpass. Both tests selected bycargo test -p jmodguard-bot each_activity_ --libfail withunexpected event automod_delete;cargo fmt --checkalso fails. Current CI logs additionally showclippy::question_markfailing atautomod_delete_marks.rs:124under-D warnings. Validation was local, with SQLite and simulated Discord delivery; no live Discord or provider acceptance testing was performed, and full preflight was not run.
gpt-6.1-sol ｜ 𝕏
|
@pullfrog review |
There was a problem hiding this comment.
Important
Three previously reported behavior issues remain: attribution before REST settlement, missing capture admission for the dedicated destination, and incorrect retained bulk accounting. An additional gap is documented inline: two built-in spam rules bypass the new labeling.
Reviewed changes Reviewed the complete PR against #159, including the follow-up fixes in b7a7b69.
- Delete correlation: Adds bounded, expiring in-memory marks carrying the triggering rule, with REST outcome handling and single-consumer gateway attribution.
- Gateway and snapshots: Classifies single deletes and separates marked messages from bulk reports across retained-storage and fallback paths.
- Configuration and rendering: Registers
automod_deleteas a core activity-log event, adds dedicated routing withmessage_deletefallback, and supplies templates and documentation without changing the guild-store event schema. - Verification: All 18 focused automod-delete tests, 2 activity-event tests, 20 snapshot-owner tests, and 84 existing logging tests pass.
cargo fmt --checkandcargo clippy --locked -p jmodguard-bot --all-targets -- -D warningsalso pass; the earlier activity-test thread is resolved. Validation used local SQLite and simulated Discord delivery, not live providers; full preflight was not run.
gpt-6.1-sol ｜ 𝕏
Only consume confirmed REST deletes, peel automod ids from bulk transcripts with correct totals, enable snapshot capture for automod_delete logs, and register spam enforcement deletes with the same mark lifecycle.
|
@pullfrog review |
There was a problem hiding this comment.
Important
Two behavior gaps remain: successful Gateway-first deletions can miss the new log, and the automatic destination fallback bypasses existing delete ignores.
Reviewed changes Reviewed the complete PR against #159, including the fixes in 4d99647 and all prior review threads.
- Delete correlation: Adds bounded, expiring marks for generic automod and spam deletions, with attribution restricted to confirmed REST success.
- Gateway and snapshots: Classifies single deletions, enables capture for the dedicated destination, and removes marked messages from bulk transcripts while correcting totals and suppressing empty summaries.
- Configuration and rendering: Registers
automod_deleteas a core activity-log event, supplies its template and documentation, and falls back to themessage_deletedestination without changing guild-store event types. - Verification: All 136 selected existing tests pass across automod-delete, activity, logging, snapshot, spam, and guild-store mapping coverage. Formatting, bot Clippy on all targets, and service-boundary checks pass. A temporary production-emitter regression probe reproduces the ignore bypass at its intended assertion (
Sentinstead ofUnconfigured); the probe was removed and the worktree is clean. The four addressed review threads are resolved.
Validation used local SQLite, loopback fixtures, and simulated Discord delivery. No live Discord/provider acceptance testing or full preflight was run.
gpt-6.1-sol ｜ 𝕏
Note gateway deletes before REST confirm and emit automod_delete after successful settlement; inherit message_delete ignore filters for the automod_delete fallback route.
|
@pullfrog review |
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 5 other issues
Security issues:
- Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/bot/src/spam_actions.rs" line_range="528-531" />
<code_context>
- Ok(crate::discord::DiscordCall::Finished(Ok(()))) => CaseOutcome::Succeeded,
- Ok(crate::discord::DiscordCall::Finished(Err(error))) => delete_outcome(&error),
- Ok(crate::discord::DiscordCall::Cancelled) | Err(_) => CaseOutcome::Uncertain,
+ Ok(crate::discord::DiscordCall::Finished(Ok(()))) => {
+ app.automod_delete_marks
+ .confirm(found.guild, *channel, *message);
+ CaseOutcome::Succeeded
+ }
+ Ok(crate::discord::DiscordCall::Finished(Err(error))) => {
</code_context>
<issue_to_address>
**Spam deletions lose automod label**
When a spam-delete gateway event arrives before its REST request succeeds, `punish_proven_match` discards the rule identity returned by `AutomodDeleteMarks::confirm`, so the gateway’s plain `message_delete` report is never reconciled into an `automod_delete` report. The deletion appears only as an ordinary message deletion.
Use the identity returned by `confirm` to emit the reconciled automod-delete report.
</issue_to_address>
### Comment 2
<location path="crates/bot/src/logging.rs" line_range="2207-2213" />
<code_context>
+) -> MessageReport {
+ app.automod_delete_marks
+ .note_gateway_delete(guild_id.get(), channel.get(), id.get());
+ let mut report = if let Some((rule_id, rule_name)) =
+ app.automod_delete_marks
+ .take(guild_id.get(), channel.get(), id.get())
+ {
+ automod_delete_report(app, guild_id, channel, id, &rule_id, &rule_name)
+ } else {
+ message_delete_report(app, guild_id, channel, id)
+ };
+ if let Some(row) = retained {
</code_context>
<issue_to_address>
**Automod deletions appear twice**
When the gateway delete is processed before the automod REST request succeeds, `observed_delete_report` emits a plain `message_delete` report before REST settles, then successful REST confirmation emits a separate `automod_delete` report. Users see two reports for one deletion, and bulk summaries can count it as a plain deletion.
Reconcile the gateway-first event into a single automod-delete report instead of emitting a plain report followed by a second report.
Also at `crates/bot/src/automod.rs:448-455`, `crates/bot/src/logging.rs:2214`, `crates/bot/src/logging.rs:2240`, `crates/bot/src/snapshot_capture.rs:1492-1494`.
</issue_to_address>
### Comment 3
<location path="crates/bot/src/automod.rs" line_range="468-470" />
<code_context>
+ }
+ }
+ crate::discord::DiscordCall::Finished(Err(error)) => {
+ app.automod_delete_marks
+ .remove_unconfirmed(guild_id, channel, message);
+ app.remembered_discord_targets.remember_rest_rejection(
+ recovery,
+ &error,
+ target,
+ Instant::now(),
+ );
+ }
+ crate::discord::DiscordCall::Cancelled => {
+ app.automod_delete_marks
</code_context>
<issue_to_address>
**Uncertain deletes lose classification**
When discord receives a delete request but the local call is cancelled or times out before its response arrives, the cancellation and timeout paths call `remove_unconfirmed` even though Discord may still complete the request. The later gateway event has no mark to match and is logged as an ordinary message deletion.
Keep marks for requests with uncertain outcomes until they can be reconciled or expire.
Also at `crates/bot/src/spam_actions.rs:538-542`.
</issue_to_address>
### Comment 4
<location path="crates/bot/src/snapshot_capture.rs" line_range="1492-1493" />
<code_context>
+ row.as_ref(),
+ );
+ if report.event == "automod_delete" {
+ transcript.peel_automod_delete();
+ submit_report(&app, &work.context, report);
+ } else if row
.as_ref()
</code_context>
<issue_to_address>
**Bulk-only logs lose automod deletes**
When a guild has a `message_bulk_delete` channel configured but neither an `automod_delete` nor `message_delete` channel, the bulk paths remove automod-marked IDs from the bulk transcript and submit them only as individual `automod_delete` reports. When that event has no route, those reports are unconfigured and the IDs disappear from the configured bulk log too.
Keep automod-marked IDs in the bulk transcript whenever their individual report cannot be routed.
Also at `crates/bot/src/snapshot_capture.rs:1634-1636`, `crates/bot/src/handlers.rs:1426-1429`.
</issue_to_address>
### Comment 5
<location path="crates/bot/src/logging.rs" line_range="2205-2209" />
<code_context>
{
return;
}
+ app.automod_delete_marks
+ .insert(guild_id, channel, message, rule_id, rule_name);
let recovery = app
</code_context>
<issue_to_address>
**Gateway race emits two reports**
When REST confirmation runs between the gateway handler’s `note_gateway_delete` and `take` calls, `note_gateway_delete` and `take` run under separate locks, so REST confirmation can remove the pending mark between them and emit a reconciled automod report. The gateway handler then finds no mark and reports the same deletion as plain, producing both log lines.
Make recording the gateway observation and consuming any confirmed mark one atomic operation.
</issue_to_address>
### Comment 6
<location path="crates/bot/src/snapshot_capture.rs" line_range="2152" />
<code_context>
"ws://{}",
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Needs a human reviewer. 6 findings to address first, and if the gateway delete arrives before the REST call confirms, it is logged immediately as a plain delete; a later REST success then emits a second automod-delete log for the same message. Those duplicate channel messages remain after reverting, though the number is bounded and they can be removed manually.
Blocking findings: crates/bot/src/spam_actions.rs:531, crates/bot/src/logging.rs:2213, crates/bot/src/automod.rs:470, crates/bot/src/snapshot_capture.rs:1493, crates/bot/src/logging.rs:2209, and 1 more
There was a problem hiding this comment.
Important
Gateway-first handling still needs changes: reconciliation loses author exclusions, and spam deletion drops the reconciliation result. The previous single-report requirement also remains unmet: a successful generic deletion can emit both plain and automod-delete reports.
Reviewed changes Reviewed the complete PR against #159, including e8370f5 and the prior review threads.
- Delete correlation: Adds bounded, expiring marks for generic automod and spam deletions, confirmed-only attribution, and Gateway-first reconciliation.
- Snapshot and bulk handling: Enables snapshot capture for the dedicated destination and separates confirmed automod deletions from plain bulk transcripts, correcting totals and suppressing empty summaries.
- Configuration and rendering: Registers
automod_deleteas a core activity-log event, supplies its template and documentation, and inherits the existing delete destination and filters without changing guild-store event types. - Verification: All 137 selected existing bot tests and the guild-store mapping contract test pass. Formatting, bot Clippy on all targets with warnings denied, and service-boundary checks pass. Temporary regression probes reproduced duplicate delivery, bypass of an ignored author, and missing Gateway-first spam delivery at their intended assertions; the probes were removed and the worktree is clean. Validation used local SQLite, loopback HTTP fixtures, and simulated log delivery, not live Discord/provider acceptance. Full preflight was not run.
gpt-6.1-sol ｜ 𝕏
Atomize gateway mark observation, suppress interim plain deletes while REST is pending, preserve snippet identity for reconciled automod reports, and emit spam reconcile results from punish_proven_match.
|
@pullfrog review |
There was a problem hiding this comment.
Important
Three logging regressions remain: pending deletions can disappear after unsuccessful REST settlement, reconciliation loses retained snapshot context, and bulk-only logging drops marked IDs. Each is reproduced by a temporary regression probe and documented inline.
Reviewed changes Reviewed the complete PR against #159, including afc257b, c1b05cc, and the prior review discussions.
- Delete correlation: Adds bounded, expiring marks for generic automod and spam deletions, atomic Gateway classification, and success-driven reconciliation while suppressing interim plain reports.
- Snapshot and bulk handling: Enables capture for the dedicated destination and separates marked deletions from retained and fallback bulk transcripts, adjusting totals and suppressing empty summaries.
- Configuration and rendering: Registers
automod_deleteas a core activity-log event, adds its template and documentation, and inherits the existing delete destination and filters without expanding guild-store event types. - Verification: All 140 selected existing bot tests and the guild-store mapping contract test pass. Formatting, bot Clippy on all targets with warnings denied, and service-boundary checks pass. Four temporary probes fail at the intended assertions, demonstrating the three findings; the probes were removed and the worktree is clean. Validation used local SQLite, loopback HTTP/WebSocket fixtures, and simulated report delivery, not live Discord/provider acceptance. Full preflight was not run.
gpt-6.1-sol ｜ 𝕏
|
@pullfrog review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 3 other issues
Security issues:
- Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/bot/src/automod.rs" line_range="458-459" />
<code_context>
+ }
+ }
+ crate::discord::DiscordCall::Finished(Err(error)) => {
+ app.automod_delete_marks
+ .remove_unconfirmed(guild_id, channel, message);
+ app.remembered_discord_targets.remember_rest_rejection(
+ recovery,
</code_context>
<issue_to_address>
**Observed deletions disappear**
When a gateway delete arrives while its automod REST request is pending and the request fails, is cancelled, or remains uncertain, `observed_delete_report` suppresses the pending event, then settlement removes or eventually expires the mark without replaying it. The deletion never reaches a log.
Preserve and emit or reconcile the pending gateway observation when REST settlement fails, is cancelled, or remains uncertain.
Also at `crates/bot/src/spam_actions.rs:546-551`, `crates/bot/src/logging.rs:2258`, `crates/bot/src/automod.rs:467`, `crates/bot/src/snapshot_capture.rs:1642-1646`, `crates/bot/src/automod_delete_marks.rs:37-51`, `crates/bot/src/automod_delete_marks.rs:116-117`.
</issue_to_address>
### Comment 2
<location path="crates/bot/src/automod_delete_marks.rs" line_range="49-50" />
<code_context>
+ self.order.pop_front();
+ self.entries.remove(&key);
+ }
+ self.entries.retain(|_, entry| entry.expires > now);
+ self.order.retain(|key| self.entries.contains_key(key));
+ }
+
</code_context>
<issue_to_address>
**Delete-mark pruning scans all entries**
When a mark is inserted, observed, confirmed or removed, `AutomodDeleteMarks::prune` calls `HashMap::retain` and `VecDeque::retain` on each mark operation, scanning the full mark set under the mutex. Ordinary delete traffic therefore incurs O(n) work per message instead of avoiding full-map scans.
Prune expired marks incrementally using an expiry-ordered structure so per-message operations do not scan the full mark set.
</issue_to_address>
### Comment 3
<location path="crates/bot/src/automod_delete_marks.rs" line_range="244-254" />
<code_context>
+ fn ttl_expired_marks_do_not_relabel() {
+ let mut marks = AutomodDeleteMarks::default();
+ let now = Instant::now();
+ marks.insert(1, 2, 3, "badword", "Bad word", now);
+ assert!(
+ marks
+ .take(
+ 1,
+ 2,
+ 3,
+ now + AUTOMOD_DELETE_MARK_TTL + Duration::from_secs(1)
+ )
+ .is_none()
+ );
+ }
+
</code_context>
<issue_to_address>
**TTL test ignores expiration**
When expiry pruning is removed or stops expiring marks, when TTL pruning is broken, `ttl_expired_marks_do_not_relabel` still passes: its unconfirmed mark makes `take` return `None` regardless of expiry, so the assertion never checks expiration.
Confirm the mark before calling `take` with the advanced time, so the assertion depends on expiration pruning.
</issue_to_address>
### Comment 4
<location path="crates/bot/src/snapshot_capture.rs" line_range="2166" />
<code_context>
"ws://{}",
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and if the delete is mislabeled or routed incorrectly, the bot can post an automod-delete notice—with available message details—to a configured Discord log channel. Reverting stops future notices but cannot retract messages already sent.
Blocking findings: crates/bot/src/automod.rs:459, crates/bot/src/automod_delete_marks.rs:50, crates/bot/src/automod_delete_marks.rs:254, crates/bot/src/snapshot_capture.rs:2166
There was a problem hiding this comment.
Important
The three previously reported logging regressions remain: pending deletions disappear without REST success, reconciliation discards retained snapshot context and can bypass author exclusions, and bulk-only logging loses marked IDs. These still need fixes before merging.
Reviewed changes Re-reviewed the complete PR against #159 at c1b05cc, which is unchanged from the previous Pullfrog review; no additional findings were identified, so the existing inline threads are retained rather than duplicated.
- Delete correlation: Adds bounded, expiring rule-aware marks for generic automod and spam deletions, atomic Gateway classification, and success-driven reconciliation while suppressing interim plain reports.
- Snapshot and bulk handling: Enables snapshot capture for the dedicated event and separates marked deletions from retained and fallback bulk transcripts, correcting totals and suppressing empty summaries.
- Configuration and rendering: Registers
automod_deleteas a core activity-log event, supplies its template and documentation, and inherits the existing delete destination and filters without expanding guild-store event types. - Verification: All 140 selected bot tests and the guild-store mapping contract test pass.
cargo fmt --checkand bot Clippy on all targets with warnings denied also pass. The service-boundary script passes its static checks and root/bot dependency checks, but cannot complete the API dependency check:failed to download aws-lc-rs v1.18.1, caused byattempting to make an HTTP request, but --offline was specified. Validation used local SQLite, loopback fixtures, and simulated Discord delivery; no live provider acceptance testing or full preflight was run. The worktree is clean.
gpt-6.1-sol ｜ 𝕏
Replay plain deletes when REST abandons a gateway-pending mark, peek snippet identity for reconciled automod reports, and only peel automod IDs from bulk transcripts when an individual automod route exists.
|
@pullfrog review |
There was a problem hiding this comment.
Important
Three logging regressions remain after the latest fixes: deferred settlement loses retained snapshot context, bulk-only fallback paths still drop marked IDs, and timed-out deletions can remain unreported. Updated findings and reproductions are inline.
Reviewed changes Reviewed the complete PR against #159, including the latest reconciliation fixes in 5cceba9 and the prior review discussions.
- Delete correlation: Adds bounded, expiring rule-aware marks for generic automod and spam deletions, atomic Gateway classification, confirmed-success reconciliation, and plain-delete replay after explicit REST failure.
- Snapshot and bulk handling: Enables capture for the dedicated event and separates marked deletions from bulk transcripts, with route-aware peeling in the retained worker and corrected summary accounting.
- Configuration and rendering: Registers
automod_deleteas a core activity-log event, adds its template and documentation, and inherits the existing delete destination and filters without expanding guild-store event types. - Verification: All 143 selected existing bot tests and the guild-store mapping contract test pass. Formatting and bot Clippy on all targets with warnings denied pass; three temporary regression probes fail at their intended assertions and were removed afterward. The service-boundary script passes its static and root/bot dependency checks, but cannot complete the API dependency check:
failed to download aws-lc-rs v1.18.1, caused byattempting to make an HTTP request, but --offline was specified.
Validation used local SQLite, loopback HTTP/WebSocket fixtures, simulated report delivery, and a mark-state expiry probe. No live Discord/provider acceptance testing or full preflight was run. The worktree is clean.
gpt-6.1-sol ｜ 𝕏
Replay plain message_delete when REST is cancelled or uncertain after a gateway-pending mark, trim mark pruning to front-expiry only, and load guild config before unqueued bulk automod peeling so bulk-only logging does not mis-route deletes.
|
@pullfrog review |
Stash retained snapshots while gateway-first automod deletes wait on REST, reapply them on reconcile and plain replay, keep bulk-only guilds from peeling unroutable marks, and retain uncertain marks until explicit settlement.
|
@pullfrog review |
… ownership. A full live-mark cache still deletes the message and reports automod_delete on the dedicated channel, suppressing a later plain gateway delete. Bulk placement records whether the id stayed in the transcript or was peeled, and settlement keeps that choice if log routes change.
|
@pullfrog review |
There was a problem hiding this comment.
Important
Three issues remain: overflow correlation leaks Gateway state, overlapping saturated attempts can double-log one deletion, and a deferred successful delete can lose its only configured bulk destination. Each is reproduced by a temporary regression probe.
Reviewed changes Reviewed the complete PR against #159 at 9827c80, including the current implementation and prior review discussions.
- Delete attribution: Adds rule-aware REST/Gateway correlation for generic automod and spam deletion, overlapping-attempt settlement, uncertain outcomes, and overflow handling.
- Snapshots and bulk delivery: Preserves retained context, separates individually routed deletes from bulk transcripts, and tracks delivery ownership with bounded report admission and ambiguous-send suppression.
- Configuration and rendering: Registers
automod_delete, adds its template and documentation, enables snapshot capture for its dedicated destination, and inherits themessage_deletedestination and filters. - Verification: The 116 focused reconciliation/snapshot tests, 111 broader logging/activity/spam tests, and guild-store mapping contract test pass. Formatting, whitespace checks, and bot all-target Clippy with warnings denied pass. Three temporary probes fail at their intended assertions and were removed; the worktree is clean.
- Validation limits: Tests used local SQLite, recording sinks, and loopback fixtures, not live Discord/provider acceptance. Full local preflight was not run. Service-boundary static and root/bot dependency checks pass, but API dependency inspection stops with
failed to download aws-lc-rs v1.18.1, caused byattempting to make an HTTP request, but --offline was specified. Hosted CI passed on the reviewed head.
⚠️ Overflow Gateway State Outlives Its Owner
[P2] The overflow path has no corresponding cleanup for its per-message gateway entries: successful settlement creates or retains them, but expiry removes only the unmarked row. Repeated saturation therefore produces uncapped process-lifetime memory growth despite the 4,096-entry limits on the two primary maps.
Technical details
# Missing Gateway-state cleanup for overflow deletions
## Affected sites
- [automod_delete_marks.rs:589-595](https://github.com/jatmn/jModGuard/blob/9827c800f94d6858c62ae57c33a66da3ad8a0464/crates/bot/src/automod_delete_marks.rs#L589-L595): `mark_unmarked_emitted` creates a settled `gateway` entry even for REST-first success.
- [automod_delete_marks.rs:451-464](https://github.com/jatmn/jModGuard/blob/9827c800f94d6858c62ae57c33a66da3ad8a0464/crates/bot/src/automod_delete_marks.rs#L451-L464): `expire_unmarked` removes the overflow owner without reclaiming its Gateway state.
- [automod_delete_marks.rs:615-648](https://github.com/jatmn/jModGuard/blob/9827c800f94d6858c62ae57c33a66da3ad8a0464/crates/bot/src/automod_delete_marks.rs#L615-L648): observed failure also removes the overflow row while leaving settled Gateway state behind.
- Normal stored-mark cleanup removes Gateway state when the last mark disappears; overflow-only targets never reach that cleanup.
## Reproduction
A temporary state-level test kept 4,096 live pending filler marks, completed two successive batches of 4,096 distinct overflow deletions through `settle_unmarked`, and advanced pruning beyond both overflow TTLs. `unmarked` was empty and only the 4,096 fillers remained in `entries`, but `gateway.len()` was 12,288 rather than 4,096. The independent cleanup assertion failed, and the probe was removed. This measures state cardinality, not production memory usage or live provider behavior.
## Required outcome
- Reclaim overflow-owned Gateway state when its last reporting/suppression owner is gone, without discarding a live mark or in-flight bulk owner.
- Keep auxiliary per-message state bounded across REST-first success, observed failure/uncertainty, and overflow expiry.
- Add churn coverage that asserts auxiliary map sizes after settlement and TTL, not only the sizes of `entries` and `unmarked`.gpt-6.1-sol ｜ 𝕏
…overlap. A deferred success keeps the bulk destination if that send never lands, overlapping saturated attempts share one overflow owner, and gateway state for those rows is dropped once the owner is gone.
|
@pullfrog review |
There was a problem hiding this comment.
Important
Two overflow-path issues remain: recovering mark capacity can produce duplicate delete reports, and failed overflow attempts accumulate expiry metadata outside the fixed cap.
Reviewed changes Full review of the automod-delete logging implementation and its integration with moderation, Gateway handling, and retained snapshots for #159.
- Delete correlation: Attempt-scoped marks reconcile REST success, failure, and uncertainty with single and bulk Gateway deletions, including overlapping attempts and saturated caches.
- Report ownership and delivery: Bulk transcripts coordinate deferred reports, routing changes, ambiguous sends, attachment fallbacks, and pre-spawn admission.
- Configuration and context:
automod_deleteadds channel/filter fallback, retained message context, coverage labels, templates, and documentation without introducing durable event rows. - Verification: The 117 focused correlation/logging/snapshot tests,
cargo fmt --check, and service-boundary checks passed. Two temporary regression probes failed at the intended assertions, confirming the findings below; they were removed and the worktree is clean. These used local SQLite, recording sinks, and modeled REST completions, not live Discord. Full preflight and live-provider validation were not run.
⚠️ Overflow expiry metadata bypasses the cap
[P2] Failed overflow attempts remove their live rows but leave their expiry records until the ten-minute deadline, with no bounded cleanup or compaction. Sustained failure churn therefore grows this index with cumulative attempts rather than the 4,096-row budget, adding memory and later pruning work under the shared marks mutex.
Technical details
# Missing bounded cleanup for the overflow expiry index
## Affected sites
- [automod_delete_marks.rs:490-501](https://github.com/jatmn/jModGuard/blob/0db5106e2306e0e0b912efb4c9d0d3e3bc974fd1/crates/bot/src/automod_delete_marks.rs#L490-L501): every newly admitted overflow row appends an expiry record.
- [automod_delete_marks.rs:695-708](https://github.com/jatmn/jModGuard/blob/0db5106e2306e0e0b912efb4c9d0d3e3bc974fd1/crates/bot/src/automod_delete_marks.rs#L695-L708): the last failed attempt removes the live row without reclaiming that record.
- [automod_delete_marks.rs:504-517](https://github.com/jatmn/jModGuard/blob/0db5106e2306e0e0b912efb4c9d0d3e3bc974fd1/crates/bot/src/automod_delete_marks.rs#L504-L517): only due deadlines reclaim stale records; the heap compaction elsewhere does not cover `unmarked_expiry`.
## Reproduction
A temporary unit probe filled the 4,096 live marks, then inserted and removed 50,000 distinct overflow attempts at the same simulated instant through production `insert` and `remove_unconfirmed`. It reached the bound assertion with `live overflow=0, expiry entries=50000`. There were no retained payloads or provider calls in this probe; this measures index entries, not process memory usage. The probe was removed.
## Required outcome
- Bound auxiliary overflow-expiry metadata independently of failure traffic within the TTL window.
- Preserve unresolved ownership and incremental maintenance rather than scanning the full map on every deletion.
- Extend churn coverage to failed overflow removals, not only successful live-mark consumption.gpt-6.1-sol ｜ 𝕏
Scope auditThis PR is for the #159 bullet "a distinct log line for automod deletes (today they can appear later as a plain message delete)." It has grown to about 9,200 lines across 18 files and more than 30 review rounds. It added race settlement, a bounded mark cache, bulk-delete reconciliation, REST uncertainty handling and overflow handling, none of which the bullet asks for. Guideline for any redo:
No more fix pushes here until the owner decides whether to close and redo this PR. |
I record a short-lived mark when automod or spam deletes a message and emit automod_delete for the later gateway delete while that mark is live. A miss stays a plain message_delete. I drop the bulk, overflow, and replay correlation from this branch.
|
@pullfrog review |
There was a problem hiding this comment.
Important
One performance regression remains: ordinary single-message deletions now scan the process-wide automod mark table while holding its mutex. This can be fixed within the trimmed best-effort scope, without restoring bulk reconciliation or overflow handling.
Reviewed changes Reviewed the complete 13-file diff against #159 and the owner's scope clarification, including the current single-delete reporting paths and relevant regression tests.
- Delete labeling: Successful generic automod and spam deletions record short-lived rule/reason marks, and matching single Gateway deletes use
automod_deleteinstead ofmessage_delete. - Routing and rendering: Registers the activity event and template, supports a dedicated destination, and inherits the ordinary delete destination and ignore list when falling back.
- Scope and storage: Keeps missing/expired marks as ordinary deletes and leaves bulk/replay machinery out; the guild-store event schema remains unchanged.
- Verification: Focused automod-delete tests, 98 logging tests, 20 snapshot tests, 7 spam-action tests, and 2 activity-event tests pass.
cargo fmt --checkand bot all-target Clippy with warnings denied pass. An isolated probe using the production mark implementation confirms the lookup scaling described inline. Tests used local SQLite and loopback/simulated Discord boundaries, not live providers; full preflight was not run. No repository files were changed, and the worktree is clean.
gpt-6.1-sol ｜ 𝕏
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
Merge readiness
- Reviewed head:
4ee8caf(merge-base9220495, currentmain). The main merge only picked up the #389parking_lotbump; no automod/mark/logging hunks differ fromc937632. - Scope (#159 only): when this bot's automod/spam successfully REST-deletes a message, a short-lived mark lets the gateway single delete log
automod_delete; a missing or expired mark staysmessage_delete, with the channel/filter fallback described in the PR body. - Scope-drift audit: verified the trim. The diff is the mark-on-success design only (+470/−18, 13 files). No bulk, overflow, replay, settlement or cap machinery.
- CI: all
Checks / *jobs,pullfrog, Socket and plan pass.pullfrog-approvalis failing (bot policy gate, not a code defect in this diff). Sourcery review skipped. - Bot threads: the Pullfrog
src/types.rseach_activity_*sample thread is fixed on this head. Older Pullfrog/Sourcery threads on bulk/settlement/gateway-reconcile code target removed machinery and are stale.
Findings
(none)
Backlog (#390 / out of scope)
Reviewed strictly against the #159 item. These are not defects for this PR and belong on #390 or a follow-up:
- Mark-map cap, overflow rows and eviction under load (TTL-only prune in this diff).
- Pullfrog:
prune()runs a full-mapretainon everytake_live/remember. Real, but a performance/cap concern, not #159 labeling. - Bulk delete peeling, bulk transcript totals, bulk-only routing, per-id
automod_deleteinside bulk events. - Replay of unconfirmed plain deletes, retained snapshot payloads, settlement heaps, gateway-first reconciliation.
- Restart-durable marks (process-local map; a missing mark after restart logs a plain delete, which the PR accepts).
Drift recheck: a gateway delete that arrives before the REST remember logs message_delete because the mark is missing. That matches the stated contract, so it is not a finding. No backlog item was promoted to Findings.
— jatmn's PR Review agent
Single-delete lookup and insert were pruning every live mark under the shared mutex. Due deadlines are now dropped from the front, and a record that no longer matches is skipped, so a keyed read does not walk the table. The ten-minute window and best-effort matching stay the same.
|
@pullfrog review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes across the current PR against main, using the owner's narrowed scope: best-effort single-delete attribution, with missing or expired marks allowed to remain plain deletes.
- Automod attribution: Successful automod and spam deletions record rule/reason marks; single-delete reports consume a live mark to select
automod_delete. - Routing and templates: The new event has a code-default template and optional dedicated channel, falling back to the
message_deletechannel and its ignore filter. - Expiry cleanup: The latest change replaces full-map pruning with deadline-front reclamation while preserving keyed consumption, expiry fallback, and replacement behavior.
- Verification: Both focused bot test runs passed (7 and 5 tests), covering mark expiry/consumption, report classification, spam marking, routing/filter fallback, and activity-event contracts.
cargo fmt --checkand the service-boundary checks also passed. These use local SQLite, recording sinks, and loopback HTTP fixtures, not live Discord. The full preflight was not run.
gpt-6.1-sol ｜ 𝕏
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer (Dustin's review agent)
Head 4b27fcc, whole diff vs merge-base 9220495, scope-drift audit against #159, drift recheck done. One P2 finding.
Scope drift audit
#159's automod-delete bullet is in scope and implemented. There's no labeling drift, and #390 still owns bulk, cap and replay. Fix-push guidance: stop iterating on label and routing changes. The only remaining work is the boundedness gap that 4b27fcc introduced, described below.
Findings
🟡 P2: keep the automod delete mark expiry deque bounded after consume
📍 crates/bot/src/automod_delete_marks.rs: remember, take_live, reclaim_expired.
💥 4b27fcc adds an expiry: VecDeque that remember pushes onto every time, but take_live removes the mark only from entries. Each consumed mark leaves an orphan deque cell that is reclaimed only once the front deadline ages out. With one live front mark and heavy delete volume, expiry can grow far past entries. For example, entries == 1 while expiry holds about 50k cells. reclaim_expired can then drain that backlog while holding the global marks mutex.
📜 Contract (AGENTS.md): "Automod state must stay bounded: prune buckets, keep lookups O(1) per-message (per-(guild,user) buckets), no full-map scans."
🏷️ Attribution: introduced by 4b27fcc. Parent 4ee8caf was map-only and stayed bounded by live marks.
🔧 Correction: when take_live succeeds, remove or quarantine the matching expiry entry, or use a structure where consuming a mark shrinks the auxiliary state. Keep O(1) keyed lookup and add no full-map retain. Add a regression test that bounds expiry size after a burst of remember+consume behind a blocked front mark.
🚫 Out of scope: #390 caps, replay and bulk; the snapshot capture gate.
Backlog (#390, not findings)
- Bulk peel, the 4096 cap, replay and overlap heaps.
- Snapshot retention when only an
automod_deletechannel is configured.
Drift recheck
- Gateway delete before REST
rememberwas dropped as drift. With no live mark it staysmessage_delete, as documented. - Marks being lost on restart was dropped as drift. Marks are kept in memory only, and plain
message_deletewhen a mark is missing is the documented behavior. - The full-map scan Pullfrog flagged is fixed on this head.
Automated reviews and CI
Pullfrog found no new issues on 4b27fcc, and its only inline thread (src/types.rs) is resolved. Sourcery skipped this head. All checks pass.
Consumed marks unlink their deadline cell, and remembering the same message again replaces that cell, so deadlines stay within the live set.
|
@pullfrog review |
|
🤖 PR Reviewer (jatmn's review agent) Review of head 🟢 Thanks for the contribution. I do not see any actionable issues from my review. Merge readinessMerge-base decision Scope drift auditAt this head the net diff vs Scope note (recheck)A candidate P2 (reverse Backlog (not findings for this head)
Pullfrog / Sourcery on head
|
There was a problem hiding this comment.
✅ No new issues found within the agreed best-effort scope.
Reviewed changes Reviewed the complete 13-file diff at 803d1f4 against #159 and the owner's narrowed scope; the PR is now merged.
- Delete attribution: Generic automod and spam success paths record rule/reason marks, and single-delete reporting consumes a live mark to select
automod_delete; missing or expired marks remain plain deletes. - Expiry index: Deadline-front reclamation avoids full-map scans, while consumption and replacement unlink and reuse deadline cells rather than accumulating consumed entries behind a live front.
- Routing and templates: Registers the new core activity event and template, supports a dedicated destination, and inherits the
message_deletedestination and ignore filter when falling back, without changing guild-store event types. - Scope boundaries: Best-effort misses are accepted here; bulk reconciliation, replay, and capacity/overflow policy remain deferred to #390.
- Verification: The focused bot run passed 8 tests, adjacent logging/snapshot/spam/activity coverage passed 133 tests, and core event/guild-store contracts passed 11 tests. Formatting, bot all-target Clippy with warnings denied, and complete service-boundary checks passed. An isolated probe also matched the production mark implementation against an independent reference model over 250,000 mixed operations. Current-head CI is green. Local integration coverage used SQLite and loopback/simulated Discord boundaries; no live provider acceptance testing or full local preflight was run. No repository files were changed, and the worktree is clean.
gpt-6.1-sol ｜ 𝕏
Part of #159
Part of #159
I trimmed this to the automod-delete bullet. When this bot's automod, including spam, deletes a message, I log a distinct
automod_deleteline with the rule, rule id, message, and reason. I keep a short-lived mark so the later single gateway delete is not also logged as a plain delete. If the mark is missing or expired, that gateway delete staysmessage_delete. Theautomod_deletechannel falls back tomessage_deleteand uses that ignore list.Bulk peel and settle, the 4096 cap and overflow rows, replay of unconfirmed plain deletes, retained snapshots, and the overlap and expiry heaps are not in this pull request. I saved that machinery on
backlog/382-delete-correlationand wrote it up in #390.I checked formatting, workspace clippy with warnings denied, and the rust-core and rust-bot test suites.
Summary (trimmed scope)
automod_deleteactivity event with a short template: rule, rule id, message, reason.automod_deleteinstead of a plain delete.message_deleteline.automod_deleterouting falls back to themessage_deletechannel and its ignore filter.I removed the earlier Sourcery summary because it described the pre-trim draft.