fix(queue): preserve empty legacy target branches - #889
Conversation
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 4:31 AM ET / 08:31 UTC. ClawSweeper reviewWhat this changesThis PR replaces whitespace-delimited legacy event fields with NUL-delimited fields so an omitted target branch remains empty and falls back to the target repository’s default branch instead of becoming the authority flag value. Merge readinessThis PR remains necessary: it repairs a concrete regression in legacy repository-dispatch intake, is narrowly scoped, and now includes credible after-fix terminal proof plus focused regression coverage. I found no discrete correctness defect in the proposed NUL-delimited handoff, but this shared workflow path should receive the stated post-merge issue, PR, and re-review canaries. Priority: P1 Review scores
Verification
How this fits togetherThe flowchart LR
A[Legacy repository-dispatch event] --> B[Legacy intake workflow]
B --> C[Decode repository branch and authority]
C --> D{Target branch supplied?}
D -->|No| E[Look up repository default branch]
D -->|Yes| F[Use supplied target branch]
E --> G[Durable review queue]
F --> G
G --> H[Fetch revision and generate review]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the narrow parser repair with its workflow-level regression coverage, then verify the three independent live intake canaries without replaying previously missed or parked events. Do we have a high-confidence way to reproduce the issue? Yes. The branchless legacy payload has a clear source-level failure mode: tab-delimited Bash Is this the best way to solve the issue? Yes. NUL-delimited fields with AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1aaed03c8d41. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(queue): preserve empty legacy target branches This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
Summary
Preserve an omitted target-branch field on legacy issue and pull-request intake so the event resolves the target repository's default branch instead of trying to fetch branch
0.Problem
#857 changed the legacy event parser to read three tab-separated fields with Bash
read. Tabs are IFS whitespace, so Bash collapses an empty middle field. For a branchless event, the serialized values were effectively:That shifted
0intotarget_branch, and intake failed or admitted the wrong branch before normal review generation. The shared parser affects any legacy payload that omitstarget_branch:pull_request_target/openedpayload withouttarget_branchin run 30235550150, then hit the affected receiver in run 30235555535.Implementation
mapfile, which preserves the empty branch slot.gh api "repos/$target_repo" --jq .default_branch.maincommand re-review.NUL is not legal in Git refs, so this avoids both whitespace collapsing and collisions with valid branch-name characters.
After-fix runtime proof
On current head
9caa0704d744fdb94cc1ed3d625131cac3f40e4a, Git Bash extracted and executed the workflow's embedded Node serializer, applied the NULmapfileassignments, and ran the workflow's live public default-branch lookup with this redacted issue payload:{"item_kind":"issue","target_repo":"openclaw/openclaw","target_branch":"","source_event":"issues","source_action":"opened"}Captured terminal output:
This demonstrates the required runtime behavior: exactly three fields survive; the target branch remains empty until GitHub resolves
main; and0remains the authority field rather than becoming the fetch branch. No token, credential, or secret value is included.Validation
node --test --test-name-pattern "legacy event field serializer preserves branchless issue and PR intake|sweep issue and PR event reviews and target fanout avoid storm amplification" test/sweep-workflow.test.ts["openclaw/openclaw", "", "0"]["openclaw/clawhub", "", "0"]["openclaw/clawhub", "", "1"]["openclaw/clawhub", "main", "0"]pnpm exec oxfmt --check .github/workflows/sweep.yml test/sweep-workflow.test.tsgit diff --checkrun:block through Git Bashbash -n.actionlintis not installed on this host. The changed surface is an Ubuntu-hosted Bash step; the focused workflow tests and explicit Git Bash syntax/runtime proof passed. Known native-Windows aggregate failures invoke WSL'sdocker-desktopenvironment without/bin/bashand are outside this hosted workflow surface.Review proof
codex review --uncommittedfound that a proposed|delimiter is legal in Git branch names. That finding was accepted; the parser was changed to NUL-delimitedmapfile, and focused proof was rerun.codex review --uncommittedwas clean: “The added regression coverage correctly executes the workflow serializer and verifies branchless issue and PR payload handling.”codex review --base origin/mainon9caa0704d744fdb94cc1ed3d625131cac3f40e4awas clean: “The workflow now uses NUL-delimited fields, preserving an empty target branch before resolving the repository default branch. The added serializer coverage exercises branchless issue and PR payloads.”Risks / rollout
target_branch: main, reached the durable control plane, and still did not publish before merge. That is not explained by this parser bug. It requires a separate trace of dispatch keyrouter-51dc3a77137b7009through queue parking/retry and publication state; this PR does not claim to fix it.@clawsweeper re-review.Links