Skip to content

fix(labels): make the label sync identity locale independent - #1139

Open
dwin-gharibi wants to merge 6 commits into
openclaw:mainfrom
dwin-gharibi:fix/label-order-determinism
Open

fix(labels): make the label sync identity locale independent#1139
dwin-gharibi wants to merge 6 commits into
openclaw:mainfrom
dwin-gharibi:fix/label-order-determinism

Conversation

@dwin-gharibi

Copy link
Copy Markdown
Contributor

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.

flushIssueLabelMutationBatch stamps every published label edit with

issue_labels_sync:<number>:add=<sorted additions>:remove=<sorted removals>

and the ledger dedupes observed mutations on that string. The two lists were sorted
with String.prototype.localeCompare, which is locale-sensitive: it answers to the
ICU configuration of whatever machine produced the key. The identical label set,
batched identically, produces two different keys on two differently configured
runners:

en_US.UTF-8  issue_labels_sync:321:add=Alpha|apple|äpple|zulu:remove=
sv_SE.UTF-8  issue_labels_sync:321:add=Alpha|apple|zulu|äpple:remove=

localeCompare also returns 0 for strings a collator considers equivalent but
that are not equal — for example two label names differing only by a zero-width
joiner, which GitHub permits. A comparator that returns 0 is not a total order, so
Array.prototype.sort leaves those names in whatever order they were queued and the
key 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 compareCodeUnits from src/stable-json.ts — the same code-unit ordering the
action 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. identity is an internal idempotency key for the action ledger; Bay does not
read 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 or
reference.

Evidence

Change

  • src/clawsweeper-label-mutations.ts — import compareCodeUnits; use it for both
    the additions and the removals sort.
  • test/label-mutation-batch.test.ts — one regression test, plus three existing
    assertions updated (see below).

11 changed lines in source, 89 in tests.

A visible ordering change, and why it is safe

Code-unit order puts uppercase before lowercase, so P2 now sorts before
impact:message-loss where the collator put it after. That changes the order of
names inside the --add-label / --remove-label arguments, and three existing
assertions pinned the old order:

  • an exact-publication label batch emits one combined deterministic issue edit
  • label definition discovery is cached across item batches
  • optional batch failures retain successful final operations and report skipped additions

Each was updated to the new order and still asserts the same behavior. GitHub treats
--add-label as a set, so the resulting label state is unchanged. In the third
test the per-label retry order also flips, which moves where the receipt lands: the
fixture rejects P2, so P2 is now the first retry and draws no receipt while
impact:message-loss succeeds and does. result.skippedAdditions is still ["P2"].

Focused tests

$ node --test test/label-mutation-batch.test.ts
ℹ tests 14   ℹ pass 14   ℹ fail 0

Reverting only src/clawsweeper-label-mutations.ts and rebuilding turns the new
test red along with the three updated ones (pass 10 / fail 4), which is what shows
they bind to this change.

Full suite and static gates

pnpm run check:static, pnpm run lint, pnpm run format:check and
pnpm run build:all all pass. The full suite is 2049 / 2066 with 2 failures —
test/action-ledger-runtime.test.ts temp-directory cleanup and
test/automerge-metrics.test.ts CLI JSON — both of which reproduce identically on
unmodified main at the same commit and neither of which imports
clawsweeper-label-mutations. That is exactly the baseline, with no additional
failure introduced.

Real Behavior Proof

Claim. The issue_labels_sync identity depends only on the label set — not on
the order the labels were queued, and not on the runner's locale.

Exercised surface.
createLabelMutationOperations(...).flushIssueLabelMutationBatch in
dist/clawsweeper-label-mutations.js, driven through the real batching API
(beginIssueLabelMutationBatch, addIssueLabel, removeIssueLabel, flush).

Scenario / fixture.
docs/proof/label-sync-identity-determinism/run-proof.mjs uses real ClawSweeper
label 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 localeCompare result of 0);
the exact key for a fixed set matches byte for byte; and the file re-executes itself
under LC_ALL=en_US.UTF-8 and LC_ALL=sv_SE.UTF-8 and compares the two keys.

Command and environment.

bash docs/proof/label-sync-identity-determinism/stage-before.sh
crabbox run --provider local-container --local-container-image node:24 --no-hydrate \
  --artifact-glob '.artifacts/**' -- \
  bash docs/proof/label-sync-identity-determinism/run-proof.sh
provider local-container (runtime docker)
image node:24v24.19.0, Linux aarch64
lease cbx_22f9e117ae32 (swift-hermit)
run run_16ab137d00ec
base staged 5439582b · src/clawsweeper-label-mutations.ts · sha256 f2fee8cc…f846
exit 0

The 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, so stage-before.sh writes
the 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:

== 4. two differently configured runners agree ==
  en_US.UTF-8  issue_labels_sync:321:add=Alpha|apple|äpple|zulu:remove=
  sv_SE.UTF-8  issue_labels_sync:321:add=Alpha|apple|zulu|äpple:remove=
  FAIL  en_US and sv_SE produce the same identity

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 in
both 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.tgz containing
.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.

Copilot AI lite review requested due to automatic review settings August 12, 2026 11:45
@clawsweeper

clawsweeper Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 12, 2026
@clawsweeper

clawsweeper Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 1:49 AM ET / 05:49 UTC.

ClawSweeper review

What this changes

The 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
Reviewed head: 7aa1c8ed0a0d1788ab600d31d49440266d08765f

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The ordering implementation is narrow and plausible, but the linked defect remains unresolved and the current-head proof record is inconsistent.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The changed production owner is the batch-to-identity path in src/clawsweeper-label-mutations.ts; the body claims a Crabbox after-fix trace, but its command depends on wrapper scripts absent from the current head and records a different base than the pinned PR base. The current executable proof is a useful controlled recording probe, but the supplied body does not establish a current-head real-behavior result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The changed production owner is the batch-to-identity path in src/clawsweeper-label-mutations.ts; the body claims a Crabbox after-fix trace, but its command depends on wrapper scripts absent from the current head and records a different base than the pinned PR base. The current executable proof is a useful controlled recording probe, but the supplied body does not establish a current-head real-behavior result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Deterministic identity change: The introduced hunk sorts both effective label lists with the existing total code-unit comparator before constructing the mutation identity.
Execution remains ungated: The production apply runner writes a mutation-attempt record and then directly calls the operation; it contains no decision based on a prior matching mutation identity.
Ledger contract: The current ledger documentation explicitly says idempotency keys identify an external side effect independently of event identity and do not authorize a duplicate mutation.
Findings 1 actionable finding [P1] Do not close the duplicate-edit issue without a gate
Security None None.

How this fits together

ClawSweeper 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]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The changed production owner is the batch-to-identity path in src/clawsweeper-label-mutations.ts; the body claims a Crabbox after-fix trace, but its command depends on wrapper scripts absent from the current head and records a different base than the pinned PR base. The current executable proof is a useful controlled recording probe, but the supplied body does not establish a current-head real-behavior result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Do not close the duplicate-edit issue without a gate (P1) - The newly canonical identity is still passed to ghObservedMutationCommand, whose apply runner records an attempt and then calls the GitHub operation. The ledger documents that this identity does not authorize duplicate prevention, so the reported duplicate edit can still occur while merging would close The issue_labels_sync idempotency key varies by runner locale #1136. Remove the closing relation or add and prove a pre-execution gate. This is the prior blocker, still unfixed.
  • Resolve merge risk (P1) - Merging with the current closing relation would close The issue_labels_sync idempotency key varies by runner locale #1136 even though a matching recorded identity still does not prevent the GitHub edit from being invoked.
  • Resolve merge risk (P1) - The body’s claimed runtime proof references removed wrapper scripts and an older base, so it is not auditable as proof for the current head.
  • Complete next step (P2) - The contributor must resolve the existing P1 scope blocker and refresh proof; no autonomous repair should choose between rescoping the issue relationship and adding an execution policy.

Findings

  • [P1] Do not close the duplicate-edit issue without a gate — src/clawsweeper-label-mutations.ts:303-307
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Change surface production +4/-2, tests +51/-9, proof material +334/-0 The runtime change is narrow, while the executable proof material needs to remain aligned with the current PR body and head.

Root-cause cluster

Relationship: partial_overlap
Canonical: #1136
Summary: This PR addresses the issue’s locale-dependent identity component, but it does not implement the reported pre-execution duplicate prevention.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Rescope the PR to recording determinism (recommended)
    Remove the closing relation, state that this change stabilizes audit identity only, and refresh proof against the current head.
  2. Implement duplicate prevention
    Add a pre-execution gate at the apply boundary and prove that a prior matching identity rejects the GitHub operation before I/O.
  3. Pause the branch
    Pause this PR if neither the narrower recording-only change nor a safe execution gate is desired.

Technical review

Best 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:

  • [P1] Do not close the duplicate-edit issue without a gate — src/clawsweeper-label-mutations.ts:303-307
    The newly canonical identity is still passed to ghObservedMutationCommand, whose apply runner records an attempt and then calls the GitHub operation. The ledger documents that this identity does not authorize duplicate prevention, so the reported duplicate edit can still occur while merging would close The issue_labels_sync idempotency key varies by runner locale #1136. Remove the closing relation or add and prove a pre-execution gate. This is the prior blocker, still unfixed.
    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 220ad5673ace.

Labels

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The changed production owner is the batch-to-identity path in src/clawsweeper-label-mutations.ts; the body claims a Crabbox after-fix trace, but its command depends on wrapper scripts absent from the current head and records a different base than the pinned PR base. The current executable proof is a useful controlled recording probe, but the supplied body does not establish a current-head real-behavior result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove status: ⏳ waiting on author: Current PR status label is status: 📣 needs proof.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.

Label justifications:

  • P2: This is a bounded label-automation correctness problem with no evidence of a current emergency.
  • merge-risk: 🚨 automation: The merge would automatically close the linked automation defect although the execution path still has no duplicate-prevention gate.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The changed production owner is the batch-to-identity path in src/clawsweeper-label-mutations.ts; the body claims a Crabbox after-fix trace, but its command depends on wrapper scripts absent from the current head and records a different base than the pinned PR base. The current executable proof is a useful controlled recording probe, but the supplied body does not establish a current-head real-behavior result. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • Martin Cleary: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Remove the closing relation or add an apply-boundary duplicate gate with a repeated-operation regression.
  • Run the current run-proof.mjs path in its documented Node 24 environment and update the PR body with the exact head, pinned base, command, and redacted result.

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 (8 earlier review cycles)
  • reviewed 2026-08-12T11:49:33.862Z sha 6220d87 :: needs real behavior proof before merge. :: [P1] Deduplicate before invoking GitHub
  • reviewed 2026-08-12T12:19:28.853Z sha 1630b5c :: needs real behavior proof before merge. :: [P1] Gate the GitHub edit on the idempotency identity
  • reviewed 2026-08-12T13:03:15.158Z sha f6071b7 :: needs real behavior proof before merge. :: [P1] Gate the GitHub edit before closing the duplicate-prevention issue
  • reviewed 2026-08-12T23:32:11.187Z sha f6071b7 :: found issues before merge. :: [P1] Reconcile the closing claim with the actual enforcement boundary
  • reviewed 2026-08-18T09:31:03.668Z sha f6071b7 :: found issues before merge. :: [P1] Align the closing claim with the enforcement boundary
  • reviewed 2026-08-19T06:12:02.873Z sha 2518cf2 :: found issues before merge. :: [P1] Align the duplicate-edit claim with the enforcement boundary
  • reviewed 2026-08-25T13:16:40.058Z sha 2518cf2 :: found issues before merge. :: [P1] Correct the duplicate-edit claim
  • reviewed 2026-09-01T04:24:28.562Z sha 2518cf2 :: found issues before merge. :: [P1] Correct the duplicate-prevention claim

@dwin-gharibi

Copy link
Copy Markdown
Contributor Author

@steipete

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Aug 12, 2026
@steipete

Copy link
Copy Markdown
Contributor

@clawsweeper re-review

@clawsweeper

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

Re-review progress:

dwin-gharibi and others added 3 commits August 19, 2026 09:34
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>
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. proof: sufficient Contributor real behavior proof is sufficient. labels Sep 1, 2026
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. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The issue_labels_sync idempotency key varies by runner locale

3 participants