Repository navigation
docs: update Claude docs from PR review analysis - #804
claude[bot] wants to merge 1 commit into
Conversation
…10-05) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
![claude[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const { '@odata.context': _ctx, ...data } = response.data as Record<string, unknown>; | ||
| return pascalToCamelCaseKeys(data) as EntityGetResponse; | ||
| ``` | ||
| Reference implementation: `src/services/orchestrator/attachments/attachments.ts`. |
There was a problem hiding this comment.
The cited reference doesn't match this rule. attachments.ts never strips @odata.context in its getById method — only in create, and there it's done after all transforms (not before, as the code example shows). The actual implementation from PR #753 that prompted this rule is src/services/orchestrator/folders/folders.ts:
const camelCased = pascalToCamelCaseKeys(response.data) as FolderGetResponse & { '@odata.context'?: string };
const { '@odata.context': _odataContext, ...folder } = camelCased;
return folder;Note that folders.ts also strips after pascalToCamelCaseKeys (not before as the code example shows). Since @odata.context starts with @ and isn't touched by case conversion, the order is functionally equivalent — but the code example and reference should agree.
Suggest either:
- Change the reference to
src/services/orchestrator/folders/folders.ts, or - Update the code example to show the after-case-conversion pattern that both existing implementations use.
|
This branch has not been deployed
1 participant
Summary
Weekly analysis of PR comments (2026-09-28 -> 2026-10-05).
Analyzed 16 PRs with resolved review threads. Found 3 actionable insights.
Changes
agent_docs/conventions.md
Validate-before-mutate rule — Always run all guard checks before mutating any shared object. Mutating state before the guard fires means that when validation fails and throws, the mutation has already applied to any shared reference, corrupting objects reused by callers or test fixtures. Additionally, shallow copies via
{ ...obj }share nested object references — when a nested object needs to be mutated, create an explicit shallow copy of it.Source: PR feat(data-fabric): federated update deltas — replaceSourceJoins + updateExternalConnection #783 — reviewer caught that
isPrimarySourcewas written to every source'sexternalObjectDetailbefore the not-found check fired. Because{ ...s }is a shallow copy, the nestedexternalObjectDetailwas the same object reference as the one in the shared test fixturefederatedRaw, so failed validation was polluting subsequent tests. Resolution: validate first with.some(), then copy-on-write the nested object:{ ...detail, isPrimarySource }.@odata.contextmust be destructured out of OData responses — ODatagetByIdresponses include@odata.context(and sometimes other@odata.*keys) that must be destructured and discarded before returning. Documents the existing pattern already used insrc/services/orchestrator/attachments/attachments.ts.Source: PR feat(folders): add getByKey lookup on odata/Folders #753 — reviewer noted
@odata.contextwas leaking into theFolderGetResponsereturn value. The fix was to destructure it out before applyingpascalToCamelCaseKeys. This pattern is used by attachments but was undocumented.agent_docs/rules.md
mockApiClientmust be typed asReturnType<typeof createMockApiClient>, notany— Flagged independently across PRs feat(du-validation): add validation start and result poll service #752, feat(folders): add getByKey lookup on odata/Folders #753, and feat(agenthub): add chat completions via LLM gateway #754 as a recurring violation. Theanytype defeats type safety in tests and hides real type errors. The correct pattern islet mockApiClient: ReturnType<typeof createMockApiClient>;.No changes
CLAUDE.md— no relevant insights foundAgents.md— no relevant insights foundagent_docs/architecture.md— no relevant insights foundPRs Analyzed