fix(labels): make the label sync identity locale independent - #1139
fix(labels): make the label sync identity locale independent#1139dwin-gharibi wants to merge 6 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 1:49 AM ET / 05:49 UTC. ClawSweeper reviewWhat this changesThe PR replaces locale-sensitive label sorting with UTF-16 code-unit ordering when constructing batched label-sync identities, and adds regression and proof material. Merge readiness⛔ Blocked until stronger real behavior proof is added - 5 items remain Keep open: deterministic ordering fixes the recorded identity, but this PR still claims to close a duplicate-publication issue while the apply path records an attempt and then always invokes GitHub without an idempotency gate. Priority: P2 Review scores
Verification
How this fits togetherClawSweeper batches issue-label changes, derives a mutation identity, records the attempt in its action ledger, and invokes GitHub to edit labels. The identity is durable audit metadata; the apply runner remains responsible for deciding whether an operation may execute. flowchart LR
A[Review decisions] --> B[Label mutation batch]
B --> C[Canonical label ordering]
C --> D[Mutation identity]
D --> E[Apply attempt record]
E --> F[GitHub label edit]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep #1136 open unless this PR adds and proves a pre-execution duplicate gate; otherwise rescope this PR to deterministic recording only and remove the closing relation. Do we have a high-confidence way to reproduce the issue? Yes: the pinned base uses locale-sensitive sorting immediately before the identity is assembled, and the supplied two-locale trace identifies the affected path; this read-only review did not execute it. Is this the best way to solve the issue? No for the linked issue’s duplicate-publication outcome: code-unit ordering is the right narrow fix for stable recording identity, but it cannot by itself stop a repeated GitHub operation. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 220ad5673ace. 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
HistoryReview history (8 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Use the existing compareCodeUnits comparator for surviving addition and removal values so recorded label identities do not depend on locale or collator ties. Preserve normalized batch, fallback, freshness and receipt semantics. Recording identity remains separate from execution policy; repeated attempts still retain distinct receipts. Remove redundant proof provisioning and staging wrappers. Share one producer-to-persisted-recorder probe between the focused test and proof commands, with explicit baseline inputs and isolated verified locales. Local validation: 14 focused tests and 2 ledger controls passed, along with the qualified four-child baseline/candidate recorder probe and narrow checks. Remote/full validation remains pending. Ref: openclaw#1136 Co-authored-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
Closes #1136.
What Problem This Solves
Fixes an issue where the same batched label edit can be recorded under two different
idempotency keys, so the action ledger does not recognize it as a repeat and the
mutation can be published again.
flushIssueLabelMutationBatchstamps every published label edit withand the ledger dedupes observed mutations on that string. The two lists were sorted
with
String.prototype.localeCompare, which is locale-sensitive: it answers to theICU configuration of whatever machine produced the key. The identical label set,
batched identically, produces two different keys on two differently configured
runners:
localeComparealso returns0for strings a collator considers equivalent butthat are not equal — for example two label names differing only by a zero-width
joiner, which GitHub permits. A comparator that returns
0is not a total order, soArray.prototype.sortleaves those names in whatever order they were queued and thekey follows the queue rather than the set.
Why This Change Was Made
The sort feeds an identity, not a human-facing list, so it needs a total order that
is fully specified rather than a natural-language collation. The batch now sorts
with
compareCodeUnitsfromsrc/stable-json.ts— the same code-unit ordering theaction ledger already uses for its canonical JSON — so the key depends only on the
label set.
Non-goals: the label sync decision logic, the batching behavior, and the ledger's
dedupe mechanism are all unchanged.
User Impact
A label sync that has already been applied is recognized as already applied,
regardless of which runner produced the earlier record. Operators stop seeing the
same label edit published twice for one item, and the ledger's dedupe guarantee
holds across a heterogeneous fleet.
OpenClaw Bay Impact
None.
identityis an internal idempotency key for the action ledger; Bay does notread or display it. No status, telemetry or dashboard data contract changes.
Documentation Lifecycle
No documentation lifecycle changes. The new file under
docs/proof/label-sync-identity-determinism/is PR evidence, not a runbook orreference.
Evidence
Change
src/clawsweeper-label-mutations.ts— importcompareCodeUnits; use it for boththe additions and the removals sort.
test/label-mutation-batch.test.ts— one regression test, plus three existingassertions updated (see below).
11changed lines in source,89in tests.A visible ordering change, and why it is safe
Code-unit order puts uppercase before lowercase, so
P2now sorts beforeimpact:message-losswhere the collator put it after. That changes the order ofnames inside the
--add-label/--remove-labelarguments, and three existingassertions pinned the old order:
an exact-publication label batch emits one combined deterministic issue editlabel definition discovery is cached across item batchesoptional batch failures retain successful final operations and report skipped additionsEach was updated to the new order and still asserts the same behavior. GitHub treats
--add-labelas a set, so the resulting label state is unchanged. In the thirdtest the per-label retry order also flips, which moves where the receipt lands: the
fixture rejects
P2, soP2is now the first retry and draws no receipt whileimpact:message-losssucceeds and does.result.skippedAdditionsis still["P2"].Focused tests
Reverting only
src/clawsweeper-label-mutations.tsand rebuilding turns the newtest red along with the three updated ones (
pass 10 / fail 4), which is what showsthey bind to this change.
Full suite and static gates
pnpm run check:static,pnpm run lint,pnpm run format:checkandpnpm run build:allall pass. The full suite is2049 / 2066with 2 failures —test/action-ledger-runtime.test.tstemp-directory cleanup andtest/automerge-metrics.test.tsCLI JSON — both of which reproduce identically onunmodified
mainat the same commit and neither of which importsclawsweeper-label-mutations. That is exactly the baseline, with no additionalfailure introduced.
Real Behavior Proof
Claim. The
issue_labels_syncidentity depends only on the label set — not onthe order the labels were queued, and not on the runner's locale.
Exercised surface.
createLabelMutationOperations(...).flushIssueLabelMutationBatchindist/clawsweeper-label-mutations.js, driven through the real batching API(
beginIssueLabelMutationBatch,addIssueLabel,removeIssueLabel, flush).Scenario / fixture.
docs/proof/label-sync-identity-determinism/run-proof.mjsuses real ClawSweeperlabel names (
P2,impact:message-loss,maturity:stable,proof: sufficient)plus a third-party name, and asserts four things: reversed / sorted / rotated queue
orders all yield the baseline identity; two names differing only by a zero-width
joiner sort deterministically (the run prints the
localeCompareresult of0);the exact key for a fixed set matches byte for byte; and the file re-executes itself
under
LC_ALL=en_US.UTF-8andLC_ALL=sv_SE.UTF-8and compares the two keys.Command and environment.
local-container(runtimedocker)node:24→v24.19.0,Linux aarch64cbx_22f9e117ae32(swift-hermit)run_16ab137d00ec5439582b·src/clawsweeper-label-mutations.ts· sha256f2fee8cc…f8460The script refuses to run below Node 24, builds the Node lane, then runs the
fixtures twice — once against the module compiled from the base commit and once
against this branch. Container images carry no
.git, sostage-before.shwritesthe base version of the changed file into the proof package on the host and it
rsyncs in; when git is available the script re-derives that file, so it cannot
drift from the base commit.
Observed result. Pre-fix, inside the lease, the two runners disagree:
The tied emoji names also follow queue order instead of sorting, and the exact key
assertion fails. On this branch both runners emit
issue_labels_sync:321:add=Alpha|apple|zulu|äpple:remove=, every assertion passes,and the focused suite is
pass 14 / fail 0. A batch still publishes a mutation inboth runs, and the script fails the proof if that ever stops holding — which is
what shows the change only affects ordering.
Artifact / trace.
.crabbox/runs/run_16ab137d00ec/run_16ab137d00ec-artifacts.tgzcontaining.artifacts/label-sync-identity-determinism-proof/—before-output.txt,proof-output.txt,focused-tests.txt, install and build logs.Limits. This proves the identity string and the ordering that feeds it. It does
not call GitHub and does not exercise the ledger's dedupe path end to end — it
establishes that the key handed to the ledger is stable, not what the ledger then
does with it. It also does not survey the repository for other locale-sensitive
sorts; the linked issue records what was checked.