fix(owner-sync): backfill empty owner name on next sync instead of re…#14
Conversation
…r ids
Bug: syncSelf returns early when the owner is unchanged (`coreOwnerId ===
localOwnerId`), before the owner-name fetch. Owner-name fetch failures/timeouts
are silently swallowed (cosmetic — must not block the owner bind), so the owner
gets bound with an empty name. Every subsequent periodic sync then short-circuits
at that check and skips the name fetch, so the empty name never backfills (until
the owner member_id itself changes). Display-only impact — access control keys on
member_id, which stays correct.
Fix (mirrors claude-openmax PR #16):
- Short-circuit only when id matches AND the local name is already non-empty;
otherwise fall through to re-fetch/backfill the name.
- Only write when something actually changed (new owner id, or a non-empty fetched
name differing from local) — if core still returns no name for an already-bound
owner, skip the write so a steady state does not re-persist an empty name every tick.
- encodeURIComponent both member ids in the /members/{id} paths (hardening).
Note: claude-openmax / zylos-openmax share this owner-sync lineage; same fix landed
on claude-openmax PR #16 (41ae89c6).
Test: two-phase backfill regression (name fetch fails -> empty name -> next sync
backfills) + no-redundant-persist (id matches, core still nameless -> no write).
All 175 tests pass + tsc build clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7f0795b to
f63738b
Compare
gavin09527
left a comment
There was a problem hiding this comment.
LGTM — approved (on commit f63738b).
The empty-owner-name backfill fix is correct: it short-circuits only when the owner id matches AND the local name is already non-empty, so a name left empty by a failed/timed-out owner-name fetch backfills on a later sync. The change-gate (write only on a new owner id, or a non-empty fetched name that differs from local) prevents re-persisting an empty name on every periodic tick, and encodeURIComponent now guards both /members/{id} paths — matching claude-openmax PR#16. Pull-not-trust semantics unchanged; display-only (access control keys on member_id). Tests cover both the two-phase backfill and the no-redundant-persist case.
Non-blocking nit: the two new test it() descriptions are still in Chinese if you want fully-English PR content.
…turning early
Bug: syncSelf returns early when the owner is unchanged (
coreOwnerId === localOwnerId), before the owner-name fetch. Owner-name fetch failures/timeouts are silently swallowed (cosmetic — must not block the owner bind), so the owner gets bound with an empty name. Every subsequent periodic sync then short-circuits at that check and skips the name fetch, so the empty name never backfills (until the owner member_id itself changes). Display-only impact — access control keys on member_id, which stays correct.Fix: add "local name is non-empty" to the short-circuit guard —
coreOwnerId === localOwnerId && orgConfig.owner?.name. When the owner is unchanged but the stored name is empty, fall through to re-fetch/backfill the name; when the name is already present, short-circuit as before. Fail-open semantics unchanged.Note: claude-openmax / zylos-openmax share this owner-sync lineage; the same finding is flagged on claude-openmax PR #16.
Test: added a two-phase regression (owner-name fetch fails first -> empty name bound -> next sync backfills the name). All 174 tests pass + tsc build clean.
What
Why
Test plan
npm testpasses locallynpm run buildpasses locallyChecklist
// nosemgrep: <rule-id>suppression is justified inline and approved by the project lead