Skip to content

feat: track public Discord invite counts - #240

Open
hannesrudolph wants to merge 6 commits into
mainfrom
feat/metrics-collection
Open

hannesrudolph wants to merge 6 commits into
mainfrom
feat/metrics-collection

Conversation

@hannesrudolph

@hannesrudolph hannesrudolph commented Sep 15, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

Tracking Discord server membership and online-presence trends currently requires a separate collector, even when Discord collection already runs through Discrawl.

User Impact

discrawl metrics collect|import|status --config METRICS_CONFIG records public invite counts and imports existing metric history into a separate database without bot credentials. Existing archive, tail, and embedding workflows are unchanged. Online presence is an approximate count of online members, not active posters or engagement.

Why This Change Was Made

The collector uses Discord's public invite count fields and stores timestamped observations with provenance and delivery sequences. The documented configuration uses OpenClaw's public guild invite. Unknown counts stay NULL, and zeroes, decreases, repeated observations, and daily revisions are preserved. Imports are scoped and idempotent by ID, with resumable batches. Database ownership/version checks prevent initialization over an existing archive or another collector's store.

Collection is an explicit command; scheduling and downstream delivery remain with the caller.

Evidence

  • Full go test -race -count=1 ./... passed; coverage excluding generated code is 86.0%. Focused race tests passed again after the final path guard.
  • golangci-lint reports zero issues; vet, staticcheck, gosec, deadcode, module verification, and documentation tests/build passed. Govulncheck found no reachable vulnerabilities.
  • Tests cover archive preservation, competing database owners, dangling paths, partial reads, NULL/zero values, decreases, daily revisions, redirects, rate limits, and interrupted/idempotent imports.
  • The staged binary passed live public-invite collection, idempotent fixture import, and read-only status checks in an isolated metrics database containing no archive tables.

@clawsweeper

clawsweeper Bot commented Sep 15, 2026

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.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 15, 2026
@clawsweeper

clawsweeper Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 15, 2026, 6:44 PM ET / 22:44 UTC (Revision 2).

ClawSweeper review

What this changes

Adds commands to collect public Discord membership and online counts, import historical observations, and report freshness from a separate SQLite database.

Merge readiness

Blocked before merge - 5 items remain

This remains useful, distinct work absent from current main. Both previously reported database-safety defects remain unchanged, so the PR is still blocked before merge.

Priority: P2
Reviewed head: 7b334b18e407deda8ce23f36677c4f9ec81e1141

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The feature has substantial implementation and test coverage, but two previously identified storage-safety defects still block readiness.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from ordinary contributor proof requirements; its body reports staged-binary collection, import, and status against an isolated metrics store, but those happy-path claims do not resolve the two source-proven initialization defects.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from ordinary contributor proof requirements; its body reports staged-binary collection, import, and status against an isolated metrics store, but those happy-path claims do not resolve the two source-proven initialization defects.
Evidence reviewed 9 items Applicable repository policy: No root or applicable ancestor AGENTS.md exists. The only tracked AGENTS.md belongs to the unrelated autoreview skill subtree; no maintainer-notes directory was found.
Both previous findings remain: The focused comparison with the earlier reviewed head is empty. Both metrics.go and metrics_test.go also have identical blob IDs at the two revisions. The previous review's path-normalization and initialization-retry findings therefore remain applicable.
Unchecked destination and failed-initialization residue: Open checks the untrimmed path with Lstat, and initialize reserves that same filename before handing it to Crawlkit. Initialization errors return without removing the newly reserved file; subsequent attempts require metadata that the failed transaction never committed.
Findings 2 actionable findings [P1] Validate the same database path that Crawlkit will open
[P2] Make failed first-time initialization safely retryable
Security None None.

How this fits together

Discrawl normally archives Discord messages and members. The new metrics commands instead accept public invite codes or historical JSON rows and write aggregate observations to an explicitly configured database for downstream readers.

flowchart TD
  A[Metrics configuration] --> B[Metrics commands]
  C[Public Discord invite counts] --> B
  D[Historical JSON rows] --> B
  B --> E[Database ownership check]
  E --> F[Separate metrics database]
  F --> G[Freshness output and downstream readers]
Loading

Before merge

  • Validate the same database path that Crawlkit will open (P1) - This prior blocker remains unchanged. On Unix, configure an absent filename ending in a space while the trimmed filename is an existing archive. Lstat and O_EXCL check/create the spaced filename, but Crawlkit v0.16.3 trims the path before opening it. The following transaction then adds metrics tables and ownership metadata to the unchecked archive. Reject noncanonical paths or normalize consistently before every filesystem check and open.
  • Make failed first-time initialization safely retryable (P2) - This prior blocker also remains unchanged. Initialization reserves the final filename before opening SQLite, but errors leave that file behind. For example, an already-canceled context makes Store.Open fail after reservation; the next healthy attempt sees an existing file and rejects it because metric_meta was never committed. Clean up only the file owned by the failed attempt or publish a fully initialized store safely, while continuing to reject unrelated empty databases.
  • Resolve merge risk (P1) - An absent metrics filename ending in whitespace can cause writable initialization of an existing archive at the trimmed path, violating the promised separation of stores.
  • Resolve merge risk (P1) - Cancellation or schema-creation failure during first initialization leaves an unowned database file that prevents subsequent collection or import until manually recovered.
  • Complete next step (P2) - Fix both database-initialization findings and add archive-preservation and failed-creation retry regression coverage before merge.

Findings

  • [P1] Validate the same database path that Crawlkit will open — internal/headlinemetrics/metrics.go:130-138
  • [P2] Make failed first-time initialization safely retryable — internal/headlinemetrics/metrics.go:130-143
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth production +468, tests +458, documentation +194 The new standalone collector and persistent store explain the production growth; the outstanding defects are concentrated in initialization.
Previous blockers 2 remain; storage implementation and tests unchanged The latest revision does not address either previously requested database-safety repair.

Merge-risk options

Maintainer options:

  1. Repair database initialization (recommended)
    Preserve the feature while making path validation match the actual destination and recovering safely from failed first-time creation.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Repair the metrics database path mismatch and failed-initialization retry behavior without changing Crawlkit APIs or accepting unrelated empty databases; add focused archive-preservation and cancellation-retry regression coverage.

Technical review

Best possible solution:

Keep metrics isolated through consistent destination validation and failure-safe initialization, with evidence that existing archives remain unchanged and interrupted creation can be retried safely.

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

Yes, for the PR defects: source establishes both the trailing-space destination mismatch and the canceled-initialization retry failure. These paths were not executed during this read-only review.

Is this the best way to solve the issue?

The separate collector is a reasonable fit and does not duplicate existing archive analytics, but its initialization implementation must preserve the actual destination and recover from failed creation.

Full review comments:

  • [P1] Validate the same database path that Crawlkit will open — internal/headlinemetrics/metrics.go:130-138
    This prior blocker remains unchanged. On Unix, configure an absent filename ending in a space while the trimmed filename is an existing archive. Lstat and O_EXCL check/create the spaced filename, but Crawlkit v0.16.3 trims the path before opening it. The following transaction then adds metrics tables and ownership metadata to the unchecked archive. Reject noncanonical paths or normalize consistently before every filesystem check and open.
    Confidence: 0.99
  • [P2] Make failed first-time initialization safely retryable — internal/headlinemetrics/metrics.go:130-143
    This prior blocker also remains unchanged. Initialization reserves the final filename before opening SQLite, but errors leave that file behind. For example, an already-canceled context makes Store.Open fail after reservation; the next healthy attempt sees an existing file and rejects it because metric_meta was never committed. Clean up only the file owned by the failed attempt or publish a fully initialized store safely, while continuing to reject unrelated empty databases.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against b1ea76ebbb40.

Labels

Label justifications:

  • P2: This is an optional, bounded collection feature with concrete storage defects, rather than an urgent regression in a shipped workflow.
  • merge-risk: 🚨 compatibility: The proposed path guard can still initialize metrics tables in an existing archive, and failed creation can make the configured destination unusable on retry.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The MEMBER-authored PR is exempt from ordinary contributor proof requirements; its body reports staged-binary collection, import, and status against an isolated metrics store, but those happy-path claims do not resolve the two source-proven initialization defects.

Evidence

Acceptance criteria:

  • [P1] GOWORK=off go test -race -count=1 ./internal/headlinemetrics ./internal/cli.
  • [P1] GOWORK=off go test -count=1 ./...
  • [P1] GOWORK=off go vet ./...

What I checked:

  • Applicable repository policy: No root or applicable ancestor AGENTS.md exists. The only tracked AGENTS.md belongs to the unrelated autoreview skill subtree; no maintainer-notes directory was found. (7b334b18e407)
  • Both previous findings remain: The focused comparison with the earlier reviewed head is empty. Both metrics.go and metrics_test.go also have identical blob IDs at the two revisions. The previous review's path-normalization and initialization-retry findings therefore remain applicable. (internal/headlinemetrics/metrics.go:92, 7b334b18e407)
  • Unchecked destination and failed-initialization residue: Open checks the untrimmed path with Lstat, and initialize reserves that same filename before handing it to Crawlkit. Initialization errors return without removing the newly reserved file; subsequent attempts require metadata that the failed transaction never committed. (internal/headlinemetrics/metrics.go:120, 7b334b18e407)
  • Affirmative dependency contract: The new storage implementation directly imports github.com/openclaw/crawlkit/store and calls its writable and read-only open functions. go.mod pins Crawlkit v0.16.3, making its path handling directly relevant to the archive-preservation claim. (internal/headlinemetrics/metrics.go:22, 7b334b18e407)
  • Pinned dependency trims paths before opening: At v0.16.3, Store.Open applies strings.TrimSpace to opts.Path before filesystem access and SQLite opening; OpenReadOnly does likewise. The GitHub tag object resolves to b33f7c3a313d8a47dabcb3a5b5f057d2af529794. Its repository policy was read in full; no dependency API modification is needed for the proposed repair. (store/store.go:40, b33f7c3a313d)
  • Current main and release do not contain this capability: Current main has no metrics command dispatch or public-invite count collector. Tree checks found no internal/headlinemetrics directory on main or v0.15.1. Existing analytics documentation describes archived-message activity, not credential-free public membership history. A bounded GitHub PR listing found no replacement for this work. (internal/cli/cli.go:50, b1ea76ebbb40)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Reject or consistently normalize whitespace-bearing database paths and prove existing archives remain byte-for-byte unchanged.
  • Make canceled or failed first initialization retryable, with regression coverage that still rejects unrelated empty databases.

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 (1 earlier review cycle)
  • reviewed 2026-09-15T22:33:38.005Z sha 9576a23 :: blocked before merge. :: [P1] Validate the same database path that Crawlkit will open | [P2] Make failed first-time initialization safely retryable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant