Skip to content

Add PR review observability foundation#684

Merged
hxy91819 merged 1 commit into
mainfrom
agent/pr-674-observability-foundation
Jul 19, 2026
Merged

Add PR review observability foundation#684
hxy91819 merged 1 commit into
mainfrom
agent/pr-674-observability-foundation

Conversation

@hxy91819

@hxy91819 hxy91819 commented Jul 19, 2026

Copy link
Copy Markdown
Member

Summary

  • make the dashboard's top-level health verdict include publication health, open DLQ state, and queue telemetry availability, with degraded states shown as amber and critical/stalled states as red
  • add a durable per-item review telemetry contract and bounded query surface keyed by repository, item number, workflow run, and attempt, including terminal outcome and phase durations
  • add a conservative orphan watchdog for stale refreshing records, with exact health counts and a bounded locator list, without automatically rewriting uncertain attempts to interrupted
  • document green/amber/red thresholds, rollout, and the post-674 verification runbook

Dependency and scope

This PR is a prerequisite for #674 and should merge first.

It was built from current origin/main, not from the PR 674 branch. It does not copy PR 674's generation-bound queue protocol or per-PR shard implementation. Instead, it establishes a backward-compatible observation contract that PR 674 can adopt:

  • post signed telemetry transitions to the internal review telemetry endpoint
  • populate the optional generation and operation_id fields once those identities exist
  • report queue, claim, review, publication, and total phase durations at the corresponding lifecycle transitions
  • use the per-item query and aggregate health surfaces during rollout verification

Terminal outcomes are first-writer immutable. A watchdog observation by itself is not sufficient evidence to write interrupted.

Isolated Worker / Durable Object proof

Ran the exact PR head with wrangler@4.107.0 dev --local on localhost, using a fresh temporary persistence directory and a synthetic webhook secret. No remote bindings, deployment, live queue, or production credentials were used. The repository and run identities below are synthetic.

Redacted terminal transcript:

unsigned POST /internal/exact-review/review-telemetry
=> HTTP 401 {"error":"invalid_signature"}

signed POST terminal attempt 1
=> HTTP 200 {"ok":true}
signed POST stale refreshing attempt 2, no lease
=> HTTP 200 {"ok":true}

GET /api/exact-review-queue/reviews?repo=example/proof&item_number=684&limit=1
=> HTTP 200, review_count=1
=> run_id=isolated-run-2 run_attempt=2 status=refreshing outcome=null
=> generation=8 operation_id=proof:684:2
=> phase_durations_ms={queue:500,claim:750,review:1500}

GET the same endpoint with limit=101
=> HTTP 400 {"error":"invalid_review_telemetry_query"}

GET /api/exact-review-queue
=> review_telemetry_health.status=critical
=> refreshing=1 slow_refreshing=1 orphan_refreshing=1
=> degraded_after_seconds=1800 orphan_after_seconds=9000
=> orphan locator={repo:example/proof,item_number:684,run_id:isolated-run-2,run_attempt:2,lease_expires_at:null}

GET /api/status
=> dashboard_health.conclusion=needs_attention
=> dashboard_health.severity=red
=> dashboard_health.reasons includes orphan_review_status
=> exact_review_queue.review_telemetry_health.status=critical

GET bounded item telemetry after both health reads
=> isolated-run-2 remains status=refreshing outcome=null
=> isolated-run-1 remains status=completed outcome=succeeded

The local /api/status snapshot also reported degraded workflow execution because the isolated Worker intentionally had no GitHub credentials. The review-telemetry signal independently reached the top-level red verdict through orphan_review_status, and reading the watchdog did not mutate the stale attempt to interrupted.

Validation

  • Node v24.15.0
  • pnpm run check
  • /data/code/openclaw/openclaw/.agents/skills/autoreview/scripts/autoreview --mode local --stream-engine-output
    • accepted and fixed unsafe integer ingestion validation
    • final result: no accepted/actionable findings
  • isolated wrangler@4.107.0 dev --local Worker/Durable Object lifecycle proof above

The rollout and post-674 verification steps are in docs/pr-674-observation-runbook.md.

@hxy91819
hxy91819 marked this pull request as ready for review July 19, 2026 01:27
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: passed. Reviewed July 18, 2026, 10:03 PM ET / July 19, 2026, 02:03 UTC.

Summary
The branch adds durable per-review telemetry storage and bounded APIs, aggregates queue/publication telemetry into dashboard health, reports stale attempts conservatively, and documents and tests the rollout contract for the later item-shard work.

Reproducibility: not applicable. as a defect report. The PR body supplies a concrete isolated Worker/Durable Object run demonstrating the after-fix write, read, validation, watchdog, and dashboard paths on the proposed head.

Review metrics: 3 noteworthy metrics.

  • Patch scope: 8 files affected; 905 added, 17 removed. The change is concentrated in the dashboard queue/worker, tests, and one rollout runbook rather than spread through unrelated automation.
  • Validation coverage: 3 focused test files changed or added. The patch covers contract normalization, dashboard aggregation, and durable queue behavior in addition to the reported isolated Worker proof.
  • Route surface: 1 authenticated write route and 1 bounded read route. These endpoints create the operational interface that later producers and dashboard consumers will rely on.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Obtain an explicit maintainer decision on the persisted telemetry and dashboard-health compatibility contract before merging.

Risk before merge

  • [P1] This adds a long-lived persisted telemetry and dashboard-status contract; once producers and operators depend on its keys, thresholds, and terminal semantics, changing them may require compatibility handling.
  • [P1] The top-level dashboard can newly report amber or red from telemetry, publication, and DLQ signals, so maintainers should explicitly accept the intended operational-alerting behavior before merge.

Maintainer options:

  1. Approve the durable contract (recommended)
    Accept the persisted telemetry fields, terminal immutability, bounded reads, retention, and dashboard severity policy as the supported compatibility baseline before merging.
  2. Narrow before landing
    Keep the PR open while maintainers reduce the public or durable contract to the minimum needed for the upcoming rollout.
  3. Defer to one integrated rollout
    Pause this prerequisite if maintainers prefer to decide telemetry semantics together with the item-shard producer implementation.

Next step before merge

  • [P2] A maintainer must choose whether to adopt the durable operational contract; there is no narrow mechanical repair to dispatch.

Maintainer decision needed

  • Question: Should ClawSweeper adopt this persisted review-telemetry schema and its green/amber/red dashboard-health policy as the compatibility foundation for the per-item-shard rollout?
  • Rationale: The patch is mechanically focused and proven, but retention, terminal immutability, query shape, and top-level alert severity become durable operational contracts that code review cannot select unilaterally.
  • Likely owner: hxy91819 — They authored both the foundation and the dependent rollout and can clarify the intended contract boundary for final maintainer approval.
  • Options:
    • Approve the foundation (recommended): Accept the backward-compatible telemetry and dashboard-health contract, then merge it before the dependent item-shard rollout.
    • Narrow the contract first: Keep the PR open while maintainers reduce or revise the persisted fields, public query surface, or health-policy commitments.
    • Defer observability with the rollout: Pause this prerequisite and require the later rollout to propose a single integrated telemetry design instead.

Security
Cleared: The patch adds signed internal telemetry ingestion and local queue/dashboard handling without introducing third-party execution, dependency, permission, secret, or supply-chain changes.

Review details

Best possible solution:

Approve the durable telemetry and dashboard-health contract as the foundation for the item-shard rollout, then merge this focused prerequisite before integrating producers in #674.

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

Not applicable as a defect report. The PR body supplies a concrete isolated Worker/Durable Object run demonstrating the after-fix write, read, validation, watchdog, and dashboard paths on the proposed head.

Is this the best way to solve the issue?

Unclear pending maintainer intent. The implementation is a focused prerequisite for #674, but adopting its persistent schema and operational health semantics is a product and compatibility decision rather than a purely mechanical repair.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (terminal): The PR body provides a redacted terminal transcript from an exact-head isolated local Worker/Durable Object run, showing signature rejection, successful signed writes, bounded reads, watchdog reporting, dashboard escalation, and non-mutating terminal behavior.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 🚀 automerge armed.

Label justifications:

  • P2: This is a substantial but non-emergency automation observability change whose correctness and contract selection need normal maintainer review.
  • merge-risk: 🚨 compatibility: The PR introduces persisted telemetry records, query fields, terminal semantics, retention, and dashboard status meanings that downstream producers and operators may depend on.
  • merge-risk: 🚨 automation: The telemetry routes and health aggregation alter how the review workflow reports queue and publication health to operational dashboards.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (terminal): The PR body provides a redacted terminal transcript from an exact-head isolated local Worker/Durable Object run, showing signature rejection, successful signed writes, bounded reads, watchdog reporting, dashboard escalation, and non-mutating terminal behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a redacted terminal transcript from an exact-head isolated local Worker/Durable Object run, showing signature rejection, successful signed writes, bounded reads, watchdog reporting, dashboard escalation, and non-mutating terminal behavior.
Evidence reviewed

What I checked:

  • Durable telemetry contract: The new telemetry module defines the persisted attempt identity, terminal outcomes, phase-duration fields, health thresholds, and optional generation/operation identifiers used by the queue and dashboard. (dashboard/review-telemetry.ts:1, 0040765ef7ad)
  • Bounded authenticated surface: The worker adds a signed internal telemetry-write route and a separately bounded read route, while the queue implementation adds the matching telemetry endpoints and storage table. (dashboard/worker.ts:527, 0040765ef7ad)
  • Compatibility-sensitive dashboard behavior: The new dashboard aggregator raises amber/red status from unavailable queue telemetry, handoff state, publication state, DLQ state, and review-telemetry health; this changes the operational meaning of the top-level health verdict. (dashboard/dashboard-health.ts:8, 0040765ef7ad)
  • Focused regression coverage: The patch adds dashboard-health and telemetry contract tests and extends queue tests for persistence, bounded reads, terminal immutability, and watchdog behavior. (test/dashboard-worker.test.ts:11205, 0040765ef7ad)
  • Related rollout dependency: The PR body and linked context consistently describe Review each selected PR in its own item shard #674 as the later item-shard rollout that will consume this optional telemetry contract; it remains open and draft, so this PR is an adjacent prerequisite rather than a superseded duplicate. (docs/pr-674-observation-runbook.md:1, 0040765ef7ad)
  • Repository policy fit: The supplied complete AGENTS.md requires conservative review of automation and dashboard changes, calls for pnpm run check, treats long-lived generated state carefully, and preserves release-owned changelog handling; the PR includes check and focused proof without changing CHANGELOG.md. (AGENTS.md:1, c5314ce8de1e)

Likely related people:

  • hxy91819: Authored this observability foundation and the linked per-item-shard rollout it is designed to support, providing concrete ownership context for the new telemetry boundary beyond this PR alone. (role: recent related contributor; confidence: medium; commits: 0040765ef7ad, f57a43b93d42; files: dashboard/review-telemetry.ts, dashboard/exact-review-queue.ts, dashboard/worker.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (3 earlier review cycles)
  • reviewed 2026-07-19T01:26:11.082Z sha 0040765 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-19T01:30:30.410Z sha 0040765 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-19T01:48:23.595Z sha 0040765 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 19, 2026
@hxy91819

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 19, 2026
@hxy91819

Copy link
Copy Markdown
Member Author

@clawsweeper automerge

Special instructions:
Adopt this backward-compatible durable review-telemetry and dashboard-health contract as the prerequisite foundation for #674. Preserve terminal immutability, bounded reads, conservative orphan handling, and optional generation/operation_id compatibility.

@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper automerge is enabled.

  • Head: 0040765ef7ad
  • Label: clawsweeper:automerge
  • Action: exact-head review queued (workflow sweep.yml, event repository_dispatch).
  • Flow: review this head, repair/rebase only if needed, then re-review the exact repaired head before merge.

Draft PRs stay fix-only until GitHub marks them ready for review. Pause with /clawsweeper stop.

Automerge progress:

  • 2026-07-19 02:00:48 UTC review queued 0040765ef7ad (queued)

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:automerge Maintainer opted this ClawSweeper PR into bounded ClawSweeper-reviewed automerge status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 19, 2026
@hxy91819
hxy91819 merged commit 0bb5964 into main Jul 19, 2026
23 checks passed
@hxy91819
hxy91819 deleted the agent/pr-674-observability-foundation branch July 19, 2026 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:automerge Maintainer opted this ClawSweeper PR into bounded ClawSweeper-reviewed automerge merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. 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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant