docs(vault): document allowedAgents enforcement, and correct the javadoc that denies it - #667
Conversation
…doc that denies it allowedAgents became enforced (#662) and enforcement became the default (#664), but docs/secrets-vault.md never mentioned eddi.vault.grant-enforcement at all. The only description of the feature lived in the changelog, which operators do not read. Adds an "Agent Grants" section: the three modes, the two parsing rules (an unknown value fails startup, absent/blank resolves to enforce), what counts as granted, which configurations are scanned, and the upgrade step for the one deployment shape that is actually affected -- a master key plus a deliberately narrowed grant, where the first symptom is an agent refusing to deploy with no warning phase. The javadoc was worse than missing. Four places still told the reader the field is not enforced: SecretMetadata (flatly wrong), VaultSecretProvider and EncryptedSecret (true of those classes, but reading as "not enforced anywhere"), and the AgentSetupService comment arguing for "*" on the grounds that the enforcement does not exist. AgentSetupService still writes ["*"], which is still correct, but for a different reason than the old comment gave: the wizard vaults the key before the agent exists (vaultApiKey at line 165, agentId extracted at 209) and only ever receives agentName, so narrowing there would mean guessing an unassigned ID and blocking the agent the key was vaulted for. Documentation only -- no executable line changed.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis documentation-only change defines ChangesallowedAgents documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
The "Additional vault settings in application.properties" block listed cache-ttl-minutes and cache-max-size but not grant-enforcement, so an operator scanning it for the available knobs would not have found the one that decides whether a misconfigured agent deploys.
![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 `@src/main/java/ai/labs/eddi/secrets/model/SecretMetadata.java`:
- Around line 16-20: Update the Javadoc near SecretMetadata to qualify that an
allowed-agent violation stops deployment only when VaultGrantGate.Mode.ENFORCE
is active; note that WARN permits deployment after logging and OFF skips
enforcement.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd10a643-704e-4e85-8754-7b4f28c27a94
📒 Files selected for processing (6)
docs/changelog.mddocs/secrets-vault.mdsrc/main/java/ai/labs/eddi/engine/setup/AgentSetupService.javasrc/main/java/ai/labs/eddi/secrets/impl/VaultSecretProvider.javasrc/main/java/ai/labs/eddi/secrets/model/EncryptedSecret.javasrc/main/java/ai/labs/eddi/secrets/model/SecretMetadata.java
…inish the sweep Review feedback on #667: the javadoc said a violation stops the agent coming up, which is true only under enforce. warn logs and allows, off does not check at all. Every place asserting the blocking behavior now names eddi.vault.grant-enforcement as what decides it -- SecretMetadata (the flagged one), plus EncryptedSecret and VaultSecretProvider, which carried the same unqualified claim and were not flagged. The doc's own lead paragraph had the same problem and now points at the modes table instead of asserting the agent "simply does not come up". Grepping the phrase rather than fixing one file per review comment turned up four more files still describing the old access model as current: ISecretProvider, SecretReference, IRestSecretStore and SecretResolver all said "access control is via configuration authorship". SecretResolver's second sentence -- that the resolver itself does not check permissions -- was accurate and is kept; only the stale lead-in changed. VaultGrantChecker and its test quote the old wording deliberately ("was documented as...") and keep it. That is history, not a stale claim. Documentation only -- no executable line changed. compile, validate and 80 vault tests green.
main moved again before this branch was pushed (#664, #665, #667, #668). Conflicts, both in docs: - docs/changelog.md — both sides prepended entries again; kept both, nothing dropped. - docs/secrets-vault.md — #667 documented vault agent grants independently, and #664 changed the default to enforce, which made this branch's "warn (default)" row wrong. Resolved in main's favour: main's section is kept whole and this branch's duplicate dropped, rather than interleaved. Follow-on fixes: the group docs' cross-reference now points at main's anchor and says the thing main's section does not — that a sub-agent inheriting a parent's vault reference must itself be granted the secret, or under the new default it will not deploy. Re-verified after the merge: the "80+ MCP tools" claim in README/docs/AGENTS still holds (84 @tool methods, matching McpToolFilter's whitelist exactly).
2 participants
allowedAgentsbecame enforced in #662 and enforcement became the default in #664, butdocs/secrets-vault.mdnever mentionededdi.vault.grant-enforcementat all. The only description of the feature lived indocs/changelog.md, which operators don't read.The doc section
New Agent Grants section in
docs/secrets-vault.md, between Secret References and Encryption:AgentFactory.deployAgent— the one boundary every deployment path funnels throughenforcerather than something weakernull/empty means unrestricted, not "deny all", and that uncertainty (unreadable metadata, disabled vault, missing secret) never becomes a violationThe javadoc was worse than missing — it was wrong
Four places still told the reader the field is not enforced:
SecretMetadataVaultSecretProviderEncryptedSecretAgentSetupServiceThis is the same text that, earlier in this review, caused a proposed narrowing of
allowedAgentsto be reverted as security theater. It was accurate when written and became false when the behavior changed under it — left alone, it now misleads in the opposite direction.AgentSetupServicestill writes["*"]Still correct, but for a different reason than the old comment gave. The wizard vaults the key before the agent exists —
vaultApiKeyis called at line 165,agentIdis only extracted at line 209, and the method only ever receivesagentName. Narrowing at that call site would mean guessing an unassigned ID, and guessing wrong blocks the very agent the key was vaulted for. The comment now says that, instead of citing an enforcement gap that has since closed.Verification
compileVaultGrantGateModeTest,VaultGrantCheckerTest,VaultSecretProviderTest,VaultSecretProviderBranchTest), counts read from the Surefire XML; the.txtsummary reportsTests run: 0for@NestedclassesOperator note
This documents, but does not change, the behavior shipped in #664. The deployments actually affected by enforcement have both a vault master key and a deliberately narrowed grant; everywhere else the control is inert (no master key short-circuits the checker, and auto-vaulted keys carry
["*"]).Summary by CodeRabbit