Skip to content

test(outbound): align implicit source reply sink#88673

Closed
vincentkoc wants to merge 1 commit into
mainfrom
fix-current-main-source-reply-ci-20260531
Closed

test(outbound): align implicit source reply sink#88673
vincentkoc wants to merge 1 commit into
mainfrom
fix-current-main-source-reply-ci-20260531

Conversation

@vincentkoc

@vincentkoc vincentkoc commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Updates the current-main outbound routing test to expect the internal source sink for implicit message_tool_only source replies.
  • Keeps explicit current-source sends covered by the following Slack plugin-delivery test.

Verification

  • node scripts/run-vitest.mjs src/infra/outbound/message-action-runner.core-send.test.ts --reporter=verbose: passed, 8 tests.
  • ./node_modules/.bin/oxfmt --check src/infra/outbound/message-action-runner.core-send.test.ts: passed.
  • node scripts/run-oxlint.mjs src/infra/outbound/message-action-runner.core-send.test.ts: passed.
  • git diff --check: passed.
  • node scripts/changed-lanes.mjs --json: selected only coreTests.
  • node scripts/check-changed.mjs --dry-run: selected only coreTests.
  • GitHub checks after ready-for-review: pass/skipped only, no pending/failing checks in the final poll.

Real behavior proof

Behavior addressed: current main's CI expected implicit non-webchat message_tool_only source replies to call the Slack plugin send path, but the runtime now routes that implicit source reply through the internal source sink.

Real environment tested: local linked gwt worktree with the exact failing outbound test file.

Exact steps or command run after this patch: node scripts/run-vitest.mjs src/infra/outbound/message-action-runner.core-send.test.ts --reporter=verbose; ./node_modules/.bin/oxfmt --check src/infra/outbound/message-action-runner.core-send.test.ts; node scripts/run-oxlint.mjs src/infra/outbound/message-action-runner.core-send.test.ts; git diff --check; node scripts/changed-lanes.mjs --json; node scripts/check-changed.mjs --dry-run; ghx pr checks 88673.

Evidence after fix: the previously failing test file now passes 8/8, and GitHub checks are pass/skipped only.

Observed result after fix: the implicit source-reply test asserts handledBy: "internal-source", to: "current-run", and no Slack sendText call.

What was not tested: no broad local pnpm check:changed; this is a one-file test-only CI unblocker and GitHub PR checks covered the ready-for-review gate.

@vincentkoc vincentkoc self-assigned this May 31, 2026
@openclaw-barnacle openclaw-barnacle Bot added the maintainer Maintainer-authored PR label May 31, 2026
@vincentkoc vincentkoc mentioned this pull request May 31, 2026
Closed
@vincentkoc
vincentkoc marked this pull request as ready for review May 31, 2026 15:03
@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed June 29, 2026, 7:11 PM ET / 23:11 UTC.

Summary
The branch changes one outbound core-send test to expect an implicit message_tool_only Slack source reply to route to the internal source sink instead of Slack plugin delivery.

PR surface: Tests +13. Total +13 across 1 file.

Reproducibility: yes. at source level for the review finding: current main and v2026.6.10 keep configured Slack current-source sends on plugin delivery, while the PR assertion expects internal-source handling. I did not run tests because this is a read-only review.

Review metrics: 1 noteworthy metric.

  • Sink Expectation Changed: 1 implicit current-source assertion inverted. The test expectation changes from Slack plugin delivery to internal-source handling, which affects the regression guard for message delivery policy.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🌊 off-meta tidepool
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Rebase or replace the branch so it is no longer DIRTY / CONFLICTING.
  • Keep the configured Slack current-source route on plugin delivery unless maintainers intentionally change runtime policy.
  • Rerun the focused outbound test file after the policy-aligned update.

Risk before merge

Maintainer options:

  1. Rebase To Current Sink Policy (recommended)
    Update the assertion against current main so the configured Slack current-source route continues proving plugin delivery, or include the deliberate runtime policy change and sibling tests that justify a new expectation.
  2. Pause Behind Canonical Routing Work
    Hold this PR until maintainers decide the external-plugin routing behavior in message(action=send) without to fails for plugin channels under message_tool_only #94597 and fix(outbound): prevent internal sink from intercepting action-only pl… #94952.

Next step before merge

  • [P2] The PR is protected and the remaining action is maintainer delivery-policy judgment plus rebase, not a narrow automated repair.

Security
Cleared: The diff only changes a test expectation and does not touch workflows, dependencies, secrets, package metadata, or runtime security boundaries.

Review findings

  • [P2] Rebase the assertion onto current sink policy — src/infra/outbound/message-action-runner.core-send.test.ts:251-264
Review details

Best possible solution:

Rebase or replace the test update so configured current-source plugin delivery stays covered unless a deliberate runtime policy change lands with sibling tests.

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

Yes, at source level for the review finding: current main and v2026.6.10 keep configured Slack current-source sends on plugin delivery, while the PR assertion expects internal-source handling. I did not run tests because this is a read-only review.

Is this the best way to solve the issue?

No. A test-only assertion change is not the best fix while the current and shipped runtime still preserve configured current-source plugin delivery and the broader sink-policy question is tracked separately.

Full review comments:

  • [P2] Rebase the assertion onto current sink policy — src/infra/outbound/message-action-runner.core-send.test.ts:251-264
    The added expectation asserts handledBy: "internal-source" and no Slack send for a configured Slack source route. Current main and v2026.6.10 keep configured current-source channels on normal plugin delivery, so this test should keep proving sendText unless a runtime policy change with sibling coverage lands.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a narrow outbound test PR with limited blast radius, but it currently conflicts with current and released message-routing behavior.
  • merge-risk: 🚨 message-delivery: Merging the test change would weaken coverage for configured current-source plugin delivery and could let message-routing policy drift.
  • 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: The external contributor proof gate does not apply to this maintainer-labeled test-only PR; the body records focused local test and lint evidence for reviewer context.
Evidence reviewed

PR surface:

Tests +13. Total +13 across 1 file.

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

What I checked:

Likely related people:

  • vincentkoc: Recent history shows work on internal-source-reply and this exact outbound core-send test surface, including the current PR branch. (role: recent area contributor; confidence: high; commits: 0ea08076c3b5, 09bfefe04138; files: src/infra/outbound/internal-source-reply.ts, src/infra/outbound/message-action-runner.ts, src/infra/outbound/message-action-runner.core-send.test.ts)
  • steipete: Commit history shows configured source-reply routing work on the same message-action runner and core-send test boundary. (role: adjacent source-reply contributor; confidence: medium; commits: 1955f42bfeb2; files: src/infra/outbound/message-action-runner.ts, src/infra/outbound/message-action-runner.core-send.test.ts)
  • tianxiaochannel-oss88: The merged implicit tool-only source-reply PR introduced adjacent best-effort/current-source behavior that this test intersects with. (role: introduced adjacent behavior; confidence: medium; commits: f547ea7668a1; files: src/infra/outbound/message-action-runner.ts, src/infra/outbound/message-action-runner.core-send.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 (1 earlier review cycle)
  • reviewed 2026-06-29T22:15:45.876Z sha 09bfefe :: found issues before merge. :: [P2] Keep configured source replies on plugin delivery

@clawsweeper clawsweeper Bot added 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. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. P2 Normal backlog priority with limited blast radius. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels May 31, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 19, 2026
This was referenced Jun 19, 2026
Open
Closed
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Closing this old maintainer branch because the scope is not a landable, durable unit: it has no linked user bug or owner request and is either test-only maintenance, a broad mixed QA rewrite, or a very large compatibility/config redesign whose risk and review cost dwarf the demonstrated value.

Smaller behavior-backed fixes can be reopened as focused PRs when a current issue or release gate needs them. Thanks for the work and validation invested here.

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

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

maintainer Maintainer-authored PR merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @steipete