Skip to content

Review Endor remediation PRs until clean - #1241

Draft
jesse-merhi wants to merge 3 commits into
mainfrom
jesse/endor-until-clean-implementation
Draft

Review Endor remediation PRs until clean#1241
jesse-merhi wants to merge 3 commits into
mainfrom
jesse/endor-until-clean-implementation

Conversation

@jesse-merhi

@jesse-merhi jesse-merhi commented Aug 25, 2026

Copy link
Copy Markdown
Member

Endor remediation PRs currently receive one normal ClawSweeper review, with no exact-head convergence loop or strict notification fence. A single result cannot prove that the same commit stays clean across fresh reviews, and a delivery failure is not tied to publication completion. This PR requires three consecutive clean reviews on one unchanged head, resets progress after findings or a new commit, and sends the terminal result through Hermit. ClawSweeper completes publication only after it has queued the next review or durably recorded the terminal state and delivery receipt.

Change breakdown

Part Files +LOC -LOC
Implementation 3 +1,365 -1
Tests 5 +1,142 -3
Documentation 1 +46 -0
Workflow and tooling 2 +290 -6
Total 11 +2,843 -10

Proof

Before: direct base

fresh Endor review -> ordinary publication completes
clean streak: not tracked
terminal Hermit receipt: not required

After: PR

clean review 1, same head -> requeue, clean=1/3
clean review 2, same head -> requeue, clean=2/3
clean review 3, same head -> notify ready, clean=3/3
same terminal event replay -> skipped, existing receipt retained
new head or finding -> prior streak reset
ambiguous result -> never counted as clean

The controlled proof exercises the exact-review workflow, state files, head revalidation, Hermit request, retry classification, and durable delivery ledger. It does not run a live Endor tenant or send to production Discord because external configuration was deliberately left unchanged.

Linux proof passed 37/37 tests on AWS Crabbox lease cbx_8b4077021265. The exact rebased head also passes static checks, all builds, lint, and the 37 focused tests on Node 24.18.1.

How to verify

  1. Run pnpm run check:static, pnpm run build:all, and pnpm run lint on Node 24 or newer.
  2. Run the focused Endor notification, workflow, retry, and state-writer tests.
  3. Confirm transient Hermit failures produce retryable_failure/hermit_transient without a delivery receipt.

Implementation notes

This depends on openclaw/hermit#30, which must be deployed first. Then set CLAWSWEEPER_HERMIT_URL and CLAWSWEEPER_HERMIT_TOKEN in ClawSweeper. OpenClaw Bay is unaffected because this adds no public status, data contract, navigation, or controls.

@clawsweeper

clawsweeper Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

Comment thread test/repair/notify-endor-remediation.test.ts Fixed
Comment thread test/repair/notify-endor-remediation.test.ts Fixed
Comment thread test/repair/notify-endor-remediation.test.ts Fixed
@clawsweeper clawsweeper Bot added merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 25, 2026
@clawsweeper

clawsweeper Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 24, 2026, 10:12 PM ET / August 25, 2026, 02:12 UTC.

ClawSweeper review

What this changes

This PR adds an Endor-specific three-review convergence loop, durable state and receipts, and Hermit notifications to ClawSweeper’s exact-review publication workflow.

Merge readiness

Blocked by patch quality or review findings - 7 items remain

Keep open: the prior P1 security blocker remains on the current head—Hermit can still receive a bearer credential over cleartext HTTP.

Likely related people: Peter Steinberger (high confidence, recent exact-review publication work) and joshavant (medium confidence, recent authority-chain review work).

Priority: P1
Reviewed head: 007b49dbb8ba9e9fdb53344a405c907ea2f98f81

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The proposed workflow has substantial test coverage, but the unresolved plaintext credential transport flaw makes the patch not ready to merge.
Proof confidence 🌊 off-meta tidepool Not applicable: Not applicable: this MEMBER-authored PR is exempt from the external-contributor proof gate; its body describes controlled tests, but the security finding still blocks merge.
Patch quality 🧂 unranked krab (1/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Not applicable: this MEMBER-authored PR is exempt from the external-contributor proof gate; its body describes controlled tests, but the security finding still blocks merge.
Evidence reviewed 6 items Cleartext endpoint remains accepted: The current PR head explicitly accepts both HTTPS and HTTP for the Hermit URL, so a configured HTTP endpoint reaches the notifier instead of failing closed.
Bearer token is sent to that endpoint: The notifier attaches the configured Hermit token as an Authorization bearer credential to the accepted endpoint.
Workflow supplies production notification credentials: The Endor delivery step passes both the configured Hermit URL and secret token to the notifier, making the transport choice security-sensitive.
Findings 1 actionable finding [P1] Reject cleartext Hermit notification endpoints
Security Needs attention Bearer credential may be sent over cleartext HTTP: Endpoint validation allows http:, then the notifier posts the shared Hermit bearer token and structured review event to that endpoint.

How this fits together

Exact-review publication takes a durable GitHub review result and either completes it or schedules recovery work. This change recognizes Endor remediation PRs, persists their clean-review streak, and sends terminal results through Hermit before finalizing publication.

flowchart LR
A[Durable GitHub review] --> B[Endor eligibility check]
B --> C[Review convergence state]
C --> D{Three clean reviews?}
D -->|No| E[Queue next exact review]
D -->|Yes| F[Hermit delivery]
F --> G[Durable delivery receipt]
G --> H[Publication completion]
Loading

Before merge

  • Reject cleartext Hermit notification endpoints (P1) - The validator still accepts http: even though the subsequent request sends Authorization: Bearer ${config.token}. A configured plaintext endpoint exposes the delivery credential and terminal review payload; reject HTTP and add a regression test. This re-raises the unresolved prior review blocker.
  • Resolve security concern: Bearer credential may be sent over cleartext HTTP - Endpoint validation allows http:, then the notifier posts the shared Hermit bearer token and structured review event to that endpoint.
  • Resolve merge risk (P1) - A cleartext CLAWSWEEPER_HERMIT_URL would expose the Hermit bearer credential and terminal review payload on the network.
  • Resolve merge risk (P1) - This 11-file workflow and state change should not be landed until the current-head security fix is covered by the controlled publication proof.
  • Complete next step (P2) - A narrow, mechanical security repair is available: reject cleartext endpoints before the existing authenticated fetch path.
  • Improve patch quality - Reject http: Hermit endpoints and add a focused rejection test.
  • Improve patch quality - Rerun the stated controlled Endor publication proof against the new head after the security repair.

Findings

  • [P1] Reject cleartext Hermit notification endpoints — src/repair/hermit-notification.ts:37-38
  • [high] Bearer credential may be sent over cleartext HTTP — src/repair/hermit-notification.ts:37
Agent review details

Security

Needs attention: The new credential-bearing Hermit delivery path permits HTTP and can disclose its bearer token and payload.

Review metrics

Metric Value Why it matters
Changed surface 11 files, +2,843 / -10 lines The PR substantially alters review publication, state persistence, and notification behavior.
Production and test delta production +1,365, tests +1,142 The implementation is broadly exercised in tests, but the credential transport boundary still needs correction.

Merge-risk options

Maintainer options:

  1. Require TLS before merge (recommended)
    Reject http: Hermit URLs before any request is constructed and add a regression test proving no plaintext endpoint is accepted.
  2. Pause the Endor delivery route
    Do not enable this credential-bearing notification path until its endpoint validation and current-head controlled proof are complete.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Require HTTPS-only Hermit endpoints and add a regression test rejecting an http:// configuration while preserving custom HTTPS hosts.

Technical review

Best possible solution:

Require HTTPS-only Hermit origins, add a focused rejection test for http: input, then rerun the controlled Endor publication proof on the updated head.

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

Yes—source inspection shows that resolveHermitEndorNotificationUrl("http://...") accepts the endpoint and the notifier subsequently sends a bearer authorization header to it.

Is this the best way to solve the issue?

No—the convergence design can remain, but accepting HTTP is not safe for a credential-bearing delivery path; HTTPS-only validation is the narrow fix.

Full review comments:

  • [P1] Reject cleartext Hermit notification endpoints — src/repair/hermit-notification.ts:37-38
    The validator still accepts http: even though the subsequent request sends Authorization: Bearer ${config.token}. A configured plaintext endpoint exposes the delivery credential and terminal review payload; reject HTTP and add a regression test. This re-raises the unresolved prior review blocker.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P1: A misconfigured endpoint can disclose a production bearer credential and review payload during normal notification delivery.
  • merge-risk: 🚨 security-boundary: The PR introduces a new authenticated network delivery boundary that currently permits cleartext transport.
  • merge-risk: 🚨 automation: The PR changes exact-review queueing, durable publication, retry classification, and terminal completion behavior.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Not applicable: this MEMBER-authored PR is exempt from the external-contributor proof gate; its body describes controlled tests, but the security finding still blocks merge.

Evidence

Security concerns:

  • [high] Bearer credential may be sent over cleartext HTTP — src/repair/hermit-notification.ts:37
    Endpoint validation allows http:, then the notifier posts the shared Hermit bearer token and structured review event to that endpoint.
    Confidence: 0.99

Acceptance criteria:

  • [P1] pnpm run build:repair.
  • [P1] pnpm run test:repair.
  • [P1] pnpm run check.

What I checked:

Likely related people:

  • Peter Steinberger: Recent history includes the exact re-review drift publication path that this workflow extends. (role: recent exact-review publication contributor; confidence: high; commits: ce250708c1ea, a3469c99f6a8; files: .github/workflows/sweep.yml, dashboard/exact-review-queue.ts)
  • joshavant: Recent commits introduced and scoped authority-chain evaluation relevant to this new credential-bearing delivery path. (role: authority-chain review contributor; confidence: medium; commits: 868eb60db2fb, 9a09faa3da3b; files: .github/workflows/sweep.yml, src/repair/hermit-notification.ts)

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-08-25T02:02:58.541Z sha 6bb3081 :: needs changes before merge. :: [P1] Reject cleartext Hermit notification endpoints

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

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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.

2 participants