Chore/pr 31 followup doc and fix sync#38
Merged
Conversation
Three doc/diagram updates that the PR #31 code changes made stale: 1. docs/code-flows/catalogservice.md — Flow 2 (PUT /products/{id}) Mermaid showed `Products.Include(Category).FirstOrDefaultAsync(...)` with a SELECT * FROM products JOIN categories. After PR #31 removed the unused .Include(p => p.Category), the actual query is `Products.FirstOrDefaultAsync(...)` with no JOIN. Updated the diagram line + added a Note explaining why the Include was removed. 2. docs/code-flows/catalogservice.md — Flow 3 (gRPC ReserveStock) The xmin-stale branch said "DbUpdateConcurrencyException bubbles up as gRPC Internal status — OrderService sees the call fail and aborts the order." That described the pre-fix behavior. After PR #31's try/catch around SaveChangesAsync, the handler now returns false and the gRPC response is `ReserveStockResponse { Success = false }` — the same shape as insufficient stock. OrderService aborts cleanly without surfacing a 500. Updated the Note + added the explicit return-path lines in the diagram. 3. docs/code-flows/orderservice.md — file inventory Updated the OrderEndpoints.cs and GetOrderById.cs entries to describe the new IDOR contract: endpoint extracts JWT NameIdentifier and passes it as RequestingBuyerId; handler's EF Where clause filters by both Id AND BuyerId (non-owner → null straight from the database → 404 per anti-enumeration). 4. CLAUDE.md "Authorization" rule (Security Requirements) The previous wording — "Handler loads the entity, then returns null on owner mismatch" — described the post-materialization in-memory check, which the current implementations don't use. Both ShippingService .GetShipmentByOrder and (after PR #31) OrderService.GetOrderById push the ownership predicate into the EF Where clause for tighter defense-in-depth. The rule now distinguishes: - Read handlers: predicate in SQL (preferred) - Write handlers: tracked load + in-memory check (required because they need the tracked entity to mutate) Reference templates list expanded with explicit file pairings + which pattern each demonstrates. 5. .claude/agents/architecture-reviewer.md — IDOR heuristic update Synced with the CLAUDE.md rule rewrite. A read handler whose ownership check is in C# rather than SQL is now flagged as Should-consider (structurally weaker), not Must-fix (still satisfies the external null → 404 contract). Reference templates updated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two diagrams had text from the repository-wrapper era that no longer matches
the code:
docs/hybridcache-flow.excalidraw
- "repo.UpdateAsync(product) → SaveChangesAsync"
→ "context.SaveChangesAsync() (xmin / RowVersion check fires here)"
- Factory body "product = await repo.GetByIdAsync(id, ct); return ...
new ProductDto { ... project ... }"
→ "return context.Products.AsNoTracking().Where(p => p.Id == id)
.Select(p => new ProductDto { ... }).FirstOrDefaultAsync(ct)"
- Source path "CatalogService.Application/Handlers/{GetProductById,
UpdateProduct,ReserveStock}Handler.cs"
→ "CatalogService/Features/{GetProductById,UpdateProduct,ReserveStock}.cs"
(after the catalog VSA collapse, those files moved out of the
Application/Handlers/ subdirectory of the deleted CatalogService.
Application project)
docs/service-request-flow.excalidraw
- Step label "Repository.UpdateAsync → DbContext.SaveChangesAsync
(entity write + outbox row)"
→ "context.SaveChangesAsync() (entity write + outbox row — atomic
via Wolverine)"
- Annotation "Repository implementation calls context.SaveChangesAsync()..."
→ "Handler calls context.SaveChangesAsync() directly (no repository
wrapper). EF generates UPDATE .. WHERE Id=@id AND RowVersion=@orig ..."
- Handler-step annotation "Constructor-injected dependencies (repositories,
IMessageBus, ICatalogClient for gRPC, ILogger)"
→ "DbContext directly (no IFooRepository wrapper — DbContext IS
Unit-of-Work, DbSet<T> IS Repository), IMessageBus, ICatalogClient
(gRPC port — substituted in tests), ILogger"
Both `text` (rendered, possibly word-wrapped) and `originalText` (the
authoritative source Excalidraw re-wraps from on open) fields were updated,
so the next person to open the .excalidraw file sees the new text rather
than the .excalidraw re-rendering back to the old original.
KNOWN DEFERRED: docs/hybridcache-flow.svg and docs/service-request-flow.svg
are the rendered exports inlined into README.md + docs/code-flows.md + docs/
code-flows/catalogservice.md, and they still contain the four stale strings
above. Excalidraw exports are not trivially scriptable from CLI (need a
browser/jsdom + canvas), so the SVGs trail until manually regenerated:
1. Open the .excalidraw file in VS Code (pomdtr.excalidraw-editor) or
excalidraw.com
2. File → Export → SVG (or the "Export image" button)
3. Overwrite the existing .svg next to the .excalidraw file
4. Commit
Not blocking — the rendered docs show slightly stale labels until then,
but the .excalidraw source-of-truth is correct.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…8.0 pin) Two related changes that together unblock SVG regeneration for the Excalidraw .excalidraw sources updated in c1a9e04: 1. .claude/skills/excalidraw-diagram/references/render_template.html Pinned the Excalidraw import from https://esm.sh/@excalidraw/excalidraw?bundle (resolves to latest = 0.18.1 today, which hangs during transitive module init in headless Chromium — `window.__moduleReady` never gets set, even at a 90s timeout, with no console errors or failed requests) to https://esm.sh/@excalidraw/excalidraw@0.18.0?bundle (resolves cleanly in <10s). 2. .claude/skills/excalidraw-diagram/references/render_excalidraw.py - Bumped the `wait_for_function("window.__moduleReady === true")` timeout from 30000 to 90000. Belt + suspenders alongside the pin — warm-cache resolves fast (~5s), cold-cache CDN fetches under load can stretch to ~60s, 90s is a comfortable ceiling. - Added page.on("console"), page.on("pageerror"), page.on("requestfailed") hooks routing to stderr. When the render ever hits a CDN issue again, the actual cause (CORS, 404, JS error, hung module init) will surface in the script output instead of the uninformative generic TimeoutError. 3. docs/hybridcache-flow.svg + docs/service-request-flow.svg Regenerated via .claude/scripts/rebuild-diagrams.sh after the pin landed. Verified: all four stale strings (repo.UpdateAsync, Repository.UpdateAsync, repo.GetByIdAsync, CatalogService.Application/Handlers) are gone; the new DbContext-direct shape (context.SaveChangesAsync, "DbContext directly", "no IFooRepository wrapper") is present. 4. .gitignore Added `docs/*.png` — the rebuild-diagrams.sh script generates both .svg (committed, inlined into README + docs/code-flows.md) and .png (local-only render side-effect — ~1-2 MB binaries that aren't referenced anywhere). The other Excalidraw sources in docs/ have always followed this convention; the rule just makes it explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
WalkthroughThis PR refines IDOR ownership-check patterns for read vs. write handlers, cascades the updated patterns through code-flow documentation and service diagrams, and hardens the Excalidraw rendering pipeline with improved timeouts, diagnostics, and dependency pinning. ChangesIDOR Ownership-Check Pattern and Code-Flow Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@CLAUDE.md`:
- Around line 41-44: Add the required audit flag to the CLAUDE.md update by
inserting the exact note "Run /check-rules locally to audit paraphrases against
this diff." adjacent to the changed policy text (the paragraph describing Read
handlers, Write handlers, 404 vs 403, and the reference templates like
OrderEndpoints.cs / Features/GetOrderById.cs, ShippingEndpoints.cs /
Features/GetShipmentByOrder.cs, CatalogEndpoints.cs / Features/UpdateProduct.cs)
so reviewers see the prompt to run /check-rules locally when this
paraphrase/content is changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c3d2abe7-3950-4c32-b5c2-c6ac009caac1
⛔ Files ignored due to path filters (2)
docs/hybridcache-flow.svgis excluded by!**/*.svgdocs/service-request-flow.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
.claude/agents/architecture-reviewer.md.claude/skills/excalidraw-diagram/references/render_excalidraw.py.claude/skills/excalidraw-diagram/references/render_template.html.gitignoreCLAUDE.mddocs/code-flows/catalogservice.mddocs/code-flows/orderservice.mddocs/hybridcache-flow.excalidrawdocs/service-request-flow.excalidraw
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates architecture/code-flow documentation and diagram rendering tooling to reflect recent refactors (direct DbContext usage, EF query shapes, and scoped-access security patterns), while making Excalidraw diagram exports more reliable.
Changes:
- Refreshes service-request and HybridCache diagrams/docs to match new handler/repository patterns and message/outbox semantics.
- Tightens documented IDOR guidance to prefer ownership predicates inside EF
Whereclauses for read handlers. - Stabilizes Excalidraw rendering by pinning the CDN module version and improving render-time diagnostics/timeouts.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/service-request-flow.excalidraw | Updates diagram text to describe direct DbContext usage and Wolverine outbox atomicity. |
| docs/hybridcache-flow.svg | Regenerated diagram SVG to reflect new CatalogService feature file locations and EF query shape. |
| docs/hybridcache-flow.excalidraw | Updates Excalidraw source for the HybridCache flow (source paths + factory/query/write steps). |
| docs/code-flows/orderservice.md | Clarifies endpoint/handler security contract (buyer-scoped predicate + 404 anti-enumeration behavior). |
| docs/code-flows/catalogservice.md | Updates sequence diagrams to remove unnecessary Include join and document concurrency exception handling behavior. |
| CLAUDE.md | Refines security guidance for scoped reads/writes with concrete EF predicate patterns and references. |
| .gitignore | Ignores locally-generated Excalidraw PNG renders under docs/. |
| .claude/skills/excalidraw-diagram/references/render_template.html | Pins Excalidraw ESM import to a known-good version to avoid headless hangs. |
| .claude/skills/excalidraw-diagram/references/render_excalidraw.py | Adds console/request failure logging and increases module-load wait timeout. |
| .claude/agents/architecture-reviewer.md | Aligns the reviewer checklist with the updated IDOR/ownership-predicate guidance. |
Comment on lines
+152
to
155
| page.on("requestfailed", lambda req: print(f" [request failed] {req.url} — {req.failure}", file=sys.stderr)) | ||
|
|
||
| # Load the template | ||
| page.goto(template_url) |
| # the browser pulls in many transitive modules afterwards; 30s was | ||
| # too tight on slower networks. 90s is a comfortable ceiling — if we | ||
| # ever hit that, the CDN is genuinely down rather than slow. | ||
| page.wait_for_function("window.__moduleReady === true", timeout=90000) |
Comment on lines
+1695
to
+1696
| "text": "Handler calls context.SaveChangesAsync() directly (no repository wrapper). EF generates UPDATE\n.. WHERE Id=@id AND RowVersion=@orig (SQL Server) or AND xmin=@orig (Postgres) — concurrency token in the WHERE clause. Outbox envelopes staged during the handler flush in the SAME transaction via Wolverine UseEntityFrameworkCoreTransactions.", | ||
| "originalText": "Handler calls context.SaveChangesAsync() directly (no repository wrapper). EF generates UPDATE\n.. WHERE Id=@id AND RowVersion=@orig (SQL Server) or AND xmin=@orig (Postgres) — concurrency token in the WHERE clause. Outbox envelopes staged during the handler flush in the SAME transaction via Wolverine UseEntityFrameworkCoreTransactions.", |
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
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.
Summary
PR #31 was squash-merged before these doc/tooling updates finished pushing. This PR brings
mainback in sync with the actual code that landed in PR #31. Zero code logic changes outside of.claude/skills/excalidraw-diagram/renderer tooling.What's in scope
1. Walkthroughs aligned with the code now on main
docs/code-flows/catalogservice.md— Flow 2 (PUT /products/{id}) Mermaid no longer showsProducts.Include(Category).FirstOrDefaultAsync(...)+ JOIN againstcategories. The handler stopped Includeing Category in PR Refactor/catalog vsa collapse #31 (it never read it). Added a Note explaining why the Include was removed. Flow 3 (gRPC ReserveStock) — the xmin-stale branch previously said "DbUpdateConcurrencyException bubbles up as gRPC Internal status." After PR Refactor/catalog vsa collapse #31's try/catch, the handler returnsfalseand the gRPC response isReserveStockResponse { Success = false }— same shape as insufficient stock. OrderService aborts cleanly without surfacing a 500. Diagram + Note updated.docs/code-flows/orderservice.md— File inventory describes the new IDOR contract: endpoint extracts JWTNameIdentifierand passes it asRequestingBuyerId; handler's EFWhereclause filters byId AND BuyerId(non-owner → null straight from the database → 404 per anti-enumeration).2. CLAUDE.md IDOR rule rewrite
The previous wording — "Handler loads the entity, then returns null on owner mismatch" — described the post-materialization in-memory check, which the current implementations don't use. Both
ShippingService.GetShipmentByOrderand (after PR #31)OrderService.GetOrderByIdpush the ownership predicate into the EFWhereclause for tighter defense-in-depth.The rule now distinguishes:
Reference templates list expanded with explicit file pairings.
.claude/agents/architecture-reviewer.mdIDOR heuristic synced — a read handler with C#-only ownership check is now Should-consider (still satisfies the null → 404 external contract, but structurally weaker; recommend tightening to the SQL-predicate shape).3. Excalidraw diagram sync
docs/hybridcache-flow.excalidraw— Three elements patched: the write-step (repo.UpdateAsync(product)→context.SaveChangesAsync()), the cache-miss factory body (product = await repo.GetByIdAsync(id, ct); return product is null ? null : new ProductDto { ... }→ inlinecontext.Products.AsNoTracking().Where(...).Select(...).FirstOrDefaultAsync(ct)), and the source-path footer (CatalogService.Application/Handlers/{...}Handler.cs→CatalogService/Features/{...}.cs).docs/service-request-flow.excalidraw— Three elements patched: the DB step (Repository.UpdateAsync → DbContext.SaveChangesAsync→context.SaveChangesAsync()), the annotation that referenced "Repository implementation calls context.SaveChangesAsync()..." → handler-direct wording, and the handler-step annotation listing "repositories" as a constructor-injected dependency → DbContext-direct.text(rendered) andoriginalText(Excalidraw's re-wrap source) fields updated for every change, so the diagrams won't revert on next open.4. Regenerated SVG renders
docs/hybridcache-flow.svganddocs/service-request-flow.svgregenerated via.claude/scripts/rebuild-diagrams.sh. Verified — none of the four stale strings (repo.UpdateAsync,Repository.UpdateAsync,repo.GetByIdAsync,CatalogService.Application/Handlers) remain; the new DbContext-direct text is present.5. Excalidraw renderer tooling unblock
The rebuild script had been failing with a generic
TimeoutErroronwindow.__moduleReady === true. Root cause: the template imported@excalidraw/excalidraw?bundlefrom esm.sh unpinned, which resolves to0.18.1today — and0.18.1's transitive module init hangs in headless Chromium with no console errors, no failed requests, no diagnostics.render_template.html— pinned to@excalidraw/excalidraw@0.18.0?bundle.0.18.0resolves cleanly in <10s.render_excalidraw.py— bumpedwait_for_functiontimeout from 30s to 90s as belt-and-suspenders, AND added three Playwright listeners (console,pageerror,requestfailed) routing to stderr. Next time the renderer hangs, the actual cause (CDN issue, JS error, hung module) will surface in the script output instead of a bare TimeoutError.6. .gitignore —
docs/*.pngThe rebuild script generates both
.svg(committed, inlined into README +docs/code-flows.md+docs/code-flows/catalogservice.md) and.png(1–2 MB local-only side-effects, not referenced anywhere). Other diagrams have always followed the SVG-only convention; this rule makes it explicit.What's NOT in scope
.claude/skills/and isn't part of the .NET build)The 4 code-correctness fixes from PR #31's CodeRabbit review (GetOrderById IDOR predicate in SQL, ReserveStock concurrency catch, SearchProducts empty-term guard, UpdateProduct unused Include removal) are already on main as part of the PR #31 squash.
Test plan
uv+ Playwright) Re-run.claude/scripts/rebuild-diagrams.sh --force docs/hybridcache-flow.excalidrawlocally to confirm the renderer no longer hangs and produces matching SVG output🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores