Skip to content

feat: collect repository metrics in an independent store - #206

Merged
steipete merged 11 commits into
mainfrom
feat/metrics-collection
Sep 28, 2026
Merged

steipete merged 11 commits into
mainfrom
feat/metrics-collection

Conversation

@hannesrudolph

@hannesrudolph hannesrudolph commented Sep 15, 2026 •

Copy link
Copy Markdown
Member

What Problem This Solves

Repository headline history currently requires a separate collector instead of a discoverable Gitcrawl command.

User Impact

Adds gitcrawl metrics collect|import|status --config metrics.json for stars, forks, actual subscribers, open PRs/issues, optional completed-day clones, and stable releases. Operators can retain OpenClaw repository metrics in a separate private SQLite metrics database, using native GitHub authentication and JSON output.

Why This Change Was Made

The metrics store preserves unknown values, zeroes, decreases, original import IDs, and daily corrections. Imports are scoped and atomic; archive/wrong-owner databases are rejected before a writable open. The commands never invoke archive refresh, embeddings, models, or schedules. Help, control metadata, and source documentation expose the full workflow.

Evidence

  • Collector/store and native CLI fixtures pass, including actual watcher semantics, missing/incomplete PR counts, optional clone 403/404, release pagination beyond five pages, cancellation, rollback of late invalid imports, daily revisions, and archive ownership/path protection.
  • Full Go suite and focused race checks pass; coverage exceeds the repository's 85% gate.
  • Vet, vulnerability/dead-code scans, module tidiness, formatting, docs build/tests, and release-script tests pass. One existing formatting-only indentation issue was corrected to satisfy the formatting gate.
  • A locally built CLI passed fixture-only collect/import/status acceptance: two repositories, idempotent releases/imports, daily deduplication, and preserved NULLs. No live archives, API credentials, jobs, or services were changed.
  • Local source builds require no signing credentials. Cross-platform GoReleaser snapshot builds were not run locally (GoReleaser is unavailable); the existing CI matrix covers them. Official signing/notarization remains the release workflow's responsibility; no release was published.

@clawsweeper

clawsweeper Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

🦞👀
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. 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

Codex review: needs real behavior proof before merge. Reviewed September 28, 2026, 7:14 AM ET / 11:14 UTC (Revision 9).

ClawSweeper review

What this changes

This PR adds commands to collect GitHub repository counters and releases, import historical observations, and inspect a separate SQLite metrics database.

Merge readiness

⛔ Blocked before merge - 6 items remain

The metrics workflow is absent from current main and v0.12.0. The latest branch addresses both earlier review findings, but its foreign-database safeguard can still change a substituted SQLite file before rejecting it.

Priority: P2
Reviewed head: 77f02d742e686d0667854ded9394bba518bafcd2
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The branch has substantial focused validation and resolves prior findings, but the foreign-database final effect remains unproven and the source shows a connection-ordering defect.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Authority-chain proof required: The MEMBER-authored PR's swap tests use the real SQLite path and show rejection without metrics tables, but do not show that the nearest forbidden target—a substituted archive—keeps its journal mode, permissions, and bytes before final I/O. The new separate schema and existing-store reuse tests support data-model compatibility without a legacy metrics migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: Authority-chain proof required: The MEMBER-authored PR's swap tests use the real SQLite path and show rejection without metrics tables, but do not show that the nearest forbidden target—a substituted archive—keeps its journal mode, permissions, and bytes before final I/O. The new separate schema and existing-store reuse tests support data-model compatibility without a legacy metrics migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 10 items PR introduction: The pinned main-to-head diff adds the metrics command, collector, store, tests, and documentation.
Shared-store dependency signal: The new metrics store directly imports CrawlKit's store package and calls its writable opener; that dependency's connection setup determines whether ownership rejection is side-effect-free.
Validation follows writable open: After a read-only pathname check, Open calls CrawlKit's writable store.Open at line 254; initialize verifies ownership and file identity only afterward.
Findings 1 actionable finding [P2] Validate database ownership before writable SQLite setup
Security Needs attention Foreign database can change before rejection: The injected pathname-swap case reaches a WAL-configured, permission-tightening opener before ownership validation; the tests check schema preservation but not those earlier filesystem effects.

How this fits together

Gitcrawl normally stores GitHub issues and pull requests for local maintainer search. The new metrics commands take a separate config, GitHub responses or imported history, then write an independent database and report its status.

flowchart LR
  A[Metrics config] --> B[Metrics commands]
  C[GitHub repository API] --> D[Counter and release collector]
  D --> B
  E[Imported history] --> B
  B --> F[Separate SQLite store]
  F --> G[Status output]
Loading

Decision needed

Question Recommendation
Must the metrics command leave a substituted foreign SQLite database unchanged even during connection setup? Preserve the boundary: Use an opener that validates the exact opened database before writable setup and cover the foreign-file final effects.

