VOIP-1270-case-peer-address-contact-claim - #1139
Merged
Merged
Conversation
- docs: add DESIGN.md for Case Peer address auto-claim on Contact assign - Documents recon finding: single call path (console only), no Flow automation path exists; simplifies away route-based branching - Defines conditional auto-claim logic, target query filter addition, detach rollback policy (asymmetric, no rollback), and frontend post-hoc notification direction
- docs: close design review loop (4 rounds, 2x consecutive APPROVE) - Drops invented AddressGetByTarget dbhandler method, reuses AddressList with the new target filter instead (dogfoods the API change) - Clarifies auto-claim only runs on the attach branch, not detach - Notes reuse of existing AddressClaimTx race/tombstone-repair logic - Documents pre-existing (not new) case-level concurrency gap as out-of-scope - Disclosed: delegate_task unavailable this session, review rounds are self-conducted per missing-tool-workflow-substitution skill
- DESIGN.md: Replace auto-claim + post-hoc-toast direction with explicit modal confirmation + remember-decision UX per new direction - DESIGN.md: Add scenario split (new address / already-owned / owned-by-other-contact) with modal A/B copy - DESIGN.md: Design new backend release/reassign contact_addresses endpoints and dbhandler tx pattern (mirrors existing AddressClaim) - DESIGN.md: Document localStorage-based remember-decision storage with separate keys per scenario - DESIGN.md: Define square-admin frontend integration points in CaseContactAttributionPanel/handleAttach
- docs: rewrite DESIGN.md section 4 (release/reassign API design) to integrate the contact_address_ownership_periods subsystem instead of updating contact_addresses.contact_id in isolation - docs: AddressReleaseTx now closes the owning period via closeOwnOpenPeriodTx before nulling contact_id, replacing the previously imagined standalone post-lock reconfirmation - docs: AddressReassignTx closes the from-contact period then reopens via OwnershipPeriodsLockAndResolveTx/applyOpenResolutionTx for the new contact in one transaction, with a single final contact_id UPDATE instead of two - docs: rewrite section 4.5 race condition policy to reflect the actual AddressClaim retry-on-ErrStaleTarget pattern and explain why release/reassign instead fail fast on ErrConflict - docs: align section 5 API summary wording with the revised section 4
- docs: fix TOCTOU gap found in Round 2 design review — closeOwnOpenPeriodTx performs no owner verification in the skew (empty lockedRows) branch, so AddressReleaseTx/AddressReassignTx's final contact_id write needs its own WHERE contact_id = <expected> + RowsAffected safety net, mirroring AddressDeleteTx's existing B5-fix pattern
- docs: fix error-type ambiguity found in Round 3 design review — addressSetContactIDTx's skew-case safety net must return ErrConflict (not ErrStaleTarget) so it is NOT auto-retried by AddressClaimTx's existing ErrDeadlock||ErrStaleTarget retry loop, which would have silently reintroduced the takeover race §4.5 exists to prevent - docs: add addressSetContactIDTx to §4.1 as an explicitly new helper (not a reuse of the ownership-period subsystem) to resolve the 'never invent new logic' vs 'calls an undocumented function' contradiction Round 3 flagged - docs: §4.5 now enumerates all 3 ErrConflict emission points and states the release/reassign retry loop must check ErrDeadlock only, not the ErrDeadlock||ErrStaleTarget pair AddressClaimTx uses
- docs: fix Round 4 finding — §2 scenario 3 wording read as a 2-step frontend call (release then claim) despite §4.4 explicitly rejecting that pattern in favor of a single atomic reassign endpoint; clarified wording to match §4.4/§6.1 - docs: add missing test case for §4.5's error-emission-point #2 (OwnershipPeriodsLockAndResolveTx conflict on the new_contact_id side) to §8's AddressReassignTx test list
- docs: fix Round 5 finding — §4.3/§4.4 pseudocode declared addrType as plain string, but the existing helpers it reuses (closeOwnOpenPeriodTx, OwnershipPeriodsLockAndResolveTx) require commonaddress.Type; as written this would not compile against the real function signatures
- docs: fix Round 6 finding — §4.1 declared addressSetContactIDTx's expectedCurrentContactID param as *uuid.UUID, but §4.3/§4.4's actual call sites pass a plain uuid.UUID value with no pointer conversion; fixed the signature to match the call sites (value type, since both callers always have a confirmed non-nil expected owner) rather than changing the call sites
- docs: close design review loop (8 rounds; R1-R6 CHANGES_REQUESTED with real defects each round, R7-R8 consecutive APPROVE) - docs: address Round 8's two non-fatal gaps directly in this commit — document release/reassign reusing contacthandler's existing EventTypeContactUpdated audit event (mirrors ClaimAddress), and the modal-B tombstoned-owner 404 fallback UX note - docs: update status header to APPROVED FOR IMPLEMENTATION, add §10 review loop record table (round-by-round verdict, finding, commit)
- docs: v6 — supersede v5's release/reassign POST endpoints per
대표님's direction to simplify. Fold contact_id reassignment into
the existing PUT /contact_addresses/{id} (mirrors contact_cases/{id}
PUT convention: empty string clears, value assigns)
- docs: keep POST /contact_addresses/{id}/claim as-is — confirmed it
is already used in production by contacts_detail.js's Unresolved
Address Picker feature (2026-07-02 design), out of scope to touch
- docs: no conflict validation (last-write-wins), matching
contact_cases.contact_id's existing PUT policy
- docs: explicitly document the resulting asymmetry -- this PUT does
not update contact_address_ownership_periods the way claim/delete/
update do, and flag it as an accepted tradeoff pending future review
- docs: v5's 8-round review record preserved in §10 (marked SUPERSEDED,
no longer applicable code path); §11 marks v6 as DRAFT pending its
own review loop
- docs: v7 — supersede v6's PUT contact_id field. Instead extend the
existing POST /contact_addresses/{id}/claim with an optional
force parameter that lets it overwrite a live owner's claim
without a 409, reusing the exact orphan-close code path
(OwnershipPeriodsLockAndResolveTx Step 1) that already exists for
tombstoned owners
- docs: default behavior (force absent/false) is byte-for-byte
unchanged -- backward compatible with existing callers including
contacts_detail.js's Unresolved Address Picker
- docs: unlike v6, this closes the ownership-period history
asymmetry entirely since force still routes through
AddressClaimTx's existing history-writing path
- docs: flag the shared-helper blast radius risk explicitly --
OwnershipPeriodsLockAndResolveTx is used by
AddressCreateTx/AddressUpdateTx/AddressDeleteTx too, so the force
param threading must be verified not to leak into those paths
- docs: drop unclaim from scope -- no UI trigger exists in the
VOIP-1270 Case assignment screen (confirmed with 대표님)
- docs: v5/v6 review records preserved in §10/§4 SUPERSEDED notices;
§11 marks v7 as DRAFT pending its own review loop
- docs: R1 fix -- correct the force extension point. AddressClaimTx's internal OwnershipPeriodsLockAndResolveTx is unreachable for a live owner; addressClaimAttempt (address.go:226-244) already short-circuits to ErrConflict before AddressClaimTx is ever called. Rewire force through ClaimAddress -> AddressClaim -> addressClaimAttempt instead, reusing the existing staleRowRepairTx reset path that the tombstone branch already takes - docs: correct §4.3's blast-radius note -- OwnershipPeriodsLockAndResolveTx is untouched entirely, so no shared-helper risk with AddressCreateTx/AddressUpdateTx/AddressDeleteTx exists in this design - docs: update §8 test plan and §11 review log to match
- docs: R2 fix -- staleRowRepairTx is a no-op for live owners by design (tombstone-only repair helper), so reusing it for the force:true case left the prior owner's ownership period open, and AddressClaimTx's own unconditional call to OwnershipPeriodsLockAndResolveTx re-detected the same live conflict and re-raised ErrConflict -- R1's fix didn't actually fix the symptom - docs: replace with closeOwnOpenPeriodTx (the same CLOSE helper v5 already validated for release/reassign), called with the prior owner's contactID to close their open period before AddressClaimTx runs; separate the tombstone branch (staleRowRepairTx, unchanged) from the force branch (closeOwnOpenPeriodTx) explicitly instead of merging them into one reset path - docs: add the missing listenhandler wiring point to §4.3's change list (was already referenced in §8 tests but omitted from scope)
- docs: R3 fix -- §4.3's core pseudocode (closeOwnOpenPeriodTx) was verified correct by Round 3's code trace, but §4.2 and §8 still carried R1-era staleRowRepairTx wording that contradicted §4.3's already-corrected approach. Align both sections to the closeOwnOpenPeriodTx-based design and clarify the tombstone branch is untouched/separate from the force branch
- docs: R4 fix -- the v6-to-v7 rewrite accidentally deleted the entire '§5. API/OpenAPI Change Summary' header, silently dropping the target query-filter spec for GET /contact_addresses along with it (release/reassign items in old §5 were correctly dropped, but the independent target-filter prerequisite was not). §6.1's frontend flow cannot resolve its 3-way branch without this filter, and the live code confirms it still doesn't exist (AddressList only supports contact_id/type/unresolved today) - docs: restore the filter spec as new §4.6 (openapi/main.yaml, dbhandler.AddressList, listenhandler parsing -- same content v5's SUPERSEDED §10 already had), fix the two dangling §5 references in §6.1/§8 to point at §4.6
- docs: record R5 APPROVED in the review log
- docs: R6 APPROVED -- final review loop round, consecutive 2x APPROVE achieved (R5+R6), exceeds min-3-round floor (6 rounds) - docs: incorporate R6's two non-blocking observations -- (1) force overwrite doesn't emit an audit event for the contact losing ownership (§4.4, added to §9 out-of-scope), (2) Modal B's owner display_name lookup can 404 against a tombstoned prior owner since AddressList doesn't filter by contact liveness, unlike claim's own staleRowRepairTx handling (§2, fallback text noted for implementation) - docs: update Status header to APPROVED FOR IMPLEMENTATION
Implement DESIGN.md v7 §4: extend POST contact_addresses/{id}/claim
with an optional force parameter so square-admin can reassign a
Case's Peer address to a new Contact without a separate release
endpoint, plus a target query filter needed for the frontend's
3-way scenario lookup.
- bin-openapi-manager: add force (boolean, optional) to
id_claim.yaml requestBody with a description documenting the
backward-compatible default and force:true semantics; add target
(string, optional) query param to main.yaml's GET
- bin-contact-manager: addressClaimAttempt now branches on force --
tombstoned owner keeps the existing staleRowRepairTx repair-in-place
path unchanged; live owner + force:true closes the prior owner's
open ownership period via the existing closeOwnOpenPeriodTx helper
before AddressClaimTx runs (OwnershipPeriodsLockAndResolveTx itself
is untouched, per design's explicit warning that it's a shared
create/update/delete helper); live owner + !force keeps returning
ErrConflict
- bin-contact-manager: AddressList gains a target filter (same
sq.Eq pattern as the existing type filter)
- bin-contact-manager: AddressClaim/addressClaimAttempt,
ClaimAddress (contacthandler), and the claim listenhandler/request
struct all thread force through end to end; GET listenhandler
parses the new target query param
- bin-contact-manager: new address_claim_force_test.go covers
force=false baseline (unresolved success, live-owner conflict),
force=true unresolved no-op, force=true live-owner reassignment
with DB-level ownership-period verification (prior period closed,
new period open), force+tombstone parity, and the target filter
- bin-common-handler: ContactV1ContactAddressClaim takes force and
forwards it in the request body; ContactV1ContactAddressList
forwards a target filter
- bin-api-manager: ContactAddressClaim (admin/manager path) takes
force end to end from the generated OpenAPI body through to the
RPC call; ServiceAgentContactAddressClaim (agent-facing, out of
this ticket's scope per DESIGN.md) always passes force=false since
its OpenAPI body has no force field
- mocks regenerated across bin-contact-manager, bin-common-handler,
bin-api-manager; go mod tidy/vendor clean, golangci-lint 0 issues
in bin-openapi-manager, bin-common-handler, bin-contact-manager,
bin-api-manager
Allow claiming web_session-typed addresses onto contact_addresses without ever exposing them on the public Contact.Addresses field -- a webchat-originated Case's Peer.Type can be web_session, and the prior tel/email-only write whitelist made §6's scenario-1 (new address) create-address call fail with ADDRESS_TYPE_INVALID for those Cases (대표님 지시, post-approval follow-up documented in DESIGN.md §4.7). - bin-contact-manager: add contact.AddressTypeWebSession (commonaddress.TypeWebSession) to the write whitelist (isValidContactAddressType), covering all three write entry points (Create/AddAddress/UpdateAddress) that already share it; deliberately NOT added to contact.ReachableAddressTypes, whose own comment already documents this exact writable-but-not-reachable split as an intentional, allowed divergence. AddressListByContactID (which populates the public Contact.Addresses field) filters strictly by ReachableAddressTypes, so exposure is blocked with no new filtering logic needed - bin-contact-manager: new Test_IsValidContactAddressType_WebSession_WritableButNotReachable pins both directions -- writable, and absent from ReachableAddressTypes - bin-openapi-manager: add web_session to contact_addresses/main.yaml's GET type filter enum and POST body type enum; add web_session to openapi.yaml's CommonAddress.type enum (ContactManagerAddress's response schema inherits it) -- other pre-existing gaps in that enum (webchat, ai, ai_team, etc.) are unrelated to this change and left untouched - go generate re-run in bin-openapi-manager and bin-api-manager to regenerate gens/; go mod tidy/vendor clean, golangci-lint 0 issues in bin-openapi-manager, bin-contact-manager, bin-api-manager
Round 1 review nit fixes for the web_session commit (5e11c67): - bin-contact-manager: fix AddAddress's ADDRESS_TYPE_INVALID error message, which still said "must be tel or email" after web_session became a valid write type -- misleading to a caller hitting the error for an actually-invalid type - bin-contact-manager: strengthen Test_AddressListByContactID's read-side exclusion check with a real web_session row (in addition to the pre-existing hypothetical "webchat" row), closing the gap where the exact type real write traffic can now produce had no direct read-side regression coverage
Fix a real bug reported by 대표님: after assigning a webchat-based Case
to a Contact and confirming Modal A, GET /contact_interactions?contact_id=
returned an empty result even though the web_session address had been
successfully claimed onto the contact (visible in contact_addresses).
Root cause: bin-contact-manager: InteractionList's contact_id branch
read c.Addresses (populated by ContactGet via AddressListByContactID,
which filters to contact.ReachableAddressTypes -- tel/email only,
VOIP-1270 §4.7) to build its peer_events search filter. This was
already a documented anti-pattern -- AddressListByContactID's own doc
comment explicitly says it must not be reused for this
interaction-matching read path -- but interaction_read.go violated it.
The bug predates VOIP-1270 (it silently affected every contact_id-based
interaction lookup), but VOIP-1270's write/read-whitelist split for
web_session is what made it visible: web_session addresses can now be
legitimately claimed onto a contact, yet were invisible to the very
lookup this feature's UX copy promises ("you can view its activity
history under Interactions").
- bin-contact-manager: add AddressListAllByContactID (unfiltered by
ReachableAddressTypes) alongside the existing
AddressListByContactID (still used for the public Contact.Addresses
API field, unchanged)
- bin-contact-manager: InteractionList's contact_id branch now calls
AddressListAllByContactID instead of reading c.Addresses
- bin-contact-manager: update/extend the two affected tests
(Test_InteractionList_ByContact, Test_InteractionList_ByContact_NoAddresses)
to mock AddressListAllByContactID; extended the by-contact test with
a web_session row to directly guard against this regression --
mutation-tested by reverting interaction_read.go and confirming
exactly these 2 tests fail
- go generate (mockgen) run for the new interface method; go build,
go test, go mod tidy/vendor, golangci-lint all clean across
bin-contact-manager (0 issues)
Note: the companion square-admin English-only UI fix (Modal A/B Korean
copy) lives in monorepo-javascript PR #394, not in this commit -- kept
out of this repo's history since it touches a separate repository.
pchero
force-pushed
the
VOIP-1270-case-peer-address-contact-claim
branch
from
July 26, 2026 00:51
bdb4ad9 to
3d9787b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the existing claim endpoint with an optional force parameter so square-admin's Case-to-Contact assignment flow can reassign a Peer address away from a currently-live Contact without a separate release/reassign API surface, plus a target query filter the frontend needs to resolve its 3-way address scenario. Design doc:
.worktrees/VOIP-1270-case-peer-address-contact-claim/DESIGN.md(v7, 6 independent review rounds, APPROVED FOR IMPLEMENTATION).