e2e/billinginvoices_v3_test.go (2)

887-914: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Minor: TaxId isn't preserved by the supplier mapping.

Given the documented "replace, not merge" semantics for UpdateSupplier, omitting TaxId here would silently clear it if the supplier ever has one set. Currently harmless since the test profile's supplier has no tax ID, but worth a note for future-proofing if this helper gets reused with a supplier that has tax data.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/billinginvoices_v3_test.go` around lines 887 - 914, The supplier
round-trip helper in updateSupplierFromInvoiceParty drops TaxId, which can
unintentionally clear an existing tax ID because UpdateSupplier replaces the
full object. Update updateSupplierFromInvoiceParty to copy the invoice party’s
TaxId into the returned apiv3.UpdateSupplier alongside Id and Name, and keep the
address mapping unchanged.

---

719-745: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Beef up the success-path assertions to cover the actual mutable fields.

The "Should update the standard invoice via v3 PUT" case only asserts Id and Description on the response (lines 743-744), even though the request also carries Customer, Supplier, and an always-empty Workflow: apiv3.UpdateInvoiceWorkflowSettings{}. Since workflow/supplier/customer merging is a core part of what this PR ships, a regression in mergeStandardInvoiceFromAPI's handling of those fields (e.g., accidentally resetting workflow settings when the request submits a zero-value struct) wouldn't be caught by this test. Consider asserting that the returned invoice's supplier/customer match what was sent, and that workflow settings come back as expected (even if that's "reset to defaults" — pin down the expected value).

As per path instructions, "Make sure the tests are comprehensive and cover the changes." — supplier/customer/workflow merging is one of the changes this test is meant to exercise.

✅ Example additional assertions

 		stdInv, err := updatedInv.AsBillingInvoiceStandard()
 		require.NoError(t, err)
 		assert.Equal(t, invoice.Id, stdInv.Id)
 		assert.Equal(t, newDescription, lo.FromPtr(stdInv.Description))
+		assert.Equal(t, *invoice.Customer.Name, stdInv.Customer.Name)
+		assert.Equal(t, updateSupplierFromInvoiceParty(invoice.Supplier).Name, stdInv.Supplier.Name)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/billinginvoices_v3_test.go` around lines 719 - 745, The v3 PUT
success-path test in the billing invoice update case only checks Id and
Description, so it won’t catch regressions in mutable fields handled by
mergeStandardInvoiceFromAPI. Extend the assertions in the “Should update the
standard invoice via v3 PUT” test to verify the returned standard invoice’s
Customer, Supplier, and WorkflowSettings match the request/expected merged
state, using the existing updateReq, updatedInv, and AsBillingInvoiceStandard
symbols to locate the assertions.

api/spec/packages/aip/src/shared/request.tsp (1)

34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Clarify UpdateRequestNested's full-object nested-replace semantics.

Unlike UpdateRequest<T> (which wraps fields in OptionalProperties), UpdateRequestNested<T> does not — so nested sub-object fields with defaults (e.g. auto_advance, draft_period on invoicing settings) become required in the generated update payload. That means updating a nested section like workflow requires supplying the whole sub-object, not a partial patch — a meaningful difference from the top-level UpdateRequest pattern that isn't documented anywhere on this model.

Worth a short docstring note explaining this full-replace-of-nested-section behavior so future readers don't assume partial-patch semantics apply here too.

As per coding guidelines: "Add docstrings to domain helpers when the name compresses important business semantics, and describe the observable business contract rather than implementation mechanics."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/spec/packages/aip/src/shared/request.tsp` around lines 34 - 42, Add a
short docstring note on UpdateRequestNested<T> clarifying that it is a
full-replace update shape for nested sections, not a partial patch like
UpdateRequest<T>. Explain that because it omits OptionalProperties, callers
updating a nested object (for example via workflow) must supply the complete
sub-object including any defaulted fields such as auto_advance and draft_period.
Keep the note alongside the UpdateRequestNested model and reference its
relationship to FilterVisibility/DefaultKeyVisibility so readers understand the
contract.

Source: Coding guidelines

api/spec/packages/aip-client-javascript/src/models/schemas.ts (2)

1697-1702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Generic updateResourceReference carries a hardcoded "TaxCode reference." description.

Given the stack outline calls this out as a reusable ResourceReference visibility helper, baking a TaxCode-specific description into the shared generated type is misleading if it's ever reused for a different resource kind (feature key, etc.). Worth checking resource.tsp/request.tsp (not in this batch) for whether each instantiation should carry its own contextual doc via the property, not the shared model.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/spec/packages/aip-client-javascript/src/models/schemas.ts` around lines
1697 - 1702, The shared updateResourceReference schema is incorrectly hardcoded
with a TaxCode-specific description, which makes this reusable helper
misleading. Update the schema in updateResourceReference so it uses a generic
description or no description at all, and move any resource-specific wording to
the instantiating property/documentation in resource.tsp or request.tsp where
the reference is actually used.

---

4144-4145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

array / array_2 are pretty opaque export names.

export const array = z.array(updatePriceTier) and later array_2 = z.array(updateInvoiceLine) (line 5554) are exported public symbols with no descriptive naming — likely a symptom of the TypeSpec models lacking @friendlyName on these anonymous array properties. Not a functional bug, but it's rough for anyone consuming the generated SDK directly and risks future name collisions as more anonymous arrays get generated.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/spec/packages/aip-client-javascript/src/models/schemas.ts` around lines
4144 - 4145, The generated public export name is opaque here, so update the
schema generation around the anonymous array model used by array and array_2 to
produce descriptive symbols instead of generic names. Add or propagate a
friendly name for the underlying TypeSpec array properties so the exported
z.array(...) constants in schemas.ts get stable, meaningful identifiers, and
make the same adjustment for both the updatePriceTier and updateInvoiceLine
cases to avoid future collisions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/set/set.go (1)

36-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Use RLock instead of Lock for a read-only check.

Has only reads the map, so grabbing the write lock is heavier than it needs to be — unnecessarily blocks concurrent readers. IsEmpty right below already does this correctly with RLock/RUnlock, so let's stay consistent.

⚡ Proposed fix

 func (s *Set[T]) Has(item T) bool {
-	s.mu.Lock()
-	defer s.mu.Unlock()
+	s.mu.RLock()
+	defer s.mu.RUnlock()

 	_, exists := s.content[item]
 	return exists
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/set/set.go` around lines 36 - 43, The Has method on Set[T] is performing
a read-only map lookup while taking the exclusive lock, which blocks other
readers unnecessarily. Update Has to use the read lock path instead, matching
the IsEmpty method’s RLock/RUnlock pattern, and keep the existing
mutex-protected access to s.content so concurrent reads remain safe.

Source: Path instructions