feat: add rule to limit stored procedures to single-partition atomic …#180
feat: add rule to limit stored procedures to single-partition atomic …#180ramnnn2006 wants to merge 3 commits into
Conversation
ramnnn2006
requested review from
TheovanKraay,
jaydestro and
sajeetharan
as code owners
ramnnn2006
force-pushed
the
feat/pattern-stored-procedure-scope
branch
from
c6cdb90 to
77d0c22
Compare
avinashkamat48
left a comment
avinashkamat48
left a comment
There was a problem hiding this comment.
The generated AGENTS.md section now has ### 9.15 ... followed immediately by another ## Use Stored Procedures Only... heading and a second Impact line. That duplicates the rule heading inside the numbered hierarchy and makes the AGENTS doc structure inconsistent with the TOC entry. Could you drop the standalone rule ## heading / duplicate impact when embedding this into AGENTS.md, similar to the surrounding numbered sections?
|
@avinashkamat48-design |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Cosmos DB best-practice rule to the cosmosdb-best-practices skill clarifying that stored procedures should be used only for multi-document atomic operations within a single partition, and otherwise prefer normal SDK calls or transactional batch. It also updates the compiled AGENTS.md output and adds an eval task to validate the behavior.
Changes:
- Added a new Design Patterns rule:
pattern-stored-procedure-scope.md. - Regenerated
skills/cosmosdb-best-practices/AGENTS.mdto include the new rule in the TOC and content. - Added a new eval task YAML to exercise the rule’s expected guidance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| skills/cosmosdb-best-practices/rules/pattern-stored-procedure-scope.md | New rule defining correct/incorrect stored procedure usage with examples and references. |
| skills/cosmosdb-best-practices/AGENTS.md | Compiled skill output updated to include the new rule. |
| evals/cosmosdb-best-practices/tasks/pattern-sproc-scope.yaml | New evaluation task to ensure the guidance is triggered appropriately. |
| function transfer(fromId, toId, amount) { | ||
| var collection = getContext().getCollection(); | ||
| collection.readDocument(documentLink(fromId), function (err, from) { | ||
| if (err) throw err; | ||
| if (from.balance < amount) throw new Error("Insufficient funds"); | ||
| from.balance -= amount; | ||
| collection.replaceDocument(from._self, from, function (err) { | ||
| if (err) throw err; // any failure rolls back the whole transfer | ||
| // ...read and credit the destination account next | ||
| }); | ||
| }); | ||
| } |
| function transfer(fromId, toId, amount) { | ||
| var collection = getContext().getCollection(); | ||
| collection.readDocument(documentLink(fromId), function (err, from) { | ||
| if (err) throw err; |
TheovanKraay
left a comment
TheovanKraay
left a comment
There was a problem hiding this comment.
Good rule, stored procedures are definitely over-recommended by agents. A few issues:
The "When a stored procedure is still the right tool" example calls documentLink(fromId) which isn't defined anywhere in the snippet. Copilot flagged this too. Please make it self-contained (e.g., query for the document or construct the link explicitly).
There's still a duplicate heading in the rule body. ## Use Stored Procedures Only... appears right after the compile.js-generated ### 9.15 heading. Please remove the ## heading from the source rule file and rebuild.
The transfer sproc example is incomplete (stops at // ...read and credit the destination account next). Since this is the example showing when a sproc is warranted, it should demonstrate the full pattern.
Also, we recently merged a skill split (#204) that added topic-specific skills alongside the monolith. We're in a transitional phase where both coexist while we evaluate whether agent routing can handle the split on its own. Until that's resolved, new rules need to live in both places. Since this is a pattern- prefixed rule, please copy skills/cosmosdb-best-practices/rules/pattern-stored-procedure-scope.md to skills/cosmosdb-design-patterns/rules/pattern-stored-procedure-scope.md and then run npm run build to regenerate AGENTS.md for both skills.
|
@ramnnn2006 there are some ongoing changes being evaluated to the structure that could require this to be modified. you'll definetely get notice when it's time to make any changes to avoid merge conflicts. |
|
okay , lmk when I could start working on it then @jaydestro |
Description
This adds a new rule under Design Patterns about when to actually use stored procedures in Cosmos DB. The short version is sprocs only make sense when you need multiple writes to commit atomically in the same partition and transactional batch can't do it. For everything else (single item CRUD, cross partition stuff, heavy computation) the rule says use plain SDK calls or transactional batch instead. Also covers the limitations like the 5 second execution cap and how painful sprocs are to debug. Includes the regenerated AGENTS.md and an eval task.
Type of Change
Checklist
npm run validateand it passednpm run buildto regenerate AGENTS.md (if a{prefix}-{description}.mdtitle, `impacTests (Required)
waza run evals/cosmosdb-best-practices/eval.yamland all tasks passid,name,description, d.outcomes`Eval task file: `evals/cosmosdb-best-practices/tasks/
For New Rules
Rule file:
skills/cosmosdb-best-practices/rules/pattCategory: Design Patterns
Impact level: Medium
Why is this rule important?
Sprocs get treated like a free server side compute layer, but they only see one partition, get killed around 5 seconds, and debugging them is rough since it's JavaScript living inside the database. Went will happily suggest a sproc for a basic insert orsome aggregation, when a normal SDK call or transactional batch does the same job and is way easier to test and maintain. The rule keeps sprocs for the one case where they actually earn their keep, whis to read data before deciding what to write, all withinone partition.
Agent Testing
Related Issues
Closes #174