Skip to content

feat: reconcile externally managed channels - #126

Closed
PollyBot13 wants to merge 5 commits into
openclaw:mainfrom
PollyBot13:feat/managed-channel-reconcile
Closed

feat: reconcile externally managed channels#126
PollyBot13 wants to merge 5 commits into
openclaw:mainfrom
PollyBot13:feat/managed-channel-reconcile

Conversation

@PollyBot13

@PollyBot13 PollyBot13 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Closes #125

What Problem This Solves

External integrations cannot safely ensure that one durable ClickClack channel
exists for one external work item. Retried webhooks or concurrent workers can
create duplicates, while later lifecycle changes have no stable provider
identity to target.

Why This Change Was Made

This adds full desired-state reconciliation by immutable
(workspace, external_provider, external_ref) identity across SQLite and
Postgres, with matching HTTP, OpenAPI, and TypeScript SDK support. Exact replays
return the existing channel without another durable event; changed state emits
one normal channel update. Provider adapters, credentials, and external
workflow authority remain outside ClickClack core.

This is the first infrastructure step toward the project goal: an integration
can keep one dedicated ClickClack channel per work item—such as a pull
request—synchronized while ClickClack owns the durable collaboration record.

User Impact

Bots and integrations with channels:write can now create, update, archive,
and reopen one managed channel idempotently. Concurrent requests converge on
one channel, provider namespaces remain isolated, and ordinary channel PATCH
requests cannot detach or retarget reconciled identity.

Evidence

Exact proof head: 5c0f2629aa623884d22225229138bcb29ee946b0

  • pnpm check passes: Go, web, SDK, desktop, FakeCo, builds, typechecks, lint,
    and formatting.
  • The generated TypeScript contract now follows the OpenAPI required arrays:
    defaulted-but-optional request fields remain optional. Compile-time contract
    assertions cover channel creation, message creation, and managed-channel
    reconciliation.
  • Focused HTTP proof uses a real httptest server with a migrated temporary
    SQLite database—no mocked store—and covers create/replay/update/defaulting,
    archive/reopen, immutable identity, bot scope, and workspace binding.
  • Six concurrent reconciliations converge on one identity in both SQLite and a
    real local PostgreSQL 17 server.
  • Upgrade tests start from the schema immediately before the reconciliation
    migration in each dialect, insert a legacy managed channel, run the current
    migrator, verify the legacy row is preserved without a provider, and verify a
    namespaced reconciliation creates its own identity.
  • Independent Codex review at the exact head reported no actionable
    correctness issues.

Redacted command transcript:

$ git rev-parse HEAD
5c0f2629aa623884d22225229138bcb29ee946b0

$ go test -count=1 -v ./apps/api/internal/httpapi \
    -run 'TestManagedChannelReconciliationBotAuthorization|TestMutationAndEphemeralEndpoints'
... reconcile status=201
... replay/update status=200
... cross-workspace bot request status=403
--- PASS: TestManagedChannelReconciliationBotAuthorization
--- PASS: TestMutationAndEphemeralEndpoints
PASS

$ go test -count=1 -v ./apps/api/internal/store/sqlite \
    -run 'TestManagedChannelReconciliation(MigrationPreservesLegacyChannels|Converges)$'
--- PASS: TestManagedChannelReconciliationMigrationPreservesLegacyChannels
--- PASS: TestManagedChannelReconciliationConverges
PASS

$ CLICKCLACK_POSTGRES_TEST_DSN='<local PostgreSQL 17; redacted>' \
    go test -count=1 -v ./apps/api/internal/store/postgres \
    -run 'TestManagedChannelReconciliation(MigrationPreservesLegacyChannelsPostgres|ConvergesPostgres)$'
--- PASS: TestManagedChannelReconciliationConvergesPostgres
--- PASS: TestManagedChannelReconciliationMigrationPreservesLegacyChannelsPostgres
PASS

$ pnpm --filter @clickclack/sdk-ts typecheck
$ tsc --noEmit -p tsconfig.json

The earlier proof narrative remains available in
docs/proof/managed-channel-reconciliation-proof.md.

@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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 25, 2026
@clawsweeper

clawsweeper Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed August 2, 2026, 3:13 AM ET / 07:13 UTC.

ClawSweeper review

What this changes

This branch adds an idempotent API that maps a workspace, external-provider, and external-reference identity to one durable managed channel across SQLite, PostgreSQL, HTTP, OpenAPI, and the TypeScript SDK.

Merge readiness

Blocked by patch quality or review findings - 8 items remain

This PR is still necessary for the requested provider-identity reconciliation capability, but it is not merge-ready. The two previously reported permanent name-collision defects remain at the unchanged head, and the branch has concrete conflicts with current main after channel display titles were added.

Priority: P2
Reviewed head: 5c0f2629aa623884d22225229138bcb29ee946b0
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The real-store proof is strong, but two confirmed correctness defects and a dirty merge state leave the patch below merge-ready quality.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The contributor supplied exact-head redacted terminal proof for real HTTP behavior against migrated SQLite, real local PostgreSQL concurrency, and migration upgrades; the remaining collision case is a functional gap rather than missing proof.
Patch quality 🦪 silver shellfish (2/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The contributor supplied exact-head redacted terminal proof for real HTTP behavior against migrated SQLite, real local PostgreSQL concurrency, and migration upgrades; the remaining collision case is a functional gap rather than missing proof.
Evidence reviewed 6 items SQLite collision retry remains: The exact PR head retries a workspace/name uniqueness violation along with the managed-identity uniqueness violation. An unrelated existing channel with the requested slug therefore cannot become a successful reconciliation on retry; every attempt repeats the same insert and ends with a generic concurrency error.
PostgreSQL collision retry remains: The PostgreSQL implementation has the same retry classifier and also explicitly tests that a permanent workspace/name collision should be retried, so the earlier finding has not been addressed on the current head.
Current-main channel model changed: Current main adds DisplayTitle to channel mutation persistence, while the PR modifies the same mutation, SQL, adapter, generated-store, and test surfaces from an older base. The GitHub merge state is dirty and a three-way merge reports conflicts across these central paths.
Findings 2 actionable findings [P2] Return permanent SQLite name conflicts without retrying
[P2] Return permanent PostgreSQL name conflicts without retrying
Security None None.

How this fits together

External integrations provision ClickClack workspace channels through the HTTP API, while SQLite and PostgreSQL persist the channel and its durable events. The proposed reconciliation endpoint would make one provider-owned external work item converge on one channel and return its create, update, or unchanged result to SDK and realtime clients.

flowchart LR
  A[External integration] --> B[Managed-channel reconcile API]
  B --> C[Scope and workspace authorization]
  C --> D[Provider identity lookup]
  D --> E[SQLite or PostgreSQL transaction]
  E --> F[Channel record and durable event]
  F --> G[SDK and realtime clients]
Loading

Decision needed

Question Recommendation
Should ClickClack adopt provider-neutral managed-channel reconciliation as a permanent core HTTP, SDK, and persisted-schema contract, rather than leaving provider lifecycle provisioning to external integrations using the existing managed-channel API? Sponsor the core primitive: Accept the provider-neutral reconciliation contract after the branch is rebased and the two collision defects are fixed.

Why: The patch is not a repair of an established contract: it creates a new public endpoint and immutable stored identity with upgrade and compatibility obligations. Source review cannot determine whether that product boundary is intended.

Before merge

  • Return permanent SQLite name conflicts without retrying (P2) - The retry classifier treats the separate (workspace_id, name) uniqueness error as a concurrent managed-identity race. If an unrelated channel already owns the requested slug, every retry rereads the absent provider identity and attempts the same insert, then returns a generic concurrency error. Return the name conflict directly and add coverage for that pre-existing-channel case.
  • Return permanent PostgreSQL name conflicts without retrying (P2) - The PostgreSQL classifier repeats the SQLite behavior and explicitly classifies channels_workspace_id_name_key as retryable. An unrelated existing channel with the desired name can never converge through this loop, so return that conflict directly and cover the case instead of exhausting five retries.
  • Resolve merge risk (P1) - Merging the current dirty branch requires resolving conflicts across channel persistence, sqlc inputs/generated outputs, HTTP tests, and SDK contracts; an incorrect resolution could omit current display-title behavior.
  • Resolve merge risk (P1) - The endpoint adds a durable public identity and migration contract, so maintainers need an explicit decision that this provider-neutral reconciliation API belongs in ClickClack core before accepting its long-term compatibility burden.
  • Improve patch quality - Fix the SQLite and PostgreSQL permanent-name collision classifiers and add focused regression coverage.
  • Improve patch quality - Rebase onto current main and resolve the channel display-title conflicts without losing current behavior.
  • Improve patch quality - Obtain maintainer confirmation that the new durable reconciliation API belongs in core.

Findings

  • [P2] Return permanent SQLite name conflicts without retrying — apps/api/internal/store/sqlite/managed_channels.go:204-207
  • [P2] Return permanent PostgreSQL name conflicts without retrying — apps/api/internal/store/postgres/managed_channels.go:204-207
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 39 files affected; 1,791 added and 294 removed The feature crosses persisted schema, two database backends, HTTP, OpenAPI, SDK generation, tests, and documentation.
Database migrations 2 added migrations SQLite and PostgreSQL both gain a persisted external-provider identity and partial unique index, making upgrade behavior merge-critical.

Merge-risk options

Maintainer options:

  1. Rebase and repair before merge (recommended)
    Resolve the current-main channel-model conflicts, retain display-title support, and stop retrying permanent unrelated channel-name conflicts in both storage backends.
  2. Pause for product sponsorship
    Keep the PR open but do not merge the new persisted API until a maintainer confirms that provider-neutral reconciliation belongs in core.

Technical review

Best possible solution:

Rebase onto current main, preserve channel display-title behavior through both database backends and generated sqlc outputs, return unrelated workspace/name conflicts directly rather than retrying them, then obtain maintainer sponsorship for the new durable core API.

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

Yes for the review defect: create an unrelated channel with the requested slug, then reconcile a new provider identity using that name; source shows both backends retry the permanent uniqueness failure five times instead of returning it. The broader requested capability is a feature, not an existing bug report.

Is this the best way to solve the issue?

No: the current branch needs a rebase for the current channel model and must distinguish retryable identity races from permanent name conflicts; core adoption also requires maintainer product sponsorship.

Full review comments:

  • [P2] Return permanent SQLite name conflicts without retrying — apps/api/internal/store/sqlite/managed_channels.go:204-207
    The retry classifier treats the separate (workspace_id, name) uniqueness error as a concurrent managed-identity race. If an unrelated channel already owns the requested slug, every retry rereads the absent provider identity and attempts the same insert, then returns a generic concurrency error. Return the name conflict directly and add coverage for that pre-existing-channel case.
    Confidence: 0.99
  • [P2] Return permanent PostgreSQL name conflicts without retrying — apps/api/internal/store/postgres/managed_channels.go:204-207
    The PostgreSQL classifier repeats the SQLite behavior and explicitly classifies channels_workspace_id_name_key as retryable. An unrelated existing channel with the desired name can never converge through this loop, so return that conflict directly and cover the case instead of exhausting five retries.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This is a substantial but bounded new integration capability with normal compatibility-sensitive impact rather than an active outage.
  • merge-risk: 🚨 compatibility: The PR creates a new public endpoint and SDK method plus a persisted managed-channel identity that must remain compatible across upgrades.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦞 diamond lobster and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The contributor supplied exact-head redacted terminal proof for real HTTP behavior against migrated SQLite, real local PostgreSQL concurrency, and migration upgrades; the remaining collision case is a functional gap rather than missing proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. The contributor supplied exact-head redacted terminal proof for real HTTP behavior against migrated SQLite, real local PostgreSQL concurrency, and migration upgrades; the remaining collision case is a functional gap rather than missing proof.

Evidence

What I checked:

  • SQLite collision retry remains: The exact PR head retries a workspace/name uniqueness violation along with the managed-identity uniqueness violation. An unrelated existing channel with the requested slug therefore cannot become a successful reconciliation on retry; every attempt repeats the same insert and ends with a generic concurrency error. (apps/api/internal/store/sqlite/managed_channels.go:204, 5c0f2629aa62)
  • PostgreSQL collision retry remains: The PostgreSQL implementation has the same retry classifier and also explicitly tests that a permanent workspace/name collision should be retried, so the earlier finding has not been addressed on the current head. (apps/api/internal/store/postgres/managed_channels.go:204, 5c0f2629aa62)
  • Current-main channel model changed: Current main adds DisplayTitle to channel mutation persistence, while the PR modifies the same mutation, SQL, adapter, generated-store, and test surfaces from an older base. The GitHub merge state is dirty and a three-way merge reports conflicts across these central paths. (apps/api/internal/store/sqlite/mutations.go:276, b8293987baa5)
  • SQL generation policy observed: The branch changes both sqlc schema/query sources and their generated storedb outputs for SQLite and PostgreSQL, matching the repository policy rather than hand-editing generated code alone. (apps/api/internal/store/sqlite/sqlc/queries.sql:484, 5c0f2629aa62)
  • Real behavior proof supplied: The PR body provides an exact-head redacted transcript for real HTTP/SQLite behavior, PostgreSQL concurrency, and pre-migration upgrade coverage; this is meaningful after-fix proof, though it does not cover the permanent unrelated-name collision. (docs/proof/managed-channel-reconciliation-proof.md:1, 5c0f2629aa62)
  • Owner-history limitation: Targeted broader history searches could not hydrate missing promisor objects because outbound DNS resolution failed, but current-file blame and available logs identify the recent channel-model contributors below. (apps/api/internal/store/sqlite/mutations.go:276, b8293987baa5)

Likely related people:

  • Peter Steinberger: Current main attributes the added channel display-title persistence to this contributor; that current-main change conflicts with the PR's storage and generated-code surface. (role: recent channel-model contributor; confidence: high; commits: e455d43809a0; files: apps/api/internal/store/sqlite/mutations.go, apps/api/internal/store/postgres/mutations.go)
  • Shakker: Current blame attributes the surrounding mutable channel and externally managed-field persistence logic to this contributor in both database backends. (role: existing channel-storage contributor; confidence: medium; commits: 5000b7ef4295; files: apps/api/internal/store/sqlite/mutations.go, apps/api/internal/store/postgres/mutations.go)

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 (24 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-31T23:35:37.384Z sha 5c0f262 :: found issues before merge. :: [P2] Do not retry permanent channel-name collisions | [P2] Do not retry permanent channel-name collisions in PostgreSQL
  • reviewed 2026-08-01T04:06:26.833Z sha 5c0f262 :: found issues before merge. :: [P2] Stop retrying permanent channel-name collisions | [P2] Stop retrying permanent PostgreSQL name collisions
  • reviewed 2026-08-01T09:34:43.485Z sha 5c0f262 :: found issues before merge. :: [P2] Stop retrying permanent channel-name collisions | [P2] Stop retrying permanent PostgreSQL name collisions
  • reviewed 2026-08-01T11:27:36.880Z sha 5c0f262 :: needs changes before merge. :: [P2] Stop retrying permanent SQLite channel-name collisions | [P2] Stop retrying permanent PostgreSQL channel-name collisions
  • reviewed 2026-08-01T12:52:58.479Z sha 5c0f262 :: found issues before merge. :: [P2] Stop retrying permanent SQLite channel-name collisions | [P2] Stop retrying permanent PostgreSQL channel-name collisions
  • reviewed 2026-08-01T14:52:57.520Z sha 5c0f262 :: needs changes before merge. :: [P2] Stop retrying permanent SQLite channel-name collisions | [P2] Stop retrying permanent PostgreSQL channel-name collisions
  • reviewed 2026-08-01T17:33:50.074Z sha 5c0f262 :: needs changes before merge. :: [P2] Stop retrying permanent SQLite channel-name collisions | [P2] Stop retrying permanent PostgreSQL channel-name collisions
  • reviewed 2026-08-01T23:47:54.590Z sha 5c0f262 :: found issues before merge. :: [P2] Return SQLite name collisions without retrying | [P2] Return PostgreSQL name collisions without retrying

@PollyBot13

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

The actionable TypeScript optionality mismatch is fixed at exact head
5c0f2629aa623884d22225229138bcb29ee946b0. The PR body now includes
inspectable real HTTP/SQLite, PostgreSQL 17 concurrency, and pre-migration
upgrade transcripts. Full checks pass and an independent exact-head Codex
review found no actionable correctness issues.

@clawsweeper

clawsweeper Bot commented Jul 25, 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: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

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. 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 25, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 30, 2026
@steipete

steipete commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Closing after maintainer review because this branch no longer matches the repository's current channel display-title schema and migration sequence. Its naming contract is now stale, and collision retries eventually surface as a generic concurrency error instead of a stable user-facing uniqueness contract.

This needs one reconciled public naming API, not a second compatibility path layered over the current display-title work. The product decision remains tracked in #125; a successor should start from current main only if that contract is approved. Thanks for developing and proving the original approach.

@steipete steipete closed this Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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.

Reconcile externally managed channels by provider identity

2 participants