Add typed brokered credential contract - #84
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| session: CredentialSession | ||
| delivery: CredentialDelivery | ||
| opaque_handle: str |
There was a problem hiding this comment.
Suggestion: CredentialLease does not enforce a consistent delivery shape: an environment lease can omit env_key, while a broker lease can carry one. Downstream materialization or injection can therefore receive an unusable lease or route credential delivery through the wrong mechanism. Validate the delivery/env_key combination in __post_init__, matching the request invariants. [security]
Severity Level: Major ⚠️
- ⚠️ Host broker implementations receive unusable lease metadata.
- ⚠️ Credential proxy or environment delivery can fail validation.
- ⚠️ Each consumer must duplicate request delivery invariants.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/dream/contracts/credentials.py
**Line:** 173:175
**Comment:**
*Security: `CredentialLease` does not enforce a consistent delivery shape: an environment lease can omit `env_key`, while a broker lease can carry one. Downstream materialization or injection can therefore receive an unusable lease or route credential delivery through the wrong mechanism. Validate the `delivery`/`env_key` combination in `__post_init__`, matching the request invariants.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| class CredentialRequestResult: | ||
| status: CredentialRequestStatus | ||
| grant: CredentialGrant | None = None | ||
| ask: CredentialAsk | None = None | ||
|
|
There was a problem hiding this comment.
Suggestion: CredentialRequestResult permits structurally invalid results such as GRANTED with no grant or APPROVAL_REQUIRED with no ask. Consumers must then dereference a missing payload or add repeated status-specific checks, defeating the typed broker contract. Validate the required payload for each status and reject the contradictory payload combinations. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Credential acquisition requires repeated payload checks.
- ❌ Malformed broker results can fail grant materialization.
- ⚠️ Approval flows can lack an actionable ask identifier.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/dream/contracts/credentials.py
**Line:** 180:184
**Comment:**
*Api Mismatch: `CredentialRequestResult` permits structurally invalid results such as `GRANTED` with no `grant` or `APPROVAL_REQUIRED` with no `ask`. Consumers must then dereference a missing payload or add repeated status-specific checks, defeating the typed broker contract. Validate the required payload for each status and reject the contradictory payload combinations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| @property | ||
| def credential_broker(self) -> CredentialBrokerPort | None: ... |
There was a problem hiding this comment.
Suggestion: Adding credential_broker as a required member of the runtime-checkable ToolContext protocol breaks structural compatibility for every existing external context implementation that follows the previous contract. Such implementations will no longer satisfy isinstance(context, ToolContext) or static protocol checks even when credential access is unused. Preserve compatibility with an optional extension protocol or otherwise coordinate this as a breaking contract change rather than an additive one. [api mismatch]
Severity Level: Major ⚠️
- ❌ Existing external context adapters fail protocol checks.
- ⚠️ Third-party tools may require coordinated updates.
- ⚠️ Compatibility breaks despite the broker being optional at runtime.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/dream/contracts/tool.py
**Line:** 48:49
**Comment:**
*Api Mismatch: Adding `credential_broker` as a required member of the runtime-checkable `ToolContext` protocol breaks structural compatibility for every existing external context implementation that follows the previous contract. Such implementations will no longer satisfy `isinstance(context, ToolContext)` or static protocol checks even when credential access is unused. Preserve compatibility with an optional extension protocol or otherwise coordinate this as a breaking contract change rather than an additive one.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
@greptileai please review |
Greptile SummaryThe PR adds a public brokered-credential contract and threads an optional broker from harness construction into tool execution contexts.
Confidence Score: 4/5The PR appears safe to merge, with non-blocking improvements recommended for result-state invariants and end-to-end broker-wiring coverage. The production broker path is consistently forwarded into tool contexts, but the public result type admits contradictory status and payload combinations and the tests do not protect the full forwarding chain. Files Needing Attention: src/dream/contracts/credentials.py, tests/test_tools/test_context.py
|
| Filename | Overview |
|---|---|
| src/dream/contracts/credentials.py | Adds the credential-broker contract; the request result does not enforce consistency between its discriminator and optional payloads. |
| src/dream/_factory.py | Accepts the optional broker and correctly captures it for session-engine construction. |
| src/dream/engine/_engine.py | Correctly forwards and retains the broker on the dispatcher and query engine. |
| src/dream/engine/_tool_dispatch.py | Correctly places the configured broker into each ToolExecutionContext. |
| src/dream/contracts/tool.py | Adds the optional broker property to the public ToolContext protocol. |
| tests/test_tools/test_context.py | Verifies the destination context field directly but does not cover the newly added production forwarding chain. |
Sequence Diagram
sequenceDiagram
participant App
participant Harness as build_harness
participant Session as _build_session_engine
participant Engine as build_query_engine
participant Dispatcher as EngineToolDispatcher
participant Tool as ToolExecutionContext
App->>Harness: credential_broker
Harness->>Session: credential_broker
Session->>Engine: credential_broker
Engine->>Dispatcher: credential_broker
Dispatcher->>Tool: credential_broker
Tool-->>App: broker available to tool
Prompt To Fix All With AI
### Issue 1
src/dream/contracts/credentials.py:179-183
**Result payloads allow contradictory states**
`CredentialRequestResult` permits `GRANTED` without a grant, `APPROVAL_REQUIRED` without an ask, or both payloads at once. Consumers selecting a payload from `status` can therefore dereference `None` or process the wrong payload, so the result should enforce exactly one status-appropriate value.
```suggestion
@dataclass(frozen=True)
class CredentialRequestResult:
status: CredentialRequestStatus
grant: CredentialGrant | None = None
ask: CredentialAsk | None = None
def __post_init__(self) -> None:
if self.status is CredentialRequestStatus.GRANTED:
if self.grant is None or self.ask is not None:
raise ValueError("granted result requires only a grant")
elif self.status is CredentialRequestStatus.APPROVAL_REQUIRED:
if self.ask is None or self.grant is not None:
raise ValueError("approval-required result requires only an ask")
```
### Issue 2
tests/test_tools/test_context.py:35-42
**Broker wiring remains untested**
This test constructs `ToolExecutionContext` directly, bypassing the newly added forwarding chain from `build_harness` through the session engine and dispatcher. A regression that drops the broker at any intermediate hop would leave tools with `credential_broker=None` while this test still passes; add coverage that dispatches a tool through a broker-configured harness.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "add typed brokered credential contract" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @dataclass(frozen=True) | ||
| class CredentialRequestResult: | ||
| status: CredentialRequestStatus | ||
| grant: CredentialGrant | None = None | ||
| ask: CredentialAsk | None = None |
There was a problem hiding this comment.
CredentialRequestResult permits GRANTED without a grant, APPROVAL_REQUIRED without an ask, or both payloads at once. Consumers selecting a payload from status can therefore dereference None or process the wrong payload, so the result should enforce exactly one status-appropriate value.
| @dataclass(frozen=True) | |
| class CredentialRequestResult: | |
| status: CredentialRequestStatus | |
| grant: CredentialGrant | None = None | |
| ask: CredentialAsk | None = None | |
| @dataclass(frozen=True) | |
| class CredentialRequestResult: | |
| status: CredentialRequestStatus | |
| grant: CredentialGrant | None = None | |
| ask: CredentialAsk | None = None | |
| def __post_init__(self) -> None: | |
| if self.status is CredentialRequestStatus.GRANTED: | |
| if self.grant is None or self.ask is not None: | |
| raise ValueError("granted result requires only a grant") | |
| elif self.status is CredentialRequestStatus.APPROVAL_REQUIRED: | |
| if self.ask is None or self.grant is not None: | |
| raise ValueError("approval-required result requires only an ask") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/dream/contracts/credentials.py
Line: 179-183
Comment:
**Result payloads allow contradictory states**
`CredentialRequestResult` permits `GRANTED` without a grant, `APPROVAL_REQUIRED` without an ask, or both payloads at once. Consumers selecting a payload from `status` can therefore dereference `None` or process the wrong payload, so the result should enforce exactly one status-appropriate value.
```suggestion
@dataclass(frozen=True)
class CredentialRequestResult:
status: CredentialRequestStatus
grant: CredentialGrant | None = None
ask: CredentialAsk | None = None
def __post_init__(self) -> None:
if self.status is CredentialRequestStatus.GRANTED:
if self.grant is None or self.ask is not None:
raise ValueError("granted result requires only a grant")
elif self.status is CredentialRequestStatus.APPROVAL_REQUIRED:
if self.ask is None or self.grant is not None:
raise ValueError("approval-required result requires only an ask")
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| def test_context_carries_typed_credential_broker(tmp_path: Path) -> None: | ||
| broker = cast(CredentialBrokerPort, object()) | ||
| ctx = ToolExecutionContext( | ||
| working_dir=tmp_path, | ||
| session_id="session", | ||
| credential_broker=broker, | ||
| ) | ||
| assert ctx.credential_broker is broker |
There was a problem hiding this comment.
This test constructs ToolExecutionContext directly, bypassing the newly added forwarding chain from build_harness through the session engine and dispatcher. A regression that drops the broker at any intermediate hop would leave tools with credential_broker=None while this test still passes; add coverage that dispatches a tool through a broker-configured harness.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_tools/test_context.py
Line: 35-42
Comment:
**Broker wiring remains untested**
This test constructs `ToolExecutionContext` directly, bypassing the newly added forwarding chain from `build_harness` through the session engine and dispatcher. A regression that drops the broker at any intermediate hop would leave tools with `credential_broker=None` while this test still passes; add coverage that dispatches a tool through a broker-configured harness.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…nts, prompt surfaces, tool guardrails
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
codeant-ai
Bot
added
size:L
1 participant
User description
Summary
dream.contracts.credentialsseam with enums, grant/ask/lease objects, and no plaintext fieldsToolContextand session constructionVerification
uv run pytest -q tests/test_contracts/test_credentials.py tests/test_tools/test_context.py tests/test_factory.py tests/test_public_api.pyChorus depends on this contract bump.
CodeAnt-AI Description
Add a typed brokered credential contract for tools
What Changed
Impact
✅ No plaintext credentials in tool-facing contracts✅ Controlled credential access with approval and revocation✅ Tools can use brokered credentials through proxy or environment delivery💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.