[AI-211] Enhance agent metadata with tool schemas, guardrails, and quick-reference guide#147
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSix new documentation and configuration files are added that define the Mifos Self-Service Plugin Agent's identity, security guardrails, implementation patterns, features, and runtime/configuration metadata for integration with Apache Fineract. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (4)
skills.md (2)
109-116: Align service class name with the naming convention in this same guide.Line [110] uses
SelfSavingsServiceas a concrete class, while Line [47] defines concrete implementations asXxxServiceImpl. Keep this example consistent to avoid confusion.Proposed naming alignment
-public class SelfSavingsService { +public class SelfSavingsServiceImpl implements SelfSavingsService { private final PlatformSelfServiceSecurityContext securityContext;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills.md` around lines 109 - 116, Rename the concrete class SelfSavingsService to follow the project's concrete-implementation convention (e.g., SelfSavingsServiceImpl): update the class declaration (SelfSavingsService → SelfSavingsServiceImpl), preserve the existing constructor annotation (`@RequiredArgsConstructor`) and method validateAccess (securityContext.validateSelfServiceUserAccess(accountId)), and update any references/imports/usages and surrounding documentation/examples to use the new name so consistency with other concrete implementations (XxxServiceImpl) is maintained.
123-126: Add constructor injection to the validator snippet.Line [125] declares a
finaldependency without showing constructor wiring. Add@RequiredArgsConstructorto keep the example copy-paste safe and consistent.Proposed snippet fix
`@Component` +@RequiredArgsConstructor public class SelfSavingsDataValidator { private final FromJsonHelper fromJsonHelper;As per coding guidelines: "Prefer immutability and constructor injection for all beans."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills.md` around lines 123 - 126, The example SelfSavingsDataValidator shows a final dependency field fromJsonHelper but no constructor wiring; annotate the class SelfSavingsDataValidator with Lombok's `@RequiredArgsConstructor` (in addition to `@Component`) so the final field is injected via constructor injection, keeping the bean immutable and copy-paste safe; ensure Lombok is available/imported where this snippet is used.AGENTS.md (1)
46-46: Make Liquibase path more specific for faster onboarding.Line [46] is correct at a high level, but specifying the self-service path avoids confusion when locating entry changelogs.
Proposed doc update
-The plugin uses Liquibase for database migrations. Scripts are located in `src/main/resources/db/changelog/`. +The plugin uses Liquibase for database migrations. Self-service scripts are located in `src/main/resources/db/changelog/tenant/module/selfservice/` (entrypoint: `module-changelog-master.xml`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 46, Update the AGENTS.md line that says scripts are in `src/main/resources/db/changelog/` to the more specific self-service entry path so readers can find the entry changelogs quickly; replace the generic path with the self-service path (e.g., `src/main/resources/db/changelog/self-service/` or the exact entry changelog filename) and mention that entry changelogs live under that self-service folder.SOUL_GUARDRAILS.md (1)
10-10: Define “explicit user intent” more concretely.Line [10] is directionally correct but operationally ambiguous. Add 1–2 concrete examples so tool and reviewer behavior stays consistent.
Proposed wording refinement
-- Do not perform state-changing actions without explicit user intent. +- Do not perform state-changing actions without explicit user intent (for example: a direct confirmation action, an authenticated command, or an explicit user-approved request payload).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SOUL_GUARDRAILS.md` at line 10, The guideline "Do not perform state-changing actions without explicit user intent." is vague; update SOUL_GUARDRAILS.md by replacing that sentence with a clearer definition of "explicit user intent" and add 1–2 concrete examples — e.g., "explicit user intent" means a direct, affirmative user command such as "Send this email now" or a confirmed UI action like clicking "Confirm Purchase"; and include a counterexample such as "do not infer intent from passive conversation or send transactions based on implicit signals." Ensure the edited line and examples are concise and placed where the original sentence appears so reviewers and tools can apply consistent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent.yaml`:
- Around line 23-35: The metadata's global authentication_required: true
conflicts with the runtime allowlist in SelfServiceSecurityConfiguration which
permits unauthenticated access to the endpoints at endpoints.authentication and
endpoints.registration under endpoints.base_path; fix the mismatch by updating
the YAML to reflect runtime behavior—either set authentication_required: false
and add per-endpoint authentication flags for the endpoints that require auth,
or keep authentication_required: true and add an explicit public_endpoints list
containing the endpoints.authentication and endpoints.registration paths
(matching the base_path) so tooling sees these two as public like
SelfServiceSecurityConfiguration does.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 46: Update the AGENTS.md line that says scripts are in
`src/main/resources/db/changelog/` to the more specific self-service entry path
so readers can find the entry changelogs quickly; replace the generic path with
the self-service path (e.g., `src/main/resources/db/changelog/self-service/` or
the exact entry changelog filename) and mention that entry changelogs live under
that self-service folder.
In `@skills.md`:
- Around line 109-116: Rename the concrete class SelfSavingsService to follow
the project's concrete-implementation convention (e.g., SelfSavingsServiceImpl):
update the class declaration (SelfSavingsService → SelfSavingsServiceImpl),
preserve the existing constructor annotation (`@RequiredArgsConstructor`) and
method validateAccess
(securityContext.validateSelfServiceUserAccess(accountId)), and update any
references/imports/usages and surrounding documentation/examples to use the new
name so consistency with other concrete implementations (XxxServiceImpl) is
maintained.
- Around line 123-126: The example SelfSavingsDataValidator shows a final
dependency field fromJsonHelper but no constructor wiring; annotate the class
SelfSavingsDataValidator with Lombok's `@RequiredArgsConstructor` (in addition to
`@Component`) so the final field is injected via constructor injection, keeping
the bean immutable and copy-paste safe; ensure Lombok is available/imported
where this snippet is used.
In `@SOUL_GUARDRAILS.md`:
- Line 10: The guideline "Do not perform state-changing actions without explicit
user intent." is vague; update SOUL_GUARDRAILS.md by replacing that sentence
with a clearer definition of "explicit user intent" and add 1–2 concrete
examples — e.g., "explicit user intent" means a direct, affirmative user command
such as "Send this email now" or a confirmed UI action like clicking "Confirm
Purchase"; and include a counterexample such as "do not infer intent from
passive conversation or send transactions based on implicit signals." Ensure the
edited line and examples are concise and placed where the original sentence
appears so reviewers and tools can apply consistent behavior.
🪄 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: CHILL
Plan: Pro
Run ID: 91d34020-eee0-4bd5-aa00-cb5ee7c6977e
📒 Files selected for processing (6)
AGENTS.mdSOUL.mdSOUL_GUARDRAILS.mdagent.yamlllms.txtskills.md
|
Addressed review feedback:\n\n- Added public endpoint definitions in agent.yaml to align with runtime behavior\n- Clarified guardrail definitions for explicit user intent\n\nKept changes minimal and non-breaking. Happy to refine further if needed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
SOUL_GUARDRAILS.md (2)
18-23: Minor: License constraint placement.Line 22 ("Follow repository license and compliance constraints for generated content") is slightly tangential to data security constraints. Consider whether this belongs in a separate compliance section or should be reworded to emphasize its data handling aspect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SOUL_GUARDRAILS.md` around lines 18 - 23, The sentence "Follow repository license and compliance constraints for generated content" under the "Data Handling Constraints" section reads more like a legal/compliance item than a data-handling rule—either move that line out into a new "Compliance and Licensing" section or reword it to tie directly to data handling (e.g., "Ensure generated content and logs comply with repository licensing and compliance requirements, especially for handling and sharing sensitive data"); update the "Data Handling Constraints" block accordingly so the header contains only data-specific controls and add a new "Compliance and Licensing" header containing the original line if moved.
6-10: Consider minor rephrasing for variety.The repeated "Do not" structure is functionally clear but flagged by static analysis. If desired, you could vary the phrasing, though the parallel structure is acceptable for documentation lists.
📝 Optional stylistic variation
-Do not bypass authentication or authorization checks. -- Do not return data for accounts that the authenticated user does not own. -- Do not expose secrets, tokens, passwords, or raw stack traces. -- Do not invent financial data, account balances, or transaction outcomes. -- Do not perform state-changing actions without explicit user intent, such as a direct confirmation action or an authenticated command. +Never bypass authentication or authorization checks. +- Deny access to data for accounts the authenticated user does not own. +- Prohibit exposure of secrets, tokens, passwords, or raw stack traces. +- Refuse to fabricate financial data, account balances, or transaction outcomes. +- Require explicit user intent (direct confirmation or authenticated command) before performing state-changing actions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SOUL_GUARDRAILS.md` around lines 6 - 10, The repeated "Do not" lead-in in SOUL_GUARDRAILS.md can be varied for stylistic diversity: update the five list items (the lines starting "Do not bypass authentication or authorization checks.", "Do not return data for accounts that the authenticated user does not own.", "Do not expose secrets, tokens, passwords, or raw stack traces.", "Do not invent financial data, account balances, or transaction outcomes.", and "Do not perform state-changing actions without explicit user intent, such as a direct confirmation action or an authenticated command.") to equivalent rephrasings that preserve meaning (e.g., "Avoid bypassing authentication or authorization checks", "Only return data owned by the authenticated user", "Keep secrets, tokens, passwords, and stack traces confidential", "Never fabricate financial data, balances, or transaction results", "Require explicit user confirmation before making state-changing actions"), ensuring the semantics remain identical and that compliance guidance is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@SOUL_GUARDRAILS.md`:
- Around line 18-23: The sentence "Follow repository license and compliance
constraints for generated content" under the "Data Handling Constraints" section
reads more like a legal/compliance item than a data-handling rule—either move
that line out into a new "Compliance and Licensing" section or reword it to tie
directly to data handling (e.g., "Ensure generated content and logs comply with
repository licensing and compliance requirements, especially for handling and
sharing sensitive data"); update the "Data Handling Constraints" block
accordingly so the header contains only data-specific controls and add a new
"Compliance and Licensing" header containing the original line if moved.
- Around line 6-10: The repeated "Do not" lead-in in SOUL_GUARDRAILS.md can be
varied for stylistic diversity: update the five list items (the lines starting
"Do not bypass authentication or authorization checks.", "Do not return data for
accounts that the authenticated user does not own.", "Do not expose secrets,
tokens, passwords, or raw stack traces.", "Do not invent financial data, account
balances, or transaction outcomes.", and "Do not perform state-changing actions
without explicit user intent, such as a direct confirmation action or an
authenticated command.") to equivalent rephrasings that preserve meaning (e.g.,
"Avoid bypassing authentication or authorization checks", "Only return data
owned by the authenticated user", "Keep secrets, tokens, passwords, and stack
traces confidential", "Never fabricate financial data, balances, or transaction
results", "Require explicit user confirmation before making state-changing
actions"), ensuring the semantics remain identical and that compliance guidance
is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f28c73de-f9c7-4ebf-92ac-2f969661dc0c
📒 Files selected for processing (2)
SOUL_GUARDRAILS.mdagent.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- agent.yaml
3 participants
Summary
This is a follow-up to #115 for AI-211 and introduces small, non-breaking metadata enhancements on top of that work.
Why this PR
PR #115 introduced foundational AI metadata files. This follow-up keeps that foundation intact and adds additive improvements for tooling interoperability, developer onboarding, and safe agent behavior.
Improvements in this PR
Compatibility
Developer Experience and Safety
These additions improve discoverability for agent tools, shorten onboarding time for contributors, and provide clear safety boundaries for AI-assisted development.
Summary by CodeRabbit
Documentation
Chores