Why: The documented archive isolation guarantee and CrawlKit's current writable-open behavior meet at a shared-library boundary; an owner must approve the connection strategy.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: Authority-chain proof required: The MEMBER-authored PR's swap tests use the real SQLite path and show rejection without metrics tables, but do not show that the nearest forbidden target—a substituted archive—keeps its journal mode, permissions, and bytes before final I/O. The new separate schema and existing-store reuse tests support data-model compatibility without a legacy metrics migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Validate database ownership before writable SQLite setup (P2) - A path replaced after the read-only check reaches CrawlKit's writable store.Open before initialize rejects it. That opener requests WAL mode and chmods the path to 0600, so a foreign archive can change even when no metrics table is added. Check the exact opened file before those effects and assert its journal mode and permissions remain unchanged in the swap tests.
  • Resolve security concern: Foreign database can change before rejection - The injected pathname-swap case reaches a WAL-configured, permission-tightening opener before ownership validation; the tests check schema preservation but not those earlier filesystem effects.
  • Resolve merge risk (P1) - A foreign SQLite file substituted after the read-only check can receive CrawlKit's WAL-mode or permission changes before metrics ownership validation rejects it.
  • Complete next step (P2) - Prevent writable-open effects on a substituted foreign database and add final-effect regression proof before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P2] Validate database ownership before writable SQLite setup — internal/headlinemetrics/metrics.go:253-258
  • [medium] Foreign database can change before rejection — internal/headlinemetrics/metrics.go:254
Agent review details

Security

Needs attention: The database ownership check occurs after shared-library writable connection setup can affect a substituted foreign file.

Review metrics

Metric Value Why it matters
Code growth production +1052/-2, tests +1332, docs +320 The separate collector and store explain substantial production growth, with focused test coverage alongside it.

Merge-risk options

Maintainer options:

  1. Validate before writable setup (recommended)
    Change the connection path so foreign-file rejection precedes WAL and permission changes, and assert those final effects in the swap tests.
  2. Pause for shared-store design
    Hold this PR if the required connection ordering needs a new CrawlKit API or cross-platform ownership design.

Technical review

Best possible solution:

Verify ownership on the exact database connection before any journal-mode, permission, or schema change, then prove a substituted archive retains its bytes, mode, and journal setting.

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

Yes, by injecting a foreign SQLite file at the existing pre-open swap hook and comparing its journal mode and permissions before and after rejection. Source shows why the current assertions miss those effects; I did not execute that check.

Is this the best way to solve the issue?

Not yet. The separate store is a coherent boundary, but its writable opener must honor that boundary during connection setup.

Full review comments:

  • [P2] Validate database ownership before writable SQLite setup — internal/headlinemetrics/metrics.go:253-258
    A path replaced after the read-only check reaches CrawlKit's writable store.Open before initialize rejects it. That opener requests WAL mode and chmods the path to 0600, so a foreign archive can change even when no metrics table is added. Check the exact opened file before those effects and assert its journal mode and permissions remain unchanged in the swap tests.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 290d943b1c0c.

Labels

Label changes:

  • add merge-risk: 🚨 security-boundary: A replaced database path can reach writable connection setup before the intended ownership boundary rejects it.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Authority-chain proof required: The MEMBER-authored PR's swap tests use the real SQLite path and show rejection without metrics tables, but do not show that the nearest forbidden target—a substituted archive—keeps its journal mode, permissions, and bytes before final I/O. The new separate schema and existing-store reuse tests support data-model compatibility without a legacy metrics migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove status: ⏳ waiting on author: Current PR status label is status: 📣 needs proof.
  • remove merge-risk: 🚨 availability: Current PR review merge-risk labels are merge-risk: 🚨 security-boundary.

Label justifications:

  • P2: This is a bounded operator-facing feature with one merge-blocking database isolation concern.
  • merge-risk: 🚨 security-boundary: A replaced database path can reach writable connection setup before the intended ownership boundary rejects it.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Authority-chain proof required: The MEMBER-authored PR's swap tests use the real SQLite path and show rejection without metrics tables, but do not show that the nearest forbidden target—a substituted archive—keeps its journal mode, permissions, and bytes before final I/O. The new separate schema and existing-store reuse tests support data-model compatibility without a legacy metrics migration. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

  • [medium] Foreign database can change before rejection — internal/headlinemetrics/metrics.go:254
    The injected pathname-swap case reaches a WAL-configured, permission-tightening opener before ownership validation; the tests check schema preservation but not those earlier filesystem effects.
    Confidence: 0.89

What I checked:

  • PR introduction: The pinned main-to-head diff adds the metrics command, collector, store, tests, and documentation. (internal/headlinemetrics/metrics.go:202, 77f02d742e68)
  • Shared-store dependency signal: The new metrics store directly imports CrawlKit's store package and calls its writable opener; that dependency's connection setup determines whether ownership rejection is side-effect-free. (internal/headlinemetrics/metrics.go:21, 77f02d742e68)
  • Validation follows writable open: After a read-only pathname check, Open calls CrawlKit's writable store.Open at line 254; initialize verifies ownership and file identity only afterward. (internal/headlinemetrics/metrics.go:254, 77f02d742e68)
  • Writable opener side effects: At the verified v0.16.5 dependency commit, store.Open pings a connection configured with journal_mode(WAL), then chmods the pathname to 0600 before returning. The repository identity was checked with the GitHub repository API. (store/store.go:39, 493f7470c5e3)
  • Swap tests leave final effects unchecked: The injected file and parent swaps assert that a replacement archive gets no metrics tables and retains its row; they do not compare journal mode, file permissions, or bytes after rejection. (internal/headlinemetrics/metrics_test.go:466, 77f02d742e68)
  • Earlier timestamp finding addressed: Status now parses stored observation timestamps before choosing the latest instant, with coverage for offsets and fractional precision. (internal/headlinemetrics/metrics_test.go:352, 77f02d742e68)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Hannes Rudolph: Raw commit bd91431 adds internal/github/history.go:3 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: bd9143104d03; files: internal/github/history.go)
  • Vincent Koc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

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

  • Prevent WAL and permission changes before validating the exact opened database.
  • Add final-effect proof for an allowed metrics file and the nearest forbidden substituted archive, including its unchanged journal mode, permissions, and bytes.

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 (8 earlier review cycles)
  • reviewed 2026-09-15T21:29:36.999Z sha 22b4cf5 :: needs changes before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization
  • reviewed 2026-09-15T21:52:39.595Z sha abc138c :: needs changes before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization
  • reviewed 2026-09-15T22:37:36.873Z sha a7befd3 :: needs changes before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization
  • reviewed 2026-09-15T22:44:20.260Z sha ef72049 :: needs changes before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization
  • reviewed 2026-09-15T22:59:17.567Z sha 6b27f8d :: needs changes before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization
  • reviewed 2026-09-16T22:54:04.192Z sha 6b27f8d :: needs changes before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization
  • reviewed 2026-09-17T17:34:27.973Z sha 6b27f8d :: needs changes before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization
  • reviewed 2026-09-25T01:58:56.520Z sha 6b27f8d :: blocked before merge. :: [P2] Compare observation timestamps chronologically | [P2] Recover safely from failed first-time database initialization

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Sep 25, 2026
Retain both isolated metrics installation guidance and SIMD source-build guidance in the sole conflict, docs/installation.md.
Restore gofmt indentation in the oversized-row error path. The previous Ubuntu CI run stopped here before executing tests; no runtime behavior changes.
Recover returned first-initialization failures without removing pre-existing files, reject database hardlink aliases, compare imported timestamps and UTC days chronologically, and enforce the config size limit. Preserve original import IDs, timestamp spelling, NULLs, zeroes, and corrections.
Stop on exhausted quota while keeping completed reads, including cancellation after a response. Reject missing provider lists and emit structured partial results and explicit zero status totals. Document archive ownership inspection, permissions, recovery limits, and collection request costs.
Keep read-only prechecks, then pin the writable connection and validate ownership under BEGIN IMMEDIATE before applying schema and metadata atomically. Require new databases to remain empty, reject replaced file identities, and retain guarded cleanup. Cover file and parent swaps, in-place changes, and rollback when metadata insertion fails.
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Sep 28, 2026
@steipete
steipete merged commit b20b4f3 into main Sep 28, 2026
17 checks passed
@steipete
steipete deleted the feat/metrics-collection branch September 28, 2026 11:16
@steipete

Copy link
Copy Markdown
Contributor

Thanks @hannesrudolph! I merged main into the branch and added maintainer fixes before landing:

  • Isolation: ownership (or emptiness, for a newly created file) is now validated through the single pinned writable connection under BEGIN IMMEDIATE, followed by a file-identity check, before any schema or owner metadata is written. This closes a pathname race between the read-only pre-check and the writable open. Hardlink aliases are rejected, and a failed first initialization removes only the file this call created.
  • Data semantics: daily observations are deduplicated per UTC day regardless of timestamp spelling, "latest" is chosen chronologically rather than lexically, and oversized configs are rejected instead of truncated.
  • Collection: quota exhaustion stops further requests and reports partial results (including zero-row partial runs in JSON), and missing or null provider lists are no longer treated as a complete empty history.
  • Fixed the cloud_ingest.go formatting regression that failed CI.

The full suite and race tests passed on Linux, the Windows cross-compile passed, and autoreview is clean.

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

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants