Skip to content

cron: reject webhook URLs with embedded credentials#51822

Open
RogueL90 wants to merge 2 commits into
openclaw:mainfrom
RogueL90:cron-webhook-reject-credentials
Open

cron: reject webhook URLs with embedded credentials#51822
RogueL90 wants to merge 2 commits into
openclaw:mainfrom
RogueL90:cron-webhook-reject-credentials

Conversation

@RogueL90

Copy link
Copy Markdown

Summary

Describe the problem and fix in 2–5 bullets:

  • Problem: Cron webhook URLs could include embedded credentials (e.g. https://user:[email protected]/hook), which can leak secrets in logs, config, or URLs.
  • Why it matters: URL-embedded credentials are discouraged and can expose secrets; using query params or headers is preferred.
  • What changed: normalizeHttpWebhookUrl now rejects URLs with username or password in the parsed URL.
  • What did NOT change (scope boundary): No changes to auth headers, query params, or other credential handling; only http(s)://user:pass@host URLs are rejected.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Closes #
  • Related #

User-visible / Behavior Changes

Cron webhook URLs with embedded credentials (e.g. https://user:secret@host/path) are now rejected as invalid. URLs without credentials continue to work.

Security Impact (required)

  • New permissions/capabilities? (No)
  • Secrets/tokens handling changed? (No)
  • New/changed network calls? (No)
  • Command/tool execution surface changed? (No)
  • Data access scope changed? (No)
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS: macOS (darwin)
  • Runtime/container: Node 22+
  • Model/provider: N/A
  • Integration/channel (if any): CRON webhook delivery
  • Relevant config (redacted): N/A

Steps

  1. Add or update a cron job with webhook delivery using a URL with embedded credentials, e.g. https://user:[email protected]/webhook.
  2. Observe that the webhook URL is rejected (job will not use it).
  3. Use the same job with https://example.invalid/webhook (no credentials) and confirm it is accepted.

Expected

  • URLs with user:pass@ in the authority are rejected.
  • URLs without embedded credentials continue to work.

Actual

  • Behavior matches above

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)
Screenshot 2026-03-21 at 11 55 36 AM

Human Verification (required)

What you personally verified (not just CI), and how:

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

  • Backward compatible? (No)
  • Config/env changes? (No)
  • Migration needed? (Yes)
  • If yes, exact upgrade steps:
    Update any cron job webhook URLs that include user:pass@ in the URL. Remove embedded credentials and use headers or query params for auth instead.

Failure Recovery (if this breaks)

  • How to disable/revert this change quickly: Revert this commit.
  • Files/config to restore: src/cron/webhook-url.ts
  • Known bad symptoms reviewers should watch for: Jobs with credential URLs will fail validation or treat the webhook as invalid.

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

  • Risk: Existing jobs using credential URLs will break.
    • Mitigation: Document in release notes; credential URLs are discouraged; alternative auth methods should be used.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1329a47f5a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/cron/webhook-url.ts
Comment on lines +18 to +20
// Do not allow embedded credentials in webhook URLs.
if (parsed.username || parsed.password) {
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Prevent raw logging of rejected credential URLs

When a stored cron job already has delivery.mode = "webhook" with a user:pass@ URL, this new null path makes buildGatewayCronService fall into the warning at src/gateway/server-cron.ts:381-388, which logs deliveryTo: job.delivery.to without redaction. In that scenario the gateway will now write the full embedded credentials to logs on every run, so the hardening change introduces the secret leak it is trying to prevent.

Useful? React with 👍 / 👎.

@greptile-apps

greptile-apps Bot commented Mar 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a four-line security hardening check to normalizeHttpWebhookUrl that rejects webhook URLs containing embedded credentials (username or password fields populated by the WHATWG URL parser). The change is minimal, correctly targeted, and fits naturally into the existing validation flow alongside the protocol check above it. It closes a real credential-leakage vector and the assertDeliverySupport / assertFailureDestinationSupport call-sites in service/jobs.ts already re-use this function, so the protection is applied at all entry points.

Key observations from the review:

  • Logic is correct for the common cases: URLs with a username, a password, or both are all rejected via the falsy-empty-string behaviour of the WHATWG URL API.
  • Degenerate empty-credential case (http://:@host/path) passes through because both parsed.username and parsed.password are "", which are falsy. This is a very minor gap — empty credentials carry no real secret — but the @ symbol could confuse downstream parsers.
  • No automated tests exist for webhook-url.ts. The rest of src/cron/ follows a per-concern test-file convention and this security-sensitive validation has none, making it easy to regress silently.

Confidence Score: 4/5

  • Safe to merge; the change is minimal, correct, and directly addresses a real credential-leakage risk with no unintended side-effects on compliant URLs.
  • The core logic is sound and the change is small. The two remaining gaps (no webhook-url.test.ts and the degenerate empty-credential edge case) are non-blocking P2s that can be addressed as follow-ups without blocking the security improvement.
  • No files require special attention beyond the noted test-coverage gap in src/cron/webhook-url.ts.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21

Comment:
**Degenerate `@`-separator URL passes through**

A URL like `http://:@example.com/path` has both `parsed.username` and `parsed.password` as `""` (empty strings), so `"" || ""` evaluates as falsy and the URL is accepted. Technically these are empty credentials and not a real secret-leakage risk, but the `@` in the URL could still confuse downstream parsers or loggers into treating it as a credential-bearing URL.

If the intent is to reject any URL with the `user:pass@host` authority format (even empty), the check could be expanded:

```suggestion
    if (parsed.username || parsed.password || parsed.href.includes("@")) {
      return null;
    }
```

Alternatively, leave as-is with a comment noting the empty-credential edge case is intentionally allowed.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21

Comment:
**Missing test coverage for security validation**

There is no test file for `normalizeHttpWebhookUrl` — no `webhook-url.test.ts` exists anywhere in the codebase. Since this change rejects inputs that were previously accepted, automated tests would lock in the expected behavior and guard against regressions. The rest of the `src/cron/` directory uses dedicated per-concern test files, so adding one here would be consistent.

Suggested cases to cover at minimum:
- URL with embedded username and password → `null`
- URL with only a username in the authority → `null`
- URL with only a password in the authority → `null`
- Plain `http://` URL without any credentials → accepted
- Plain `https://` URL without any credentials → accepted
- Non-http/https protocol → `null` (existing protocol check)

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "cron: reject webhook..."

Comment thread src/cron/webhook-url.ts
Comment on lines +19 to +21
if (parsed.username || parsed.password) {
return null;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Degenerate @-separator URL passes through

A URL like http://:@example.com/path has both parsed.username and parsed.password as "" (empty strings), so "" || "" evaluates as falsy and the URL is accepted. Technically these are empty credentials and not a real secret-leakage risk, but the @ in the URL could still confuse downstream parsers or loggers into treating it as a credential-bearing URL.

If the intent is to reject any URL with the user:pass@host authority format (even empty), the check could be expanded:

Suggested change
if (parsed.username || parsed.password) {
return null;
}
if (parsed.username || parsed.password || parsed.href.includes("@")) {
return null;
}

Alternatively, leave as-is with a comment noting the empty-credential edge case is intentionally allowed.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21

Comment:
**Degenerate `@`-separator URL passes through**

A URL like `http://:@example.com/path` has both `parsed.username` and `parsed.password` as `""` (empty strings), so `"" || ""` evaluates as falsy and the URL is accepted. Technically these are empty credentials and not a real secret-leakage risk, but the `@` in the URL could still confuse downstream parsers or loggers into treating it as a credential-bearing URL.

If the intent is to reject any URL with the `user:pass@host` authority format (even empty), the check could be expanded:

```suggestion
    if (parsed.username || parsed.password || parsed.href.includes("@")) {
      return null;
    }
```

Alternatively, leave as-is with a comment noting the empty-credential edge case is intentionally allowed.

How can I resolve this? If you propose a fix, please make it concise.

Comment thread src/cron/webhook-url.ts
Comment on lines +19 to +21
if (parsed.username || parsed.password) {
return null;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing test coverage for security validation

There is no test file for normalizeHttpWebhookUrl — no webhook-url.test.ts exists anywhere in the codebase. Since this change rejects inputs that were previously accepted, automated tests would lock in the expected behavior and guard against regressions. The rest of the src/cron/ directory uses dedicated per-concern test files, so adding one here would be consistent.

Suggested cases to cover at minimum:

  • URL with embedded username and password → null
  • URL with only a username in the authority → null
  • URL with only a password in the authority → null
  • Plain http:// URL without any credentials → accepted
  • Plain https:// URL without any credentials → accepted
  • Non-http/https protocol → null (existing protocol check)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/webhook-url.ts
Line: 19-21

Comment:
**Missing test coverage for security validation**

There is no test file for `normalizeHttpWebhookUrl` — no `webhook-url.test.ts` exists anywhere in the codebase. Since this change rejects inputs that were previously accepted, automated tests would lock in the expected behavior and guard against regressions. The rest of the `src/cron/` directory uses dedicated per-concern test files, so adding one here would be consistent.

Suggested cases to cover at minimum:
- URL with embedded username and password → `null`
- URL with only a username in the authority → `null`
- URL with only a password in the authority → `null`
- Plain `http://` URL without any credentials → accepted
- Plain `https://` URL without any credentials → accepted
- Non-http/https protocol → `null` (existing protocol check)

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98e38956ec

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/cron/webhook-url.ts
Comment on lines +19 to +20
if (parsed.username || parsed.password) {
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Surface invalid legacy cron.webhook targets

For installations that still rely on legacy notify: true plus a global cron.webhook, rejecting credentialed URLs here turns previously working webhook deliveries into silent no-ops. resolveCronWebhookTarget now feeds cron.webhook through normalizeHttpWebhookUrl (src/gateway/server-cron.ts:74-77), but the only invalid-target warning is behind job.delivery.mode === "webhook" (src/gateway/server-cron.ts:381-388), so legacy jobs never emit any warning when their credentialed URL starts returning null. After upgrade those jobs just stop posting finished-run callbacks with no migration hint.

Useful? React with 👍 / 👎.

Comment thread src/cron/webhook-url.ts
Comment on lines +19 to +20
if (parsed.username || parsed.password) {
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Prevent doctor from rewriting notify jobs to invalid webhooks

For users following the documented repair flow, this change makes openclaw doctor --fix destructive when cron.webhook contains embedded credentials. The repair code still migrates notify: true jobs by copying cron.webhook verbatim into delivery.to and deleting notify (src/commands/doctor-cron.ts:95-102), but after this helper starts rejecting credentialed URLs the migrated job becomes an invalid per-job webhook that the scheduler skips. In that scenario the recommended migration path converts a working legacy job into one that never delivers.

Useful? React with 👍 / 👎.

@clawsweeper

clawsweeper Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 2:17 AM ET / 06:17 UTC.

Summary
The branch adds a userinfo check to cron webhook URL normalization so HTTP(S) webhook URLs with parsed username or password are rejected.

PR surface: Source +4. Total +4 across 1 file.

Reproducibility: yes. from source inspection: current main's cron helper delegates to isHttpUrl, and the net-policy tests classify user:pass HTTP(S) URLs as valid. I did not run a live cron add/update or dispatch flow in this read-only review.

Review metrics: 1 noteworthy metric.

  • Shared validator tightened: 1 helper changed. The helper is reused by cron add/update validation, runtime dispatch, failure alerts, and doctor migration, so maintainers need to review upgrade impact before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • [P1] Add upgrade-safe handling or an explicit maintainer-approved fail-closed policy with focused cron validation, runtime dispatch, and doctor migration tests.
  • Refresh the branch against current main so the helper refactor conflict is resolved.
  • Update the PR body with redacted real cron-path proof, such as terminal output, logs, or a recording that avoids private URLs, tokens, IPs, and non-public endpoints.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The inspected screenshot shows direct helper output only; before merge, the contributor should add redacted terminal output, logs, or a recording proving the real cron add/update, doctor migration, dispatch, or delivery path, then update the PR body for re-review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Existing stored cron webhook, completion, or failure-destination URLs with embedded credentials would become invalid after upgrade and stop posting callbacks unless maintainers approve the fail-closed behavior or add doctor/audit migration handling.
  • [P1] The branch is dirty against current main's cron webhook helper refactor, so the submitted patch cannot merge without being refreshed onto the current helper shape.
  • [P1] The attached proof only shows direct helper output and does not prove cron add/update validation, doctor migration, dispatch, or delivery behavior after the change.

Maintainer options:

  1. Add upgrade-safe cron hardening (recommended)
    Require migration/audit handling, redacted warnings, and regression tests for stored credentialed webhook URLs before merge.
  2. Accept the breaking security boundary
    Maintainers can approve fail-closed rejection if they are comfortable with existing credentialed webhook jobs stopping until operators edit config.
  3. Pause for a current-main replacement
    If the conflicted branch is not worth reviving, replace it with a narrow PR that applies the same security policy with current-main migration coverage.

Next step before merge

  • [P1] Manual review is needed because the unresolved blocker is the maintainer security/compatibility decision for stored cron webhook URLs, plus proof and branch refresh, not a narrow safe bot repair.

Maintainer decision needed

  • Question: Should cron webhook URL userinfo be rejected fail-closed for existing stored jobs, or must this PR add doctor/audit migration behavior before merge?
  • Rationale: Rejecting URL userinfo reduces secret exposure, but the shared helper also controls stored cron delivery and migration paths, so automation should not choose the security-versus-upgrade tradeoff without maintainer intent.
  • Likely owner: steipete — steipete has the strongest recent history across the cron runtime notification and doctor migration surfaces that need the policy decision.
  • Options:
    • Require upgrade path before merge (recommended): Add doctor/audit handling, redacted warnings, focused tests, and real cron-path proof before accepting the stricter validator.
    • Accept fail-closed hardening: Maintainers may intentionally accept that credentialed webhook URLs stop delivering until operators edit them, but the PR should document and prove that path.
    • Replace with redaction-first hardening: Use a fresh current-main PR that first redacts credentialed URLs while preserving delivery compatibility until a migration policy is decided.

Security
Needs attention: The PR improves credential exposure posture but changes a stored credential-bearing cron URL path without upgrade-safe migration or explicit fail-closed approval.

Review findings

  • [P1] Add upgrade-safe handling before rejecting stored webhook credentials — src/cron/webhook-url.ts:19-21
Review details

Best possible solution:

Implement credentialed cron webhook hardening on current main with an explicit maintainer-approved fail-closed policy or doctor/audit migration, redacted warnings, focused regression coverage, and real cron-path proof.

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

Yes, from source inspection: current main's cron helper delegates to isHttpUrl, and the net-policy tests classify user:pass HTTP(S) URLs as valid. I did not run a live cron add/update or dispatch flow in this read-only review.

Is this the best way to solve the issue?

No; rejecting userinfo in the shared helper is a plausible hardening point, but it is not the best merge shape until stored cron URLs have an upgrade-safe policy, migration/audit behavior, and real cron-path proof.

Full review comments:

  • [P1] Add upgrade-safe handling before rejecting stored webhook credentials — src/cron/webhook-url.ts:19-21
    This helper is shared by cron job validation and runtime delivery. Returning null for https://user:pass@host makes existing stored webhook, completion, or failure-destination URLs stop posting callbacks after upgrade; current main redacts invalid-target logs, but the PR still lacks a doctor/audit migration, maintainer-approved fail-closed policy, and regression coverage for stored jobs.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is useful security hardening with limited surface area, but it needs compatibility policy and proof before merge.
  • merge-risk: 🚨 compatibility: The PR changes validation for stored cron webhook URLs and can make existing credentialed URLs invalid after upgrade.
  • merge-risk: 🚨 message-delivery: Existing cron jobs using credentialed webhook URLs could stop delivering completion or failure callbacks until operators update them.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The inspected screenshot shows direct helper output only; before merge, the contributor should add redacted terminal output, logs, or a recording proving the real cron add/update, doctor migration, dispatch, or delivery path, then update the PR body for re-review or ask a maintainer to comment @clawsweeper re-review.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The inspected screenshot shows direct helper output only; before merge, the contributor should add redacted terminal output, logs, or a recording proving the real cron add/update, doctor migration, dispatch, or delivery path, then update the PR body for re-review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +4. Total +4 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 4 0 +4
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 4 0 +4

Security concerns:

  • [medium] Credentialed webhook rejection lacks upgrade path — src/cron/webhook-url.ts:19
    Stored cron webhook URLs containing userinfo would remain in config but stop delivering after the shared helper rejects them, so this needs doctor/audit handling or maintainer-approved fail-closed security policy before merge.
    Confidence: 0.86

What I checked:

  • PR diff tightens the shared cron webhook helper: The PR adds a parsed username/password rejection inside normalizeHttpWebhookUrl at PR head 98e3895. (src/cron/webhook-url.ts:19, 98e38956ec34)
  • Current main does not implement this rejection: Current main's normalizeHttpWebhookUrl only trims input and delegates to isHttpUrl before returning the URL, with no userinfo check. (src/cron/webhook-url.ts:12, a780541ea6c5)
  • Current tests show the underlying HTTP predicate accepts userinfo URLs: The net-policy URL protocol tests classify HTTPS URLs with user:pass userinfo as valid HTTP(S) URLs, so current main still accepts the central input shape this PR targets. (packages/net-policy/src/url-protocol.test.ts:21, a780541ea6c5)
  • Fresh and stored cron validation share the helper: Cron job validation normalizes delivery.to, completionDestination.to, and failureDestination.to with normalizeHttpWebhookUrl, so the PR affects add/update validation and stored job revalidation. (src/cron/service/jobs.ts:369, a780541ea6c5)
  • Runtime delivery shares the helper and now redacts invalid-target logs: Runtime cron completion, failure alert, and failure destination webhook paths all call normalizeHttpWebhookUrl; current invalid-target warnings redact the URL but still skip delivery when the helper returns null. (src/gateway/server-cron-notifications.ts:121, a780541ea6c5)
  • Doctor migration depends on the same URL normalization: Legacy notify migration normalizes cron.webhook and existing webhook destinations through normalizeHttpWebhookUrl before rewriting stored jobs, so credential rejection needs explicit migration or fail-closed policy coverage. (src/commands/doctor/cron/legacy-notify.ts:21, a780541ea6c5)

Likely related people:

  • steipete: Recent commit history ties steipete to cron runtime notification, doctor migration, and cron service refactors that would own the compatibility policy around stored webhook delivery. (role: recent area contributor; confidence: high; commits: 2bd07eead7e7, d1bf769dbd5c, a34b8a8d6968; files: src/gateway/server-cron-notifications.ts, src/commands/doctor/cron/legacy-notify.ts, src/cron/service/jobs.ts)
  • RomneyDa: RomneyDa authored the current helper refactor that consolidated cron webhook URL protocol predicates into normalizeHttpWebhookUrl's current main shape. (role: recent helper refactor contributor; confidence: medium; commits: 553b41687dc5; files: src/cron/webhook-url.ts)
  • advaitpaliwal: The original cron webhook delivery separation and validation work appears in commit bc67af6, which introduced the validation surface this PR tightens. (role: introduced webhook delivery validation; confidence: medium; commits: bc67af6ad89b; files: src/cron/webhook-url.ts, src/cron/service/jobs.ts)
  • Alix-007: Alix-007 authored the recent doctor change for legacy cron notify marker cleanup, adjacent to the migration behavior that needs coverage if credentialed webhook URLs become invalid. (role: doctor migration contributor; confidence: medium; commits: 4d6befe7cd92; files: src/commands/doctor/cron/legacy-notify.ts, src/commands/doctor/cron/index.test.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 (4 earlier review cycles)
  • reviewed 2026-07-03T04:23:27.890Z sha 98e3895 :: needs real behavior proof before merge. :: [P1] Add upgrade-safe handling before rejecting stored webhook credentials
  • reviewed 2026-07-05T05:03:02.139Z sha 98e3895 :: needs real behavior proof before merge. :: [P1] Add upgrade-safe handling before rejecting stored webhook credentials
  • reviewed 2026-07-05T16:35:45.677Z sha 98e3895 :: needs real behavior proof before merge. :: [P1] Add upgrade-safe handling before rejecting stored webhook credentials
  • reviewed 2026-07-05T17:55:53.638Z sha 98e3895 :: needs real behavior proof before merge. :: [P1] Add upgrade-safe handling before rejecting stored webhook credentials

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot added triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 11, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 14, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 15, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 15, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 15, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 15, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 15, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 15, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress.

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.
What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@RomneyDa

Copy link
Copy Markdown
Member

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

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

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants