fix(labels): make the label sync identity locale independent - #1139
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed September 1, 2026, 2:19 AM ET / 06:19 UTC. ClawSweeper reviewWhat this changesThe PR makes batched label-sync identities deterministic across runner locales and queue order, with regression tests and a recorder-backed proof executable. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessThe PR remains necessary: current main still uses locale-sensitive sorting, while this patch replaces it with the repository’s documented total ordering and supplies current-head recorder proof. The earlier duplicate-execution claim has been explicitly scoped away, so no blocking correctness finding remains. Priority: P2 Review scores
Verification
How this fits togetherClawSweeper groups desired issue-label changes into one GitHub mutation and records its identity in the action ledger. Deterministic ordering ensures equivalent label sets produce the same recorded business key before the GitHub adapter is invoked. flowchart LR
A[Review decisions] --> B[Label batch]
B --> C[Canonical code-unit ordering]
C --> D[Label-sync identity]
D --> E[Action ledger receipt]
C --> F[GitHub label mutation]
Before merge
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. Technical reviewBest possible solution: Land the narrow canonical-ordering repair with its current recorder proof, leaving any duplicate-execution policy work to the separate canonical issue. Do we have a high-confidence way to reproduce the issue? Yes. Current main still uses default-locale sorting, and the committed probe defines a concrete locale and collator-tie reproduction path with supplied current-head after-fix observations. Is this the best way to solve the issue? Yes. Reusing the existing strict code-unit comparator at the identity-construction boundary is the smallest maintainable repair and preserves the existing label-set semantics. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 57b4b4496721. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (9 earlier review cycles; latest 8 shown)
|
|
@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>
Related: issue 1136.
Problem and Scope
For a given target and the same effective addition/removal label values,
flushIssueLabelMutationBatchcould emit differentissue_labels_syncidentities on different runners.localeComparefollows the runner's ICU locale and can also return zero for distinct strings, leaving collator-tied labels in queue order. Both differences reach the recorded ledger business key.Use the existing
compareCodeUnitsfor both surviving value lists. Normalized map keys, last-operation-wins handling, exact surviving label spellings, required/optional handling, and freshness/receipt rules remain intact. Argument-list and optional fallback order intentionally become code-unit order; the existing order-sensitive assertions were updated without removing their checks.Scope correction: this is canonical recording, not duplicate-edit prevention. The apply runner records an attempt and, after its normal guards, invokes the operation; it does not consult the business identity to suppress execution. No execution-deduplication gate, legacy-key migration, or general collision-free label encoding is added. UTF-16 ordering of label values is separate from the ledger's UTF-8 ordering of canonical object keys. The issue reference above is not an automatic-closing claim about duplicate publication.
Simplification and Ownership
The original contribution by @dwin-gharibi is preserved in branch ancestry and co-author credit. The updated proposal is 174 lines smaller, with four files instead of six. Two feature-specific installer/compiler-repair and baseline-staging wrappers are gone. One bounded producer-to-persisted-recorder executable replaces the duplicated proof suite and embedded locale program; the focused test invokes that same executable. Runtime/dependency provisioning stays with the repository's normal manifest/build tooling.
Final diff against integrated main
220ad5673ace96b2fab6473203796639c17ac710:The complete repair still adds regression/proof coverage; it is not presented as a net-negative repository change. No dependency, workflow, configuration, guard, threshold, or changelog change is introduced. OpenClaw Bay is unaffected: no observer API/schema, status, telemetry, lifecycle or control contract changes.
Real Behavior Proof
Published head:
7aa1c8ed0a0d1788ab600d31d49440266d08765fTree:
399cce4b4e046b15fac8f7dc07bd89b7354295c4Surface: the committed probe invokes the real label-batch producer and real apply-ledger recorder, then reads actual spooled attempt/outcome events. Its adapter is synthetic; it does not call GitHub or substitute for a full apply-workflow execution. Reproduction and limits remain runnable from a normal build, without task-local files or a custom installer.
Scenarios and controls: two separate Node children assert their actual en-US/sv-SE locales and successful exits. Each exercises nine cases: normal, reversed, default-sorted and rotated queues; duplicate exact values; changed additions; changed removals; tied distinct labels on both sides; and reversed ties. Assertions cover exact values and arguments, golden identity, persisted business-key readback, different keys for changed effective sets, and distinct attempt/outcome IDs for repeated synthetic invocations. Each child reads 20 persisted events with 18 distinct mutation-receipt IDs from nine synthetic invocations. These counts are per child; they are not live GitHub effects.
Before/after contrast: a qualified local macOS build retained the actual label-owner module from main
220ad567; it was compared with the candidate using the same current recorder/helper modules, isolating the label-owner change rather than claiming execution of an entire older checkout. All four locale children ran on Node 24.20.0 / ICU 78.3. The old owner reproduced both locale divergence and collator-tie insertion-order divergence. The candidate produced identical observations across both locales and queue permutations.The normal-case identities were:
The corresponding persisted normal-case business keys were:
Fresh published-head AWS execution: run
run_25769b696ec4, 2026-09-01, leasecbx_1ca33d2ff4cf, Linux x86_64 imageami-0461d919be7deb53c,eu-west-1,c7a.8xlarge; Node 24.20.0, pnpm 11.10.0, ICU 78.3. The current candidate ran in both verified locales with the same golden identity/business key, nine cases, 20 persisted events and 18 distinct receipt IDs per child. The pre-fix contrast above was local; it is not relabeled as an AWS baseline run.The fresh lease had no instance role (IMDS credential endpoint 404), no Tailscale attachment, no hydration or normal workspace sync. Only CI was forwarded into the source wrapper. The exact public commit was fetched with depth 1 into fresh Git metadata; no local history, source overlay, credential or compiled baseline artifact was uploaded. The recorder children used their own canonical temporary roots and fixed synthetic producer metadata, with no inherited workflow/projection/credential context.
After checksum-verified Node bootstrap and pinned pnpm installation, the actual commands were:
Validation and Trace
The evidence was downloaded and verified, then the owned AWS lease was released; broker readback confirmed
released.The inline trace and immutable executable make the claim inspectable without relying solely on portal access. Earlier historical archives were not independently recoverable and are not used as current proof. No live GitHub mutation, model service, complete apply workflow, namespace containment, production deployment, execution deduplication or migration is claimed.