Repository navigation
Refactor policy gateway into hexagonal architecture - #2
Conversation
Reviewer's GuideThis PR refactors the FastAPI policy gateway into a hexagonal architecture by extracting decision logic into an application service with defined ports and domain models, isolating framework code through HTTP schemas and a file-based configuration adapter, and adds unit tests to cover policy decisions and config loading. Sequence diagram for a prompt filter requestsequenceDiagram
actor User
participant FastAPI
participant Schemas
participant PolicyDecisionService
participant ConfigFileAdapter
User->>FastAPI: POST /filter/prompt
FastAPI->>Schemas: Parse PromptCheckRequest
FastAPI->>PolicyDecisionService: decide_prompt(PromptDecisionInput)
PolicyDecisionService->>ConfigFileAdapter: load()
PolicyDecisionService-->>FastAPI: DecisionResult
FastAPI->>Schemas: DecisionResponse
FastAPI-->>User: DecisionResponse
Sequence diagram for CI check requestsequenceDiagram
actor User
participant FastAPI
participant Schemas
participant PolicyDecisionService
participant ConfigFileAdapter
User->>FastAPI: POST /ci/check
FastAPI->>Schemas: Parse CiCheckRequest
FastAPI->>PolicyDecisionService: ci_check(CiCheckInput)
PolicyDecisionService->>ConfigFileAdapter: load()
PolicyDecisionService-->>FastAPI: CiCheckResult
FastAPI->>Schemas: CiCheckResponse
FastAPI-->>User: CiCheckResponse
Class diagram for new domain models and serviceclassDiagram
class PolicyDecisionService {
- ConfigurationPort _configuration_port
+ health() Dict[str, str]
+ rules() RulesSnapshot
+ decide_prompt(request: PromptDecisionInput) DecisionResult
+ decide_output(request: OutputDecisionInput) DecisionResult
+ ci_check(request: CiCheckInput) CiCheckResult
}
class ConfigurationPort {
<<interface>>
+ load() dict
}
class ConfigFileAdapter {
- path: str | os.PathLike[str]
+ load() dict
}
class PromptDecisionInput {
+ prompt: str
+ context: Dict[str, object]
}
class OutputDecisionInput {
+ output: str
+ context: Dict[str, object]
}
class CiCheckInput {
+ quality: Dict[str, float]
+ fairness: Dict[str, float]
+ safety: Dict[str, float]
+ drift: Dict[str, float]
}
class DecisionResult {
+ allowed: bool
+ action: str
+ reasons: List[str]
}
class CiCheckResult {
+ status: str
+ violations: List[str]
}
class RulesSnapshot {
+ raw: Dict[str, object]
+ to_dict() Dict[str, object]
}
PolicyDecisionService --> ConfigurationPort
ConfigFileAdapter --|> ConfigurationPort
PolicyDecisionService --> PromptDecisionInput
PolicyDecisionService --> OutputDecisionInput
PolicyDecisionService --> CiCheckInput
PolicyDecisionService --> DecisionResult
PolicyDecisionService --> CiCheckResult
PolicyDecisionService --> RulesSnapshot
Class diagram for HTTP schemasclassDiagram
class PromptCheckRequest {
+ prompt: str
+ context: Dict[str, Any] | None
}
class OutputCheckRequest {
+ output: str
+ context: Dict[str, Any] | None
}
class CiCheckRequest {
+ quality: Dict[str, float]
+ fairness: Dict[str, float]
+ safety: Dict[str, float]
+ drift: Dict[str, float]
}
class DecisionResponse {
+ allowed: bool
+ action: str
+ reasons: list[str]
}
class CiCheckResponse {
+ status: str
+ violations: list[str]
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning
|
| Cohort / File(s) | Summary |
|---|---|
App / Endpoints services/policy-gateway/src/app.py |
Adds _build_service and get_service DI, moves health/rules/filter/ci endpoints to service delegation, adds /proxy/completion and /proxy/completion/stream (SSE) proxy endpoints and LLM adapter selection. |
Application service services/policy-gateway/src/policy_gateway/application/services.py, services/policy-gateway/src/policy_gateway/__init__.py |
New PolicyDecisionService with health(), rules(), decide_prompt(), decide_output(), ci_check() and _lookup_threshold(); exported via package __init__. |
Domain models services/policy-gateway/src/policy_gateway/domain/models.py |
New frozen dataclasses and LLM models: PromptDecisionInput, OutputDecisionInput, CiCheckInput, DecisionResult, CiCheckResult, RulesSnapshot, CompletionRequest, CompletionResponse with serialization helpers. |
Ports services/policy-gateway/src/policy_gateway/ports/configuration.py, services/policy-gateway/src/policy_gateway/ports/llm_adapter.py |
New protocol interfaces: ConfigurationPort (load) and LLMAdapterPort (complete, stream). |
Infrastructure adapters services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py, .../litellm_adapter.py, .../llm_http_adapter.py |
New ConfigFileAdapter (YAML/JSON loader, returns {} on missing/parse errors), LiteLLMAdapter (litellm integration with streaming and fallbacks), and HTTPLLMAdapter (proxy to upstream HTTP completion with streaming fallback). |
HTTP schemas / interface services/policy-gateway/src/policy_gateway/interface/http/schemas.py, specs/openapi-policy-gateway.yaml |
Pydantic request/response models for prompt/output/ci and completion proxy; OpenAPI adds /proxy/completion and CompletionRequest/CompletionResponse schemas; minor schema styling changes. |
Tests services/policy-gateway/tests/*, services/policy-gateway/tests/test_policy_service.py, .../test_http_responses.py, .../test_integration.py, .../test_llm_proxy.py, .../test_streaming.py |
New unit and integration tests for PolicyDecisionService logic, config adapter, HTTP mapping, LLM proxy forwarding, and SSE streaming behavior. |
Dev tooling & CI scripts/dev_setup.sh, services/policy-gateway/requirements-dev.txt, .github/workflows/ci.yml, justfile, .gitignore |
Adds development setup script, dev requirements, CI workflow, just task dev-setup, and ignores .venv/. |
Docs & config README.md, docs/..., docs/Reference/LLMOps_Specs/*, tmp_cfg*.yaml, .coderabbit.yml |
Extensive documentation updates (ENV, ADRs, LLMOps spec updates introducing PAC_LLM_PROVIDER and streaming), example config files, automation config, and OpenAPI docs adjustments. |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant API as FastAPI
participant Service as PolicyDecisionService
participant Config as ConfigFileAdapter
participant Adapter as LLMAdapter
Client->>API: POST /filter/prompt (PromptCheckRequest)
API->>API: Depends(get_service) -> get_service()
API->>Service: decide_prompt(PromptDecisionInput)
Service->>Config: load()
Config-->>Service: config dict
Service-->>API: DecisionResult
API-->>Client: DecisionResponse (JSON)
opt Proxy completion (sync)
Client->>API: POST /proxy/completion (CompletionRequest)
API->>API: _build_llm_adapter(PAC_LLM_PROVIDER)
API->>Adapter: complete(CompletionRequest)
Adapter-->>API: CompletionResponse
API-->>Client: CompletionResponse (JSON)
end
opt Proxy completion (stream)
Client->>API: POST /proxy/completion/stream (CompletionRequest)
API->>API: _build_llm_adapter(PAC_LLM_PROVIDER)
API->>Adapter: stream(CompletionRequest)
loop chunks
Adapter-->>API: chunk
API-->>Client: SSE data: chunk
end
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Files/areas to focus review on:
- PolicyDecisionService: threshold extraction, numeric conversions, edge-case handling in decide_prompt/decide_output/ci_check.
- Adapter implementations: error normalization, parsing of variable upstream shapes, and streaming fallbacks in LiteLLMAdapter and HTTPLLMAdapter.
- ConfigFileAdapter.load: silent-failure behavior on missing/parse errors vs. logging/visibility.
- app.py DI wiring and runtime LLM adapter selection via PAC_LLM_PROVIDER.
- SSE formatting and streaming correctness in
/proxy/completion/streamand corresponding tests.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 11.67% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Refactor policy gateway into hexagonal architecture' directly and clearly summarizes the main change: restructuring the policy gateway codebase using hexagonal (ports and adapters) architecture principles. |
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.vscode/settings.json(1 hunks)README.md(7 hunks)scripts/dev_setup.sh(1 hunks)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py(1 hunks)services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py(1 hunks)services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py(1 hunks)services/policy-gateway/tests/test_llm_proxy.py(1 hunks)specs/openapi-policy-gateway.yaml(7 hunks)
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 help to get the list of available commands and usage tips.
Summary of ChangesHello @SPRIME01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural overhaul to the policy gateway service, transitioning it to a hexagonal architecture. The primary goal is to decouple the core business logic from external concerns such as the web framework and data persistence, thereby enhancing the system's testability, maintainability, and adaptability. This refactoring involves structuring the codebase into distinct layers, utilizing dependency inversion through ports and adapters, and establishing clear boundaries between different components. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider instantiating or refreshing the PolicyDecisionService per-request (rather than as a static global) if you need to pick up config changes at runtime.
- ConfigFileAdapter.load currently swallows all exceptions silently—adding logging or error reporting would help surface issues in malformed or unreadable config files.
- The
driftfield on CiCheckInput isn’t used in the ci_check implementation; either remove it or add logic to incorporate drift checks to prevent confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider instantiating or refreshing the PolicyDecisionService per-request (rather than as a static global) if you need to pick up config changes at runtime.
- ConfigFileAdapter.load currently swallows all exceptions silently—adding logging or error reporting would help surface issues in malformed or unreadable config files.
- The `drift` field on CiCheckInput isn’t used in the ci_check implementation; either remove it or add logic to incorporate drift checks to prevent confusion.
## Individual Comments
### Comment 1
<location> `services/policy-gateway/src/policy_gateway/application/services.py:31-35` </location>
<code_context>
+
+ def decide_prompt(self, request: PromptDecisionInput) -> DecisionResult:
+ context = request.context or {}
+ if context.get("contains_pii") and not context.get("lawful_basis"):
+ return DecisionResult(
+ allowed=False,
+ action="block",
+ reasons=["PII without lawful basis"],
+ )
+ if (context.get("jailbreak_score") or 0) > 0.8:
</code_context>
<issue_to_address>
**question:** PII check logic may not handle cases where lawful_basis is present but empty or falsy.
If you require lawful_basis to be a specific non-empty value, consider checking its type or content directly rather than relying on its truthiness.
</issue_to_address>
### Comment 2
<location> `services/policy-gateway/src/policy_gateway/application/services.py:37` </location>
<code_context>
+ action="block",
+ reasons=["PII without lawful basis"],
+ )
+ if (context.get("jailbreak_score") or 0) > 0.8:
+ return DecisionResult(
+ allowed=True,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Defaulting jailbreak_score to 0 may mask missing or malformed input.
Explicitly handle cases where jailbreak_score is missing or not a float, such as by logging or raising an error, to avoid unintended behavior.
Suggested implementation:
```python
jailbreak_score = context.get("jailbreak_score")
if jailbreak_score is None:
# Log missing jailbreak_score
logger.warning("Missing jailbreak_score in context for prompt decision")
else:
try:
score = float(jailbreak_score)
if score > 0.8:
return DecisionResult(
allowed=True,
action="safe_mode",
reasons=["high jailbreak score"],
)
except (TypeError, ValueError):
logger.warning(f"Malformed jailbreak_score in context: {jailbreak_score}")
```
- Ensure that a `logger` is available in this module. If not, you may need to import and configure Python's logging module at the top of the file:
```python
import logging
logger = logging.getLogger(__name__)
```
- If you prefer to raise an error instead of logging, replace the `logger.warning` lines with `raise ValueError(...)`.
- Adjust the logging level and error handling to match your application's conventions.
</issue_to_address>
### Comment 3
<location> `services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py:25` </location>
<code_context>
- if CFG_PATH.endswith((".yml", ".yaml")):
- return yaml.safe_load(open(CFG_PATH))
- return json.load(open(CFG_PATH))
- except Exception:
- return {}
+def _build_service() -> PolicyDecisionService:
</code_context>
<issue_to_address>
**suggestion:** Exception handling in config loader is too broad.
Catching all exceptions can hide specific errors like permission issues or config syntax problems. Please narrow the exception type and add logging for better debuggability.
</issue_to_address>
### Comment 4
<location> `services/policy-gateway/tests/test_policy_service.py:25-33` </location>
<code_context>
+ return PolicyDecisionService(InMemoryConfigAdapter(config or {}))
+
+
+def test_prompt_blocks_when_pii_without_basis():
+ service = create_service()
+ request = PromptDecisionInput(prompt="hi", context={"contains_pii": True})
+ result = service.decide_prompt(request)
+ assert result.allowed is False
+ assert result.action == "block"
+ assert "PII without lawful basis" in result.reasons
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for lawful basis present with PII.
Add a test where both 'contains_pii' is True and 'lawful_basis' is present in the context to confirm the prompt is allowed as expected.
```suggestion
def test_prompt_blocks_when_pii_without_basis():
service = create_service()
request = PromptDecisionInput(prompt="hi", context={"contains_pii": True})
result = service.decide_prompt(request)
assert result.allowed is False
assert result.action == "block"
assert "PII without lawful basis" in result.reasons
def test_prompt_allows_when_pii_with_lawful_basis():
service = create_service()
request = PromptDecisionInput(
prompt="hi",
context={"contains_pii": True, "lawful_basis": "consent"}
)
result = service.decide_prompt(request)
assert result.allowed is True
assert result.action != "block"
assert "PII without lawful basis" not in result.reasons
```
</issue_to_address>
### Comment 5
<location> `services/policy-gateway/tests/test_policy_service.py:33-40` </location>
<code_context>
+ assert "PII without lawful basis" in result.reasons
+
+
+def test_prompt_safe_mode_when_jailbreak_high():
+ service = create_service()
+ request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.9})
+ result = service.decide_prompt(request)
+ assert result.allowed is True
+ assert result.action == "safe_mode"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for jailbreak_score at boundary values.
Add tests for 'jailbreak_score' at 0.8 and 0.79 to verify correct threshold behavior for 'safe_mode'.
```suggestion
def test_prompt_safe_mode_when_jailbreak_high():
service = create_service()
request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.9})
result = service.decide_prompt(request)
assert result.allowed is True
assert result.action == "safe_mode"
def test_prompt_safe_mode_when_jailbreak_at_threshold():
service = create_service()
request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.8})
result = service.decide_prompt(request)
assert result.allowed is True
assert result.action == "safe_mode"
def test_prompt_no_safe_mode_when_jailbreak_below_threshold():
service = create_service()
request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.79})
result = service.decide_prompt(request)
assert result.allowed is True
assert result.action != "safe_mode"
```
</issue_to_address>
### Comment 6
<location> `services/policy-gateway/tests/test_policy_service.py:42-47` </location>
<code_context>
+ assert result.action == "safe_mode"
+
+
+def test_output_summarize_when_verbatim_high():
+ service = create_service()
+ request = OutputDecisionInput(output="o", context={"verbatim_ratio": 0.3})
+ result = service.decide_output(request)
+ assert result.allowed is True
+ assert result.action == "summarize"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for verbatim_ratio at and below threshold.
Add tests for 'verbatim_ratio' at 0.2 and just below (e.g., 0.19) to confirm the action only triggers when the ratio exceeds 0.2.
```suggestion
def test_output_summarize_when_verbatim_high():
service = create_service()
request = OutputDecisionInput(output="o", context={"verbatim_ratio": 0.3})
result = service.decide_output(request)
assert result.allowed is True
assert result.action == "summarize"
def test_output_no_summarize_when_verbatim_at_threshold():
service = create_service()
request = OutputDecisionInput(output="o", context={"verbatim_ratio": 0.2})
result = service.decide_output(request)
assert result.allowed is True
assert result.action != "summarize"
def test_output_no_summarize_when_verbatim_below_threshold():
service = create_service()
request = OutputDecisionInput(output="o", context={"verbatim_ratio": 0.19})
result = service.decide_output(request)
assert result.allowed is True
assert result.action != "summarize"
```
</issue_to_address>
### Comment 7
<location> `services/policy-gateway/tests/test_policy_service.py:50-59` </location>
<code_context>
+ assert result.action == "summarize"
+
+
+def test_ci_check_detects_violations():
+ cfg = {
+ "thresholds": {
+ "quality": {"pass_at_5": {"target": 0.8}},
+ "fairness": {"subgroup_delta": {"target_max": 0.2}},
+ "safety": {"harmful_rate": {"target_max": 0.1}},
+ }
+ }
+ service = create_service(cfg)
+ request = CiCheckInput(
+ quality={"pass_at_5": 0.7},
+ fairness={"subgroup_delta": 0.3},
+ safety={"harmful_rate": 0.2},
+ drift={},
+ )
+ result = service.ci_check(request)
+ assert result.status == "fail"
+ assert set(result.violations) == {
+ "quality.pass_at_5",
+ "fairness.subgroup_delta",
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for partial CI check violations.
Please add tests where only a subset of CI check metrics fail, to ensure the service reports violations accurately.
</issue_to_address>
### Comment 8
<location> `services/policy-gateway/tests/test_policy_service.py:94-100` </location>
<code_context>
+ assert result.violations == []
+
+
+def test_config_file_adapter_reads_yaml(tmp_path: Path):
+ config_data = {"foo": "bar"}
+ cfg_path = tmp_path / "config.yaml"
+ cfg_path.write_text(yaml.safe_dump(config_data))
+
+ adapter = ConfigFileAdapter(cfg_path)
+ assert adapter.load() == config_data
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for invalid YAML/JSON file content.
Add tests to verify that ConfigFileAdapter returns an empty dict and handles errors gracefully when loading invalid YAML or JSON.
```suggestion
def test_config_file_adapter_reads_yaml(tmp_path: Path):
config_data = {"foo": "bar"}
cfg_path = tmp_path / "config.yaml"
cfg_path.write_text(yaml.safe_dump(config_data))
adapter = ConfigFileAdapter(cfg_path)
assert adapter.load() == config_data
def test_config_file_adapter_invalid_yaml(tmp_path: Path):
# Write invalid YAML content
cfg_path = tmp_path / "invalid.yaml"
cfg_path.write_text("foo: [unclosed_list\nbar: baz")
adapter = ConfigFileAdapter(cfg_path)
assert adapter.load() == {}
def test_config_file_adapter_invalid_json(tmp_path: Path):
# Write invalid JSON content
cfg_path = tmp_path / "invalid.json"
cfg_path.write_text('{"foo": "bar", "baz": [1, 2,}') # Invalid JSON
adapter = ConfigFileAdapter(cfg_path)
assert adapter.load() == {}
```
</issue_to_address>
### Comment 9
<location> `services/policy-gateway/src/policy_gateway/application/services.py:75` </location>
<code_context>
status = "pass" if not violations else "fail"
</code_context>
<issue_to_address>
**suggestion (code-quality):** Swap if/else branches of if expression to remove negation ([`swap-if-expression`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/swap-if-expression))
```suggestion
status = "fail" if violations else "pass"
```
<br/><details><summary>Explanation</summary>Negated conditions are more difficult to read than positive ones, so it is best
to avoid them where we can. By swapping the `if` and `else` conditions around we
can invert the condition and make it positive.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if context.get("contains_pii") and not context.get("lawful_basis"): | ||
| return DecisionResult( | ||
| allowed=False, | ||
| action="block", | ||
| reasons=["PII without lawful basis"], |
There was a problem hiding this comment.
question: PII check logic may not handle cases where lawful_basis is present but empty or falsy.
If you require lawful_basis to be a specific non-empty value, consider checking its type or content directly rather than relying on its truthiness.
| action="block", | ||
| reasons=["PII without lawful basis"], | ||
| ) | ||
| if (context.get("jailbreak_score") or 0) > 0.8: |
There was a problem hiding this comment.
suggestion (bug_risk): Defaulting jailbreak_score to 0 may mask missing or malformed input.
Explicitly handle cases where jailbreak_score is missing or not a float, such as by logging or raising an error, to avoid unintended behavior.
Suggested implementation:
jailbreak_score = context.get("jailbreak_score")
if jailbreak_score is None:
# Log missing jailbreak_score
logger.warning("Missing jailbreak_score in context for prompt decision")
else:
try:
score = float(jailbreak_score)
if score > 0.8:
return DecisionResult(
allowed=True,
action="safe_mode",
reasons=["high jailbreak score"],
)
except (TypeError, ValueError):
logger.warning(f"Malformed jailbreak_score in context: {jailbreak_score}")- Ensure that a
loggeris available in this module. If not, you may need to import and configure Python's logging module at the top of the file:import logging logger = logging.getLogger(__name__)
- If you prefer to raise an error instead of logging, replace the
logger.warninglines withraise ValueError(...). - Adjust the logging level and error handling to match your application's conventions.
|
|
||
| def test_prompt_safe_mode_when_jailbreak_high(): | ||
| service = create_service() | ||
| request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.9}) | ||
| result = service.decide_prompt(request) | ||
| assert result.allowed is True | ||
| assert result.action == "safe_mode" | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Missing test for jailbreak_score at boundary values.
Add tests for 'jailbreak_score' at 0.8 and 0.79 to verify correct threshold behavior for 'safe_mode'.
| def test_prompt_safe_mode_when_jailbreak_high(): | |
| service = create_service() | |
| request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.9}) | |
| result = service.decide_prompt(request) | |
| assert result.allowed is True | |
| assert result.action == "safe_mode" | |
| def test_prompt_safe_mode_when_jailbreak_high(): | |
| service = create_service() | |
| request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.9}) | |
| result = service.decide_prompt(request) | |
| assert result.allowed is True | |
| assert result.action == "safe_mode" | |
| def test_prompt_safe_mode_when_jailbreak_at_threshold(): | |
| service = create_service() | |
| request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.8}) | |
| result = service.decide_prompt(request) | |
| assert result.allowed is True | |
| assert result.action == "safe_mode" | |
| def test_prompt_no_safe_mode_when_jailbreak_below_threshold(): | |
| service = create_service() | |
| request = PromptDecisionInput(prompt="hi", context={"jailbreak_score": 0.79}) | |
| result = service.decide_prompt(request) | |
| assert result.allowed is True | |
| assert result.action != "safe_mode" |
| def test_ci_check_detects_violations(): | ||
| cfg = { | ||
| "thresholds": { | ||
| "quality": {"pass_at_5": {"target": 0.8}}, | ||
| "fairness": {"subgroup_delta": {"target_max": 0.2}}, | ||
| "safety": {"harmful_rate": {"target_max": 0.1}}, | ||
| } | ||
| } | ||
| service = create_service(cfg) | ||
| request = CiCheckInput( |
There was a problem hiding this comment.
suggestion (testing): Missing test for partial CI check violations.
Please add tests where only a subset of CI check metrics fail, to ensure the service reports violations accurately.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of refactoring the policy gateway into a hexagonal architecture. The separation of concerns is clear, and the introduction of application services, ports, and adapters greatly improves the maintainability and testability of the code. The addition of unit tests is also a fantastic step forward. My review includes a few suggestions to further enhance the new architecture, primarily focusing on improving error handling in the configuration adapter to prevent silent failures, and pointing out a potential logic change in the CI checks that could make them less strict.
| if fairness_max is not None and request.fairness.get("subgroup_delta", 0) > fairness_max: | ||
| violations.append("fairness.subgroup_delta") | ||
|
|
||
| safety_max = self._lookup_threshold(thresholds, "safety", "harmful_rate", "target_max") | ||
| if safety_max is not None and request.safety.get("harmful_rate", 0) > safety_max: |
There was a problem hiding this comment.
The default values for subgroup_delta and harmful_rate have been changed from 1 to 0 when the metric is missing from the request. In the original implementation, a missing value defaulted to 1, which would likely cause the check to fail (e.g., 1 > target_max). The new implementation with a default of 0 will likely cause the check to pass. This is a significant change in behavior that makes the checks less strict. If this was unintentional, the defaults should be changed back to 1 to preserve the original logic.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
services/policy-gateway/src/policy_gateway/application/services.py (2)
22-23: Consider checking configuration availability in health endpoint.The health check currently returns a static response. For production readiness, consider verifying that the configuration port can successfully load data.
Example enhancement:
def health(self) -> Dict[str, str]: - return {"status": "ok"} + try: + self._configuration_port.load() + return {"status": "ok"} + except Exception: + return {"status": "degraded"}
29-43: Consider making policy thresholds configurable.The jailbreak score threshold (0.8) and verbatim ratio threshold (0.2 in
decide_output) are hardcoded, whileci_checkproperly loads its thresholds from configuration. For consistency and flexibility, consider loading these thresholds from the configuration port as well.Example refactor:
def decide_prompt(self, request: PromptDecisionInput) -> DecisionResult: + config = self._configuration_port.load() or {} + jailbreak_threshold = config.get("prompt_thresholds", {}).get("jailbreak_score", 0.8) context = request.context or {} if context.get("contains_pii") and not context.get("lawful_basis"): return DecisionResult( allowed=False, action="block", reasons=["PII without lawful basis"], ) - if (context.get("jailbreak_score") or 0) > 0.8: + if (context.get("jailbreak_score") or 0) > jailbreak_threshold: return DecisionResult( allowed=True, action="safe_mode", reasons=["high jailbreak score"], ) return DecisionResult(allowed=True, action="allow")services/policy-gateway/src/app.py (2)
49-68: Consider explicit response construction instead of dict unpacking.Using
**decision.__dict__assumes the dataclass fields exactly match the Pydantic model fields. While this works currently, it's fragile and could break silently if the models diverge.More explicit alternative:
def filter_prompt( body: PromptCheckRequest, service: PolicyDecisionService = Depends(get_service), ) -> DecisionResponse: decision = service.decide_prompt( PromptDecisionInput(prompt=body.prompt, context=body.context or {}) ) - return DecisionResponse(**decision.__dict__) + return DecisionResponse( + allowed=decision.allowed, + action=decision.action, + reasons=decision.reasons + )
71-84: CI check implementation follows the same pattern.Same suggestion as filter endpoints: consider explicit field mapping instead of dict unpacking for robustness.
Regarding the static analysis B008 warnings about
Depends()in default arguments: these are false positives. UsingDepends()in function parameter defaults is the official FastAPI pattern and is safe because FastAPI's dependency injection system evaluates the callable at request time, not at function definition time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
services/policy-gateway/src/app.py(1 hunks)services/policy-gateway/src/policy_gateway/__init__.py(1 hunks)services/policy-gateway/src/policy_gateway/application/services.py(1 hunks)services/policy-gateway/src/policy_gateway/domain/models.py(1 hunks)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py(1 hunks)services/policy-gateway/src/policy_gateway/interface/http/schemas.py(1 hunks)services/policy-gateway/src/policy_gateway/ports/configuration.py(1 hunks)services/policy-gateway/tests/test_policy_service.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
services/policy-gateway/src/policy_gateway/__init__.py (1)
services/policy-gateway/src/policy_gateway/application/services.py (1)
PolicyDecisionService(16-93)
services/policy-gateway/src/policy_gateway/ports/configuration.py (2)
services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
load(18-26)services/policy-gateway/tests/test_policy_service.py (1)
load(17-18)
services/policy-gateway/src/policy_gateway/application/services.py (3)
services/policy-gateway/src/policy_gateway/domain/models.py (6)
CiCheckInput(20-24)CiCheckResult(35-37)DecisionResult(28-31)OutputDecisionInput(14-16)PromptDecisionInput(8-10)RulesSnapshot(41-45)services/policy-gateway/src/policy_gateway/ports/configuration.py (2)
ConfigurationPort(6-12)load(9-12)services/policy-gateway/src/app.py (1)
ci_check(72-84)
services/policy-gateway/tests/test_policy_service.py (3)
services/policy-gateway/src/policy_gateway/application/services.py (4)
PolicyDecisionService(16-93)decide_prompt(29-43)decide_output(45-53)ci_check(55-76)services/policy-gateway/src/policy_gateway/domain/models.py (3)
CiCheckInput(20-24)OutputDecisionInput(14-16)PromptDecisionInput(8-10)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (2)
ConfigFileAdapter(12-26)load(18-26)
services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
services/policy-gateway/src/policy_gateway/ports/configuration.py (2)
ConfigurationPort(6-12)load(9-12)
services/policy-gateway/src/app.py (4)
services/policy-gateway/src/policy_gateway/application/services.py (6)
PolicyDecisionService(16-93)health(22-23)rules(25-27)decide_prompt(29-43)decide_output(45-53)ci_check(55-76)services/policy-gateway/src/policy_gateway/domain/models.py (4)
CiCheckInput(20-24)OutputDecisionInput(14-16)PromptDecisionInput(8-10)to_dict(44-45)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
ConfigFileAdapter(12-26)services/policy-gateway/src/policy_gateway/interface/http/schemas.py (5)
CiCheckRequest(18-22)CiCheckResponse(31-33)DecisionResponse(25-28)OutputCheckRequest(13-15)PromptCheckRequest(8-10)
🪛 Ruff (0.14.4)
services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py
25-25: Do not catch blind exception: Exception
(BLE001)
services/policy-gateway/src/app.py
40-40: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
45-45: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
52-52: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
63-63: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
74-74: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (8)
services/policy-gateway/src/policy_gateway/application/services.py (3)
16-20: LGTM! Clean dependency injection pattern.The constructor properly implements the hexagonal architecture pattern by accepting the configuration port abstraction, enabling testability and flexibility.
25-27: LGTM! Proper defensive coding.Good use of
or {}to handle potential None returns from the configuration port.
78-93: LGTM! Robust threshold extraction.The helper properly handles missing keys, None values, and type conversion errors. The defensive programming ensures the CI check won't crash on malformed configuration.
services/policy-gateway/src/policy_gateway/domain/models.py (1)
1-45: Excellent domain model design!The dataclasses are well-structured with:
- Immutability via
frozen=Truepreventing accidental mutations- Proper use of
field(default_factory=...)to avoid mutable default pitfalls- Clear type hints and separation of concerns
- Simple, focused models aligned with hexagonal architecture principles
services/policy-gateway/src/app.py (4)
26-32: LGTM! Standard FastAPI service initialization.The module-level service instance is a common pattern in FastAPI. Note that configuration is loaded once at startup, so configuration changes require a service restart.
35-36: LGTM! Clean dependency injection.This pattern enables easy testing by allowing the service dependency to be overridden.
39-41: LGTM!Clean delegation to the service layer.
44-46: LGTM!Properly converts the RulesSnapshot to a dictionary for the HTTP response.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…s.py Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…ig_file_adapter.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…s.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ig_file_adapter.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…pdate demo steps and prerequisites
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
23-40: Fail-open: invalid config silently disables policy enforcement.Returning
{}on read/parse errors means any malformed YAML/JSON (or unreadable file) causes the gateway to run with zero policies, letting CI checks and filters fail open. We need to surface these failures so the service can abort or alert operators. Please let IO/parsing exceptions propagate (or re-raise with context) instead of swallowing them.Suggested fix:
if not self._path.exists(): return {} - try: - raw = self._path.read_text(encoding="utf-8") - except OSError: - # If the file cannot be read, treat it as missing/empty for safety. - return {} + try: + raw = self._path.read_text(encoding="utf-8") + except OSError as exc: + raise RuntimeError(f"Unable to read configuration file {self._path}") from exc if self._path.suffix in {".yml", ".yaml"}: try: - return yaml.safe_load(raw) or {} - except yaml.YAMLError: - return {} + return yaml.safe_load(raw) or {} + except yaml.YAMLError as exc: + raise ValueError(f"Invalid YAML in {self._path}") from exc try: return json.loads(raw) - except json.JSONDecodeError: - return {} + except json.JSONDecodeError as exc: + raise ValueError(f"Invalid JSON in {self._path}") from exc
🧹 Nitpick comments (4)
README.md (2)
249-249: Capitalize "GitHub" in text references.GitHub has a capital letter mid-word and should be capitalized when referencing the platform or its services in text. Line 249 currently shows
[AI Agent Instructions](.github/copilot-instructions.md)— while the directory name.githubis correct, the accompanying text should reference "GitHub" when describing it.Update to:
-| [AI Agent Instructions](.github/copilot-instructions.md) | AI Assistants | Architecture patterns, workflows | +| [AI Agent Instructions](.github/copilot-instructions.md) | AI Assistants | GitHub integration—architecture patterns and workflows |
358-358: Capitalize "GitHub" in development setup reference.GitHub should be capitalized when referencing the platform in documentation text. Line 358 reads:
**Development setup:** See [.github/copilot-instructions.md](...— update the accompanying text to capitalize "GitHub."Update to:
-**Development setup:** See [`.github/copilot-instructions.md`](.github/copilot-instructions.md) for architecture patterns, workflows, and conventions. +**Development setup:** See the [copilot-instructions.md](.github/copilot-instructions.md) file in the GitHub repository for architecture patterns, workflows, and conventions.scripts/dev_setup.sh (2)
25-25: Add validation that PYTHONPATH directories exist.Line 25 exports PYTHONPATH assuming
services/policy-gateway/srcexists at$PWD. If the script is run from an unexpected location (e.g., a subdirectory) or the directory is missing, the PYTHONPATH will be set incorrectly without notice, potentially causing silent import failures later.Additionally, the script assumes execution from the repository root, but this is not documented.
Consider adding validation and a clear prerequisite comment:
+# Ensure the script is run from the repository root +if [ ! -d "services/policy-gateway/src" ]; then + echo "Error: services/policy-gateway/src not found. Run this script from the repository root." >&2 + exit 1 +fi + export PYTHONPATH="$PWD/services/policy-gateway/src:$PYTHONPATH"
36-37: Consider using verbose pytest output for developer experience.The
-q(quiet) flag on pytest (line 37) suppresses most output, which may be surprising to developers running the setup script interactively. A more informative output would help developers understand which tests are running and which passed/failed.Consider removing
-qor making it optional:-python3 -m pytest services/policy-gateway/tests -q +python3 -m pytest services/policy-gateway/testsAlternatively, use
-vfor verbose output if debugging is needed:-python3 -m pytest services/policy-gateway/tests -q +python3 -m pytest services/policy-gateway/tests -v
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
devbox.lockis excluded by!**/*.lockservices/policy-gateway/src/__pycache__/app.cpython-312.pycis excluded by!**/*.pycservices/policy-gateway/src/policy_gateway/domain/__pycache__/models.cpython-312.pycis excluded by!**/*.pycservices/policy-gateway/src/policy_gateway/infrastructure/__pycache__/config_file_adapter.cpython-312.pycis excluded by!**/*.pycservices/policy-gateway/src/policy_gateway/ports/__pycache__/configuration.cpython-312.pycis excluded by!**/*.pycservices/policy-gateway/tests/__pycache__/test_http_responses.cpython-312.pycis excluded by!**/*.pycservices/policy-gateway/tests/__pycache__/test_policy_service.cpython-312.pycis excluded by!**/*.pyc
📒 Files selected for processing (11)
.github/copilot-instructions.md(2 hunks)README.md(7 hunks)scripts/dev_setup.sh(1 hunks)services/policy-gateway/requirements-dev.txt(1 hunks)services/policy-gateway/src/app.py(1 hunks)services/policy-gateway/src/policy_gateway/application/services.py(1 hunks)services/policy-gateway/src/policy_gateway/domain/models.py(1 hunks)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py(1 hunks)services/policy-gateway/src/policy_gateway/ports/configuration.py(1 hunks)services/policy-gateway/tests/test_http_responses.py(1 hunks)services/policy-gateway/tests/test_policy_service.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
services/policy-gateway/src/policy_gateway/application/services.py (4)
services/policy-gateway/src/policy_gateway/domain/models.py (6)
CiCheckInput(20-24)CiCheckResult(47-56)DecisionResult(28-43)OutputDecisionInput(14-16)PromptDecisionInput(8-10)RulesSnapshot(60-64)services/policy-gateway/src/policy_gateway/ports/configuration.py (2)
ConfigurationPort(6-11)load(9-11)services/policy-gateway/src/app.py (3)
health(39-40)rules(44-45)ci_check(72-84)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
load(17-40)
services/policy-gateway/src/policy_gateway/ports/configuration.py (2)
services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
load(17-40)services/policy-gateway/tests/test_policy_service.py (1)
load(21-22)
services/policy-gateway/src/app.py (4)
services/policy-gateway/src/policy_gateway/application/services.py (6)
PolicyDecisionService(16-93)health(22-23)rules(25-27)decide_prompt(29-43)decide_output(45-53)ci_check(55-76)services/policy-gateway/src/policy_gateway/domain/models.py (5)
CiCheckInput(20-24)OutputDecisionInput(14-16)PromptDecisionInput(8-10)to_response(33-43)to_response(51-56)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
ConfigFileAdapter(11-40)services/policy-gateway/src/policy_gateway/interface/http/schemas.py (5)
CiCheckRequest(18-22)CiCheckResponse(31-33)DecisionResponse(25-28)OutputCheckRequest(13-15)PromptCheckRequest(8-10)
services/policy-gateway/tests/test_policy_service.py (4)
services/policy-gateway/src/policy_gateway/application/services.py (4)
PolicyDecisionService(16-93)decide_prompt(29-43)decide_output(45-53)ci_check(55-76)services/policy-gateway/src/policy_gateway/domain/models.py (3)
CiCheckInput(20-24)OutputDecisionInput(14-16)PromptDecisionInput(8-10)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (2)
ConfigFileAdapter(11-40)load(17-40)services/policy-gateway/src/policy_gateway/ports/configuration.py (2)
ConfigurationPort(6-11)load(9-11)
services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (2)
services/policy-gateway/src/policy_gateway/ports/configuration.py (2)
ConfigurationPort(6-11)load(9-11)services/policy-gateway/tests/test_policy_service.py (1)
load(21-22)
services/policy-gateway/tests/test_http_responses.py (2)
services/policy-gateway/src/policy_gateway/domain/models.py (4)
CiCheckResult(47-56)DecisionResult(28-43)to_response(33-43)to_response(51-56)services/policy-gateway/src/policy_gateway/interface/http/schemas.py (2)
CiCheckResponse(31-33)DecisionResponse(25-28)
🪛 LanguageTool
README.md
[uncategorized] ~249-~249: The official name of this software platform is spelled with a capital “H”.
Context: ...e walkthrough | | AI Agent Instructions | AI Assistant...
(GITHUB)
[uncategorized] ~358-~358: The official name of this software platform is spelled with a capital “H”.
Context: ...RAMP, etc.) Development setup: See [.github/copilot-instructions.md](.github/copil...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
.github/copilot-instructions.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.4)
services/policy-gateway/src/app.py
39-39: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
44-44: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
51-51: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
63-63: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
74-74: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (20)
.github/copilot-instructions.md (4)
23-25: Documentation clearly articulates hexagonal architecture principles.The dependency flow and key principle that domain logic remains framework-agnostic align well with the PR objectives. This provides strong guidance for future contributors.
28-48: Domain-to-HTTP conversion pattern guidance is clear and actionable.Explicit field mapping example vs. anti-pattern clearly demonstrates why decoupling domain models from HTTP schemas matters. This directly supports the refactoring goals in the PR.
177-177: Verify external reference accuracy.Line 177 references a "Fix needed" for
services/policy-gateway/src/policy_gateway/ports/configuration.pyline 9-12 regarding Protocol method stubs. Please confirm this guidance is accurate and still applies after the refactoring, or clarify if this was already resolved.
181-181: Verify external reference accuracy.Line 181 references a "Known issue" about duplicate code in
ConfigFileAdapter.load(). Please confirm this duplicate code still exists in the current implementation or remove/update this guidance if it has been resolved during the refactoring.README.md (8)
59-99: Architecture diagram and components section look good.The restructured ASCII art diagram clearly communicates the developer workflow, and the Core Components table accurately reflects the hexagonal architecture introduced in this PR. The sidecar pattern description aligns with the Policy Gateway refactor objectives.
116-127: Clarify devbox dependency in Quick Start.Line 116 introduces
devbox shellas a prerequisite for the Local Demo workflow, but devbox is listed as "Recommended" (line 107), not "Required." This could confuse users without devbox installed.Either:
- Make devbox "Required" in prerequisites and ensure installation instructions are clear, or
- Add an alternative path for users without devbox (e.g., "devbox shell # or ensure Python 3.11+ is in PATH")
Additionally, please verify that the artifact JSON structure (lines 122–127) matches the schema expected by the
just ci-checkcommand andpac_ci.pypolicy checker.
135-142: Verify curl example request/response accuracy against implemented API.The new "Test the policy gateway" section introduces a curl example that should be validated against the actual PolicyDecisionService HTTP schemas introduced in this PR.
Please confirm that:
- The
/filter/promptendpoint and request schema ({"prompt":"test","context":{"jailbreak_score":0.9}}) match the Pydantic models ininterface/http/schemas.py- The response structure (
{"allowed":true,"action":"safe_mode","reasons":[...]}) matches the actual HTTP response model- Expected output threshold logic (jailbreak score 0.9) aligns with the decision logic in
PolicyDecisionServiceIf schemas differ, update the curl example to match the actual API contract.
147-166: Verify ADR-006 ConfigMap deployment approach aligns with ConfigFileAdapter implementation.The Production Deployment section describes creating an ADR-006 ConfigMap from
policies/adr-006.embedded-governance.yaml(lines 147–150). Given the PR introduces aConfigFileAdapterfor file-based configuration, please confirm:
- The ConfigMap mounting strategy shown here is the intended deployment mechanism for the adapter
- The path
policies/adr-006.embedded-governance.yamlis the expected ConfigMap source- The reference to
docs/PROJECT_STATE.md(line 161) for production hardening is accurate and the file exists
170-216: Governance examples are clear and compelling.The fairness gate and runtime prompt filtering examples effectively communicate the value proposition. Ensure that the response structure shown in lines 208–212 matches the actual PolicyDecisionService response format.
244-256: Verify documentation file existence and update GitHub references.Lines 246–256 reference multiple documentation files. Please confirm these files exist in the repository:
docs/IAGPM_GenAI_Handbook/Quick_Start_Guide.md.github/copilot-instructions.mddocs/IAGPM_GenAI_Handbook/Reference.mddocs/IAGPM_GenAI_Handbook/Technical/llmops_reference_runbook.mddocs/IAGPM_GenAI_Handbook/Technical/policy_as_code_starter.mddocs/PROJECT_STATE.mdtests/TESTING_GUIDE.mdAdditionally, per static analysis guidance, the capitalization of "GitHub" should be corrected in text references (currently
.github— note that the directory name is correct, but references to the GitHub platform in accompanying text should use "GitHub" not ".github").
349-359: Clarify development setup documentation path.Line 358 references
.github/copilot-instructions.mdfor "architecture patterns, workflows, and conventions." This file should be verified to exist and contain the development guidance described. Additionally, ensure the path.github/copilot-instructions.mdis correctly formatted if it's an internal reference.
403-439: Verify justfile commands and Core APIs table accuracy.The new "Just Commands" section (lines 408–411) references four automation shortcuts. Please verify these commands exist and work as documented:
just ci-checkjust deploy-configjust deploy-resjust deploy-vllm-with-gatewayAdditionally, the "Core APIs" table (lines 430–439) documents six endpoints across two services. Please confirm that all endpoints are implemented with the documented purposes:
Policy Gateway:
POST /filter/prompt– Runtime prompt filteringPOST /filter/output– Runtime output filteringPOST /ci/check– Pre-merge threshold validationRisk & Evidence Service (RES):
POST /evidence– Submit audit artifactsGET /risk/snapshot– Current compliance stateGET /metrics– Prometheus metricsThese should align with the PolicyDecisionService implementation and HTTP schemas introduced in this PR.
services/policy-gateway/src/app.py (2)
25-35: Clean hexagonal architecture initialization.The service builder pattern with a module-level singleton and DI accessor is well-structured for FastAPI applications.
38-84: FastAPI dependency injection is correctly implemented.The static analysis warnings (B008) about
Depends()in default arguments are false positives. FastAPI is specifically designed to useDepends()in function parameter defaults as its idiomatic dependency injection pattern, and this is the recommended approach per FastAPI documentation.The refactored endpoints properly:
- Delegate business logic to the service layer
- Use typed domain models for transformation
- Declare explicit response models for OpenAPI documentation
- Maintain clean separation between HTTP and domain layers
services/policy-gateway/src/policy_gateway/application/services.py (4)
16-27: LGTM: Clean service initialization and configuration loading.The constructor properly accepts the configuration port, and the
rules()method defensively handles missing configuration with theor {}fallback.
29-43: Approve the prompt decision logic with existing caveats.The decision flow is clear and correctly structured. Previous reviews have noted concerns about the truthiness check for
lawful_basisand the default value forjailbreak_score, which are valid points to keep in mind.
45-53: Output decision logic is consistent and clear.The implementation follows the same pattern as
decide_prompt()with defensive defaults.
78-93: Excellent defensive threshold extraction.The
_lookup_threshold()method properly handles missing keys, invalid types, and conversion errors with appropriate None returns, enabling safe threshold evaluation inci_check().services/policy-gateway/requirements-dev.txt (2)
1-7: Dependencies align with PR objectives.The included packages appropriately support the hexagonal architecture refactor: FastAPI/Pydantic/Uvicorn for the service layer, pytest/pytest-cov for testing, ruff for code quality, and HTTP clients for integration tests.
5-6: Remove unused HTTP client dependencies from requirements-dev.txt.Neither
requestsnorhttpxis used in the codebase. The test suite uses FastAPI'sTestClient(fromfastapi.testclient) for HTTP endpoint testing. Remove both lines 5-6 to eliminate unnecessary dependencies.
| ``` | ||
| ports/ → Interfaces (ConfigurationPort Protocol) | ||
| application/ → Service layer (PolicyDecisionService) | ||
| domain/ → Models (DecisionResult, CiCheckInput, etc.) | ||
| infrastructure/ → Adapters (ConfigFileAdapter) | ||
| interface/ → HTTP schemas (Pydantic models) | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
Line 15 starts a fenced code block without specifying a language. This violates markdown lint rules (MD040) and reduces syntax highlighting in editors and documentation renderers.
-```
+```yaml
ports/ → Interfaces (ConfigurationPort Protocol)
application/ → Service layer (PolicyDecisionService)
domain/ → Models (DecisionResult, CiCheckInput, etc.)
infrastructure/ → Adapters (ConfigFileAdapter)
interface/ → HTTP schemas (Pydantic models)
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
.github/copilot-instructions.md around lines 15 to 21: the fenced code block
starting at line 15 lacks a language identifier which triggers MD040 and
prevents syntax highlighting; update the opening fence to include a language
(e.g., "yaml") so it becomes ```yaml and leave the block contents unchanged.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| @@ -0,0 +1,10 @@ | |||
| fastapi>=0.95.0 | |||
| pydantic>=1.10.0 | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify Pydantic version constraint.
The constraint pydantic>=1.10.0 allows both Pydantic v1 and v2, which have breaking API changes. Confirm whether supporting both versions is intentional or if the constraint should be tightened (e.g., pydantic>=1.10.0,<2.0.0 for v1-only, or pydantic>=2.0.0 for v2-only).
🌐 Web query:
What breaking changes were introduced in Pydantic v2 compared to v1?
💡 Result:
Short summary of the main breaking changes in Pydantic v2 vs v1:
- Core rewrite: validation moved into the new pydantic-core (Rust) package — semantics, performance, and some behaviors changed. [3][1]
- Renamed BaseModel API: many methods/attributes renamed to model_* or pydantic*_ (e.g., parse_obj → model_validate, construct → model_construct, copy → model_copy, dict → model_dump, json → model_dump_json, json_schema → model_json_schema, fields → model_fields). Old names may be deprecated/removed. [1]
- Equality and representation: model equality semantics changed (models are no longer equal to dicts) and other repr/eq behaviors were adjusted. [3][1]
- Validators and hooks API changed: validator registration, internals (e.g., validators, pydantic_validator) and lifecycle hooks differ; many v1 validator patterns were replaced. [1]
- Dataclass behavior: Pydantic/vanilla dataclasses changed (no tuple validation for dataclass fields, post_init runs after validation, no pydantic_model backing, extra='allow' removed for dataclasses); recommend using TypeAdapter for dataclass validation/JSON schema. [1]
- from_orm & parsing: from_orm removed; use model_validate with from_attributes=True. Some parse_* helpers deprecated (parse_raw/parse_file). [3][1]
- Error format and validation details: ValidationError structure/messages/fields changed (msg/ctx/loc), and some error fields or formats differ — code that parsed v1 error messages may break. [2][1]
- Typing/annotation and schema differences: how schemas are generated and how certain typing constructs are interpreted changed (TypeAdapter introduced; JSON Schema ref formatting may differ). [1][2]
- Migration tooling & compatibility: v1 docs and a v1 compatibility module exist (pydantic.v1), and a migration guide + codemod tool are provided because the upgrade is non-trivial. [1][2]
For details and the full migration notes (including exact renamed symbols, dataclass examples, and recommended fixes) see the official migration guide and version-compatibility notes. [1][2][3]
Sources: Pydantic migration guide and docs. [1] Migration Guide; [2] Version Compatibility / Version Policy; [3] Pydantic v2 announcement.
I need to verify how Pydantic is actually used in the codebase to assess the urgency of this constraint issue.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Pydantic usage in policy-gateway service
echo "=== Pydantic imports in policy-gateway ==="
find services/policy-gateway -type f -name "*.py" 2>/dev/null | head -20 | xargs grep -l "pydantic" 2>/dev/null || echo "No .py files found"
echo -e "\n=== Main requirements.txt or setup.py for policy-gateway ==="
find services/policy-gateway -type f \( -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" \) 2>/dev/null | head -10
echo -e "\n=== Content of requirements-dev.txt ==="
cat services/policy-gateway/requirements-dev.txt 2>/dev/null || echo "File not found"
Length of output: 458
---
🏁 Script executed:
```shell
#!/bin/bash
# Check main requirements.txt
echo "=== Content of main requirements.txt ==="
cat services/policy-gateway/requirements.txt 2>/dev/null
echo -e "\n=== Pydantic usage in schemas.py (full content or key patterns) ==="
cat services/policy-gateway/src/policy_gateway/interface/http/schemas.py 2>/dev/null | head -100
Length of output: 839
I need to verify what Pydantic version FastAPI 0.115.0 actually requires to ensure requirements-dev.txt is compatible.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check FastAPI 0.115.0 dependencies - search for what Pydantic version it requires
# Using pip index to query package metadata
python3 -c "
import json
import urllib.request
# Fetch FastAPI 0.115.0 metadata from PyPI
url = 'https://pypi.org/pypi/fastapi/0.115.0/json'
try:
with urllib.request.urlopen(url) as response:
data = json.loads(response.read().decode())
requires = data['info'].get('requires_dist', [])
print('FastAPI 0.115.0 dependencies:')
for req in requires:
if 'pydantic' in req.lower():
print(f' {req}')
except Exception as e:
print(f'Error: {e}')
" 2>&1
Length of output: 134
---
🌐 Web query:
```web
FastAPI 0.115.0 pydantic version requirement dependencies
💡 Result:
FastAPI 0.115.0 requires Pydantic v2 (i.e. pydantic >= 2.x). See the FastAPI 0.115.0 release notes and ecosystem examples noting Pydantic 2 as the required major version. [1][2]
Sources:
- FastAPI release notes (0.115.0). [1]
- Example dependency guidance showing "pydantic >= 2.x required for fastapi >= 0.113.0". [2]
Tighten Pydantic constraint to Pydantic v2 to match production requirement.
FastAPI 0.115.0 requires Pydantic v2, which is pinned in the main requirements.txt via fastapi==0.115.0. The current dev constraint pydantic>=1.10.0 is unconstrained and allows Pydantic v1, creating version skew between development and production environments.
Change to:
pydantic>=2.0.0,<3.0.0
This ensures dev dependencies align with the production constraint imposed by FastAPI.
🤖 Prompt for AI Agents
In services/policy-gateway/requirements-dev.txt around line 2, the pydantic
constraint is too loose (pydantic>=1.10.0); update that line to pin Pydantic v2
by replacing it with pydantic>=2.0.0,<3.0.0 so dev dependencies match production
FastAPI's Pydantic v2 requirement.
| def ci_check(self, request: CiCheckInput) -> CiCheckResult: | ||
| config = self._configuration_port.load() or {} | ||
| thresholds: Dict[str, Any] = config.get("thresholds", {}) | ||
|
|
||
| violations: List[str] = [] | ||
|
|
||
| quality_target = self._lookup_threshold(thresholds, "quality", "pass_at_5", "target") | ||
| if quality_target is not None and request.quality.get("pass_at_5", 0) < quality_target: | ||
| violations.append("quality.pass_at_5") | ||
|
|
||
| fairness_max = self._lookup_threshold( | ||
| thresholds, "fairness", "subgroup_delta", "target_max" | ||
| ) | ||
| if fairness_max is not None and request.fairness.get("subgroup_delta", 0) > fairness_max: | ||
| violations.append("fairness.subgroup_delta") | ||
|
|
||
| safety_max = self._lookup_threshold(thresholds, "safety", "harmful_rate", "target_max") | ||
| if safety_max is not None and request.safety.get("harmful_rate", 0) > safety_max: | ||
| violations.append("safety.harmful_rate") | ||
|
|
||
| status = "fail" if violations else "pass" | ||
| return CiCheckResult(status=status, violations=violations) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that drift evaluation is implemented.
The ci_check() method evaluates quality, fairness, and safety thresholds but does not evaluate the drift field from CiCheckInput. A previous review comment indicated this was addressed in commits 75e93ff-e6a1d44, but the evaluation is not visible in the current code.
Run this script to confirm whether drift evaluation logic is present:
🏁 Script executed:
#!/bin/bash
# Description: Search for drift evaluation in the ci_check method
# Look for drift threshold lookups or drift violation checks
rg -n -B2 -A5 'drift' services/policy-gateway/src/policy_gateway/application/services.py
# Also check if there are any drift-related threshold lookups
ast-grep --pattern 'self._lookup_threshold($$$, "drift", $$$)'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Description: Comprehensive search for drift in services.py and related context
# Check file exists and search more broadly
echo "=== Checking file exists ==="
ls -la services/policy-gateway/src/policy_gateway/application/services.py
echo -e "\n=== Search entire services.py for 'drift' (case-insensitive) ==="
rg -in 'drift' services/policy-gateway/src/policy_gateway/application/services.py
echo -e "\n=== Find CiCheckInput class definition ==="
ast-grep --pattern 'class CiCheckInput { $$$ }'
echo -e "\n=== Check for DataClass with CiCheckInput ==="
rg -n 'CiCheckInput' services/policy-gateway/src/policy_gateway/application/services.py -B5 -A15
echo -e "\n=== Search for configuration/config files with drift thresholds ==="
find services/policy-gateway -name '*.py' -o -name '*.json' -o -name '*.yaml' -o -name '*.yml' | head -20Length of output: 2476
🏁 Script executed:
#!/bin/bash
# Description: Examine CiCheckInput definition and drift configuration
echo "=== CiCheckInput definition in models.py ==="
rg -n 'class CiCheckInput' services/policy-gateway/src/policy_gateway/domain/models.py -A20
echo -e "\n=== Search for 'drift' in models.py ==="
rg -in 'drift' services/policy-gateway/src/policy_gateway/domain/models.py
echo -e "\n=== Search for 'drift' in all Python files under services/policy-gateway ==="
rg -in 'drift' services/policy-gateway --type=py
echo -e "\n=== Check test files for drift expectations ==="
rg -in 'drift' services/policy-gateway/tests/ --type=py -B2 -A2Length of output: 2830
🏁 Script executed:
#!/bin/bash
# Description: Check for drift thresholds in configuration
echo "=== Check test_policy_service for drift threshold expectations ==="
rg -in 'drift' services/policy-gateway/tests/test_policy_service.py -B5 -A5
echo -e "\n=== Check config_file_adapter for drift handling ==="
cat services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py
echo -e "\n=== Check for any configuration files (json/yaml) ==="
find services/policy-gateway -type f \( -name '*.json' -o -name '*.yaml' -o -name '*.yml' \)
echo -e "\n=== Check if there are drift thresholds in test configuration ==="
rg -n 'thresholds' services/policy-gateway/tests/test_policy_service.py -B2 -A10Length of output: 3135
Add drift evaluation to the ci_check() method.
The ci_check() method currently evaluates quality, fairness, and safety thresholds but ignores the drift field from CiCheckInput. The drift field is defined in the data model and accepted by the HTTP endpoint, but it is never validated. Add drift threshold checks following the same pattern as the other metrics (lines 61-69).
🤖 Prompt for AI Agents
In services/policy-gateway/src/policy_gateway/application/services.py around
lines 55 to 76, add a drift threshold check analogous to the existing fairness
and safety checks: call self._lookup_threshold(thresholds, "drift", "psi",
"target_max") (or the correct drift metric name if different), store it to
drift_max, then if drift_max is not None and request.drift.get("psi", 0) >
drift_max append "drift.psi" to violations; place this check before computing
status so drift violations are included in the CiCheckResult.
| def test_config_file_adapter_invalid_yaml(tmp_path: Path): | ||
| # Write invalid YAML content | ||
| cfg_path = tmp_path / "invalid.yaml" | ||
| cfg_path.write_text("foo: [unclosed_list\nbar: baz") | ||
|
|
||
| adapter = ConfigFileAdapter(cfg_path) | ||
| assert adapter.load() == {} | ||
|
|
||
|
|
||
| def test_config_file_adapter_invalid_json(tmp_path: Path): | ||
| # Write invalid JSON content | ||
| cfg_path = tmp_path / "invalid.json" | ||
| cfg_path.write_text('{"foo": "bar", "baz": [1, 2,}') # Invalid JSON | ||
|
|
||
| adapter = ConfigFileAdapter(cfg_path) | ||
| assert adapter.load() == {} |
There was a problem hiding this comment.
Surface configuration parse failures instead of asserting fail-open behavior.
These tests require ConfigFileAdapter.load() to swallow invalid YAML/JSON and return {}. That expectation pushes the adapter toward a fail-open posture: if a policy file is malformed, the service proceeds with an empty config and skips all thresholds. Please change the tests to expect an exception (e.g., pytest.raises(ValueError)) so we fail fast and make configuration errors visible. Update both the YAML and JSON cases accordingly.
🤖 Prompt for AI Agents
In services/policy-gateway/tests/test_policy_service.py around lines 134 to 149,
the tests currently assert that ConfigFileAdapter.load() returns {} on invalid
YAML/JSON which enforces a fail-open behavior; update both tests to expect a
parsing exception instead (use pytest.raises(ValueError) or the specific
exception your loader raises) so malformed configs fail fast; ensure pytest is
imported if not already, wrap the adapter.load() call with pytest.raises(...) in
both the YAML and JSON test cases, and adjust any test names/docstrings to
reflect the new expectation.
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: true
poem: false
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
.github/workflows/ci.yml (1)
31-46: Consider using $GITHUB_PATH for cleaner virtual environment persistence.The review comment correctly identifies that venv activation doesn't persist across steps. However, the proposed solutions (re-activation or absolute paths) are not the most idiomatic GitHub Actions pattern.
Each step runs in its own shell/process, so activation modifications don't carry forward. The recommended approach is to add the venv's bin directory to
$GITHUB_PATHin the initial setup step, which automatically makes the venv Python available to all subsequent steps:- name: Install dependencies run: | python -m pip install --upgrade pip curl -LsSf https://astral.sh/uv/install.sh | sh || true + # Create venv and add to PATH for all subsequent steps if command -v uv >/dev/null 2>&1; then uv venv .venv - source .venv/bin/activate pip install -r services/policy-gateway/requirements-dev.txt else python -m venv .venv - source .venv/bin/activate pip install -r services/policy-gateway/requirements-dev.txt fi + echo "${{ github.workspace }}/.venv/bin" >> $GITHUB_PATHThis eliminates the need to modify linter and test steps. Alternatively, the proposed re-activation or absolute-path approaches will also work.
tmp_cfg2.yaml (1)
1-4: Consider renaming or relocating this configuration file.The
tmp_cfg2.yamlfilename suggests this is a temporary test fixture. If this file is used for testing purposes, it should be moved to thetests/directory or renamed to clearly indicate its purpose (e.g.,test_threshold_config.yaml). If it's intended for production use, remove the "tmp" prefix and document its purpose.README.md (1)
59-519: Excellent documentation improvements with comprehensive coverage.The README updates significantly improve the project's documentation:
- Clear problem statement and business value proposition
- Well-structured quick start with concrete examples
- Detailed streaming endpoint usage with curl and JavaScript examples
- Production deployment guidance with checklists
- Comprehensive core APIs table
The streaming documentation (lines 186-214) is particularly helpful with multiple client examples.
Optional: Address minor formatting issues flagged by linters:
- Lines 164, 186: Convert setext headings to ATX style:
-Provider selection & streaming ------------------------------- +### Provider selection & streaming -Streaming endpoint (SSE) ------------------------- +### Streaming endpoint (SSE)
- Line 192: Remove spaces inside code span:
- - POST /proxy/completion/stream — returns `text/event-stream` chunks where each + - POST /proxy/completion/stream — returns `text/event-stream` chunks where each
- Lines 325, 434: Use capital 'H' in GitHub references per style guide (
.github→.githubis correct for paths, but the linter may be flagging text references).docs/Reference/LLMOps_Specs/ADR.md (1)
159-173: LGTM! Clear documentation of runtime adapter and streaming additions.The ADR update effectively documents the new architectural capabilities:
- Runtime provider selection via
PAC_LLM_PROVIDER- SSE streaming endpoint for low-latency responses
- Rationale for the changes (provider flexibility without code changes)
Fix the list indentation flagged by markdownlint:
- New environment variable: `PAC_LLM_PROVIDER` — controls which adapter the Policy Gateway uses at runtime. Supported values: - - `http` (default) — forward to an upstream vLLM-like HTTP endpoint - - `litellm` — use an in-process LiteLLM client adapter when available + - `http` (default) — forward to an upstream vLLM-like HTTP endpoint + - `litellm` — use an in-process LiteLLM client adapter when availableThis ensures consistent 2-space indentation for nested list items.
services/policy-gateway/tests/test_llm_proxy.py (2)
13-16: Narrow the exception type for JSON parsing.The bare
Exceptioncatch is overly broad. Usejson.JSONDecodeErrorto handle only JSON parsing failures.Apply this diff:
try: j = json.loads(body or "{}") - except Exception: + except json.JSONDecodeError: j = {}
47-53: Avoid runtime sys.path manipulation.Dynamically manipulating
sys.pathat runtime is fragile and depends on the test file's location in the directory structure. Consider using pytest'spythonpathconfiguration or ensuring the src directory is already on the path viaPYTHONPATHenvironment variable (as mentioned in the PR testing instructions).If the testing instructions already specify
PYTHONPATH=services/policy-gateway/src, these lines can be removed entirely:- # Ensure the app imports from the correct src path - repo_root = Path(__file__).resolve().parents[3] - src_dir = str(repo_root / "services" / "policy-gateway" / "src") - import sys - - sys.path.insert(0, src_dir) import app as gateway_app # type: ignoreservices/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py (1)
5-8: Narrow the exception type for import handling.The bare
except Exceptionis too broad for handling import failures. Useexcept ImportErrorto catch only module import errors.Apply this diff:
try: import litellm -except Exception: # pragma: no cover - litellm may not be installed in analysis env +except ImportError: # pragma: no cover - litellm may not be installed in analysis env litellm = Noneservices/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py (1)
26-44: Consider adding retry logic for transient failures.The adapter currently fails immediately on any exception. For production robustness, consider adding retry logic with exponential backoff for transient network errors or 5xx responses.
Example using
httpxwith retry:from httpx import Client, HTTPStatusError def complete(self, request: CompletionRequest) -> CompletionResponse: url = self.endpoint.rstrip("/") + "/completion" payload: Dict[str, object] = {"prompt": request.prompt} if request.model: payload["model"] = request.model if request.max_tokens is not None: payload["max_tokens"] = request.max_tokens # Simple retry logic for attempt in range(3): try: resp = httpx.post(url, json=payload, timeout=self.timeout) resp.raise_for_status() j = resp.json() break except (httpx.NetworkError, HTTPStatusError) as exc: if attempt == 2: # last attempt raise RuntimeError(f"LLM request failed: {exc}") from exc # exponential backoff: 0.1s, 0.2s time.sleep(0.1 * (2 ** attempt)) else: raise RuntimeError("LLM request failed after retries") content = j.get("content") or j.get("text") or json.dumps(j) return CompletionResponse( content=str(content), model=j.get("model"), usage=j.get("usage", {}) )services/policy-gateway/src/app.py (1)
43-55: Cache the LLM adapter to avoid repeated instantiation.The
_build_llm_adapter()function is called for every proxy request (lines 62, 84), creating a new adapter instance each time. For the HTTP adapter, this means recreating the httpx client for each request. Consider caching the adapter or using FastAPI's dependency injection with a singleton scope.Option 1: Use
functools.lru_cachefor simple caching:+from functools import lru_cache + +@lru_cache(maxsize=1) def _build_llm_adapter(): """Factory for LLM adapters. Selects implementation using PAC_LLM_PROVIDER.Option 2: Use FastAPI dependency with singleton pattern:
_llm_adapter_singleton = None def get_llm_adapter(): global _llm_adapter_singleton if _llm_adapter_singleton is None: _llm_adapter_singleton = _build_llm_adapter() return _llm_adapter_singleton @app.post("/proxy/completion", response_model=CompletionResponse) def proxy_completion( body: CompletionRequest, adapter = Depends(get_llm_adapter), ): # use adapter...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.coderabbit.yml(1 hunks).github/workflows/ci.yml(1 hunks).gitignore(1 hunks)README.md(7 hunks)docs/Reference/ENVIRONMENT.md(1 hunks)docs/Reference/LLMOps_Specs/ADR.md(1 hunks)docs/Reference/LLMOps_Specs/UPDATES.md(1 hunks)justfile(1 hunks)scripts/dev_setup.sh(1 hunks)services/policy-gateway/requirements-dev.txt(1 hunks)services/policy-gateway/src/app.py(1 hunks)services/policy-gateway/src/policy_gateway/domain/models.py(1 hunks)services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py(1 hunks)services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py(1 hunks)services/policy-gateway/src/policy_gateway/interface/http/schemas.py(1 hunks)services/policy-gateway/src/policy_gateway/ports/llm_adapter.py(1 hunks)services/policy-gateway/tests/test_integration.py(1 hunks)services/policy-gateway/tests/test_llm_proxy.py(1 hunks)services/policy-gateway/tests/test_streaming.py(1 hunks)specs/openapi-policy-gateway.yaml(7 hunks)tmp_cfg.yaml(1 hunks)tmp_cfg2.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tmp_cfg.yaml
- docs/Reference/LLMOps_Specs/UPDATES.md
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/dev_setup.sh
- services/policy-gateway/requirements-dev.txt
- services/policy-gateway/src/policy_gateway/interface/http/schemas.py
- services/policy-gateway/src/policy_gateway/domain/models.py
🧰 Additional context used
🧬 Code graph analysis (6)
services/policy-gateway/src/policy_gateway/ports/llm_adapter.py (5)
services/policy-gateway/src/policy_gateway/domain/models.py (2)
CompletionRequest(69-73)CompletionResponse(77-80)services/policy-gateway/src/policy_gateway/interface/http/schemas.py (2)
CompletionRequest(36-39)CompletionResponse(42-45)services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py (2)
complete(29-74)stream(76-98)services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py (2)
complete(26-44)stream(46-68)services/policy-gateway/tests/test_streaming.py (1)
stream(20-22)
services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py (3)
services/policy-gateway/src/policy_gateway/domain/models.py (2)
CompletionRequest(69-73)CompletionResponse(77-80)services/policy-gateway/src/policy_gateway/ports/llm_adapter.py (3)
LLMAdapterPort(8-21)complete(19-19)stream(21-21)services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py (2)
complete(26-44)stream(46-68)
services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py (3)
services/policy-gateway/src/policy_gateway/domain/models.py (2)
CompletionRequest(69-73)CompletionResponse(77-80)services/policy-gateway/src/policy_gateway/ports/llm_adapter.py (3)
LLMAdapterPort(8-21)complete(19-19)stream(21-21)services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py (2)
complete(29-74)stream(76-98)
services/policy-gateway/src/app.py (6)
services/policy-gateway/src/policy_gateway/application/services.py (6)
PolicyDecisionService(16-93)health(22-23)rules(25-27)decide_prompt(29-43)decide_output(45-53)ci_check(55-76)services/policy-gateway/src/policy_gateway/domain/models.py (8)
CiCheckInput(20-24)OutputDecisionInput(14-16)PromptDecisionInput(8-10)CompletionRequest(69-73)CompletionResponse(77-80)to_dict(63-64)to_response(33-43)to_response(51-56)services/policy-gateway/src/policy_gateway/infrastructure/config_file_adapter.py (1)
ConfigFileAdapter(11-40)services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py (3)
LiteLLMAdapter(14-98)complete(29-74)stream(76-98)services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py (3)
HTTPLLMAdapter(12-68)complete(26-44)stream(46-68)services/policy-gateway/src/policy_gateway/interface/http/schemas.py (7)
CiCheckRequest(18-22)CiCheckResponse(31-33)CompletionRequest(36-39)CompletionResponse(42-45)DecisionResponse(25-28)OutputCheckRequest(13-15)PromptCheckRequest(8-10)
services/policy-gateway/tests/test_integration.py (1)
services/policy-gateway/tests/test_streaming.py (1)
_import_app(7-13)
services/policy-gateway/tests/test_streaming.py (4)
services/policy-gateway/tests/test_integration.py (1)
_import_app(7-14)services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py (1)
stream(76-98)services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py (1)
stream(46-68)services/policy-gateway/src/policy_gateway/ports/llm_adapter.py (1)
stream(21-21)
🪛 actionlint (1.7.8)
.github/workflows/ci.yml
27-27: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
README.md
[uncategorized] ~325-~325: The official name of this software platform is spelled with a capital “H”.
Context: ...e walkthrough | | AI Agent Instructions | AI Assistant...
(GITHUB)
[uncategorized] ~434-~434: The official name of this software platform is spelled with a capital “H”.
Context: ...RAMP, etc.) Development setup: See [.github/copilot-instructions.md](.github/copil...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
README.md
164-164: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
186-186: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
192-192: Spaces inside code span elements
(MD038, no-space-in-code)
docs/Reference/LLMOps_Specs/ADR.md
165-165: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
166-166: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (14)
.gitignore (1)
3-3: Correct and consistent with project conventions.Adding
.venv/aligns with the new development setup introduced in this PR and follows the existing pattern of trailing slashes for directory entries.services/policy-gateway/src/policy_gateway/ports/llm_adapter.py (1)
8-21: LGTM! Clean port abstraction following hexagonal architecture.This Protocol definition correctly establishes the contract for LLM adapters:
- Clear separation between synchronous (
complete) and streaming (stream) operations- Well-documented, especially the note about
stream()potentially yielding a single element for non-streaming providers- Proper use of domain models (
CompletionRequest,CompletionResponse)- No implementation leakage
This enables runtime provider selection (HTTP vs. LiteLLM) without changing application code.
services/policy-gateway/tests/test_streaming.py (1)
16-35: LGTM! Well-structured streaming endpoint test.This test effectively validates the SSE streaming behavior:
- Clean mock adapter implementation that yields multiple chunks
- Proper use of
monkeypatchto inject the test adapter- Validates the key SSE contract:
text/event-streamcontent type anddata:prefixed chunks- Follows the same path-manipulation pattern as other tests for consistent app imports
The test coverage is appropriate for verifying that the streaming endpoint correctly wraps the adapter's stream() method and formats chunks as SSE events.
services/policy-gateway/tests/test_integration.py (1)
17-62: LGTM! Excellent integration test coverage.This test provides comprehensive end-to-end validation:
Strengths:
- Proper isolation: Uses
tmp_pathfixture andmonkeypatchfor clean test environment- Multiple scenarios: Tests both success and failure paths for CI checks
- Context-aware filtering: Validates prompt filtering with jailbreak detection
- Threshold validation: Confirms the threshold-based decision logic works correctly
The test effectively validates the hexagonal architecture refactor by exercising:
- Configuration loading via
PAC_CONFIG- Domain logic in
PolicyDecisionService- HTTP interface and schemas
- Decision rules for both runtime (
/filter/prompt) and pre-merge (/ci/check) scenariosjustfile (1)
26-28: LGTM! Clean developer convenience addition.The
dev-setuptask provides a straightforward entry point for environment setup. The comment clearly documents what the script does. Verification confirms thatscripts/dev_setup.shexists and is executable.services/policy-gateway/src/policy_gateway/infrastructure/litellm_adapter.py (2)
76-95: Verify the litellm streaming API signature.Similar to the
complete()method, thestream()method uses apromptparameter which may not match the standard litellm API. Please verify the streaming API signature as well.
23-74: Defensive response normalization is appropriate here.The extensive fallback logic for extracting
content,model, andusagefrom various response shapes is good defensive programming given that litellm may wrap different provider APIs with varying response structures. This approach maintains adapter robustness across potential litellm versions and provider backends.services/policy-gateway/src/policy_gateway/infrastructure/llm_http_adapter.py (2)
12-44: Well-structured HTTP adapter with appropriate error handling.The implementation correctly:
- Configures endpoint from environment or parameter
- Builds payload with optional fields
- Uses broad exception handling at the adapter boundary (documented as intentional)
- Provides sensible fallback for content extraction
46-68: Streaming fallback is well-designed.The fallback from streaming to a single completed chunk on error is a sensible degradation strategy that maintains the streaming interface while handling upstream endpoints that don't support streaming.
specs/openapi-policy-gateway.yaml (2)
84-99: New proxy endpoint and schemas are well-defined.The new
/proxy/completionendpoint and associatedCompletionRequest/CompletionResponseschemas correctly:
- Define the required
promptfield- Include optional
modelandmax_tokensfields- Match the domain models from
policy_gateway/domain/models.py- Follow the existing API patterns
Also applies to: 149-163
15-15: Consistent quote styling improves spec quality.The change from single quotes to double quotes for response status codes (
"200") is a good stylistic improvement that aligns with common OpenAPI conventions.Also applies to: 25-25, 43-43, 60-60, 77-77, 94-94
services/policy-gateway/src/app.py (3)
28-31: Dependency injection pattern is well-implemented.The use of factory functions with FastAPI's
Dependscorrectly enables:
- Per-request service instantiation (allowing tests to monkeypatch environment variables)
- Clean separation between construction and usage
- Testability via dependency injection
Also applies to: 37-40
77-98: SSE streaming implementation is correct.The streaming endpoint correctly:
- Builds a domain request from the HTTP body
- Yields SSE-formatted chunks with
data:prefix and\n\nterminator- Returns a
StreamingResponsewith appropriatetext/event-streammedia type
111-147: Endpoint refactoring follows hexagonal architecture principles.The refactored endpoints correctly:
- Use dependency injection for the service layer
- Map HTTP schemas to domain models
- Delegate business logic to the service
- Map domain results back to HTTP responses via
to_response()methodsThis is a clean implementation of the hexagonal architecture pattern.
| @@ -0,0 +1,186 @@ | |||
| language: en-US | |||
| tone_instructions: 'Generate concise, actionable, and unambiguous fix instructions. Specify exact file paths and edits; focus on mechanical details required for automated or manual application. Focus on critical functional issues only. Avoid unnecessary explanations or context.' | |||
There was a problem hiding this comment.
Fix validation error: tone_instructions exceeds 250 character limit.
The tone_instructions field exceeds the schema-defined maximum of 250 characters, causing a validation error. This will prevent the configuration from being accepted.
Shorten the tone_instructions to stay within 250 characters. For example:
- tone_instructions: 'Generate concise, actionable, and unambiguous fix instructions. Specify exact file paths and edits; focus on mechanical details required for automated or manual application. Focus on critical functional issues only. Avoid unnecessary explanations or context.'
+ tone_instructions: 'Provide concise, actionable fix instructions with exact file paths and edits. Focus on critical functional issues only.'This condensed version conveys the key guidance while complying with the character limit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tone_instructions: 'Generate concise, actionable, and unambiguous fix instructions. Specify exact file paths and edits; focus on mechanical details required for automated or manual application. Focus on critical functional issues only. Avoid unnecessary explanations or context.' | |
| tone_instructions: 'Provide concise, actionable fix instructions with exact file paths and edits. Focus on critical functional issues only.' |
🤖 Prompt for AI Agents
.coderabbit.yml around line 2: the value of tone_instructions exceeds the
250-character schema limit and must be shortened; replace the current long
string with a concise instruction under 250 characters (keep it quoted), for
example a single-sentence guidance that preserves intent but is <250 chars, then
validate YAML to ensure no trailing commas or invalid quoting.
| - name: Setup Python | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} |
There was a problem hiding this comment.
Update actions/setup-python to a newer version.
The runner of "actions/setup-python@v4" is too old to run on GitHub Actions. Consider upgrading to @v5 or later to ensure compatibility with current GitHub Actions infrastructure.
- name: Setup Python
- uses: actions/setup-python@v4
+ uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}🧰 Tools
🪛 actionlint (1.7.8)
27-27: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 26 to 29: the workflow uses
actions/setup-python@v4 which is outdated for current GitHub Actions runners;
update the action reference to a supported newer major version (for example
actions/setup-python@v5 or the latest stable release) by changing the uses line
accordingly and verify the existing with: python-version input remains valid;
commit the updated workflow file and run CI to ensure compatibility.
| - name: Run linter (ruff) | ||
| env: | ||
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | ||
| run: | | ||
| ruff services/policy-gateway || true |
There was a problem hiding this comment.
Reconsider suppressing linter errors in CI.
Ruff errors are suppressed with || true (line 52), which means linting failures won't cause the CI workflow to fail. This defeats the purpose of running linting in CI. Consider removing the || true to ensure code quality standards are enforced:
- name: Run linter (ruff)
env:
PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src
run: |
- ruff services/policy-gateway || true
+ ruff services/policy-gateway📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run linter (ruff) | |
| env: | |
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | |
| run: | | |
| ruff services/policy-gateway || true | |
| - name: Run linter (ruff) | |
| env: | |
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | |
| run: | | |
| ruff services/policy-gateway |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 48 to 52: the ruff invocation is suffixed
with `|| true`, which masks lint failures and prevents CI from failing on lint
errors; remove the `|| true` so that ruff's non‑zero exit code will fail the
job, and if desired configure ruff via a config file or adjust the job to run
only on changed files rather than suppressing errors.
| - name: Run tests | ||
| env: | ||
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | ||
| run: | | ||
| pytest -q |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure test runner uses the configured virtual environment.
The test step does not explicitly activate the venv (related to the issue flagged in lines 31–46). Ensure pytest is invoked from the correct environment by re-activating the venv or using the absolute path:
- name: Run tests
env:
PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src
run: |
+ source .venv/bin/activate
pytest -qOr use the absolute path:
- pytest -q
+ .venv/bin/pytest -qI'll gather the necessary information to verify this review comment by examining the workflow file and venv setup context.
🏁 Script executed:
#!/bin/bash
# Read the GitHub Actions workflow file to inspect venv setup and test invocation
if [ -f .github/workflows/ci.yml ]; then
echo "=== Full workflow context (lines 30-60) ==="
sed -n '30,60p' .github/workflows/ci.yml
else
echo "Workflow file not found"
fiLength of output: 1085
🏁 Script executed:
#!/bin/bash
# Check if there are other workflow files or CI configurations
fd -t f "(ci|test)" --path ".github" Length of output: 233
Activate the venv or use absolute path to pytest in the test step.
Each GitHub Actions step runs in a new shell session, so the venv activation from the "Install dependencies" step (line 40) does not persist to the "Run tests" step. This means pytest -q currently runs outside the configured virtual environment. Use either approach:
Option 1 (re-activate):
- name: Run tests
env:
PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src
run: |
+ source .venv/bin/activate
pytest -qOption 2 (absolute path):
- name: Run tests
env:
PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src
run: |
- pytest -q
+ .venv/bin/pytest -qThe same issue affects the "Run linter (ruff)" step on line 51.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run tests | |
| env: | |
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | |
| run: | | |
| pytest -q | |
| - name: Run tests | |
| env: | |
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | |
| run: | | |
| source .venv/bin/activate | |
| pytest -q |
| - name: Run tests | |
| env: | |
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | |
| run: | | |
| pytest -q | |
| - name: Run tests | |
| env: | |
| PYTHONPATH: ${{ github.workspace }}/services/policy-gateway/src | |
| run: | | |
| .venv/bin/pytest -q |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 51 to 58: the "Run linter (ruff)" and "Run
tests" steps are invoking tools (ruff, pytest) without ensuring the virtualenv
from the "Install dependencies" step is active in each new shell; either
re-activate the venv at the start of each step (e.g., source .venv/bin/activate
or . .venv/Scripts/activate on Windows) or call the tools via absolute paths
inside the venv (e.g., .venv/bin/ruff or .venv/bin/pytest or python -m pytest
using the venv's python) so the commands run inside the installed environment.
| # Project development environment | ||
|
|
||
| Minimal environment document. See README.md for full developer setup instructions. | ||
|
|
||
| ## Goals / contract | ||
|
|
||
| - Inputs: source files in the repository, a working devbox environment (or a local machine with Python 3.11+). | ||
| - Outputs: reproducible dev environment with isolated `.venv`, installed dev dependencies, runnable FastAPI policy gateway, and passing unit tests. | ||
| - Error modes: missing `uv` (falls back to `pip`), PEP 668-managed Python (create venv first), missing OS packages for binary wheels. | ||
|
|
||
| ### Key files & locations | ||
|
|
||
| - `services/policy-gateway/src/app.py` — policy gateway FastAPI app. | ||
| - `services/policy-gateway/src/policy_gateway/*` — hexagonal architecture code (ports, application, domain, infrastructure, interface). | ||
| - `services/policy-gateway/requirements-dev.txt` — development dependencies used by the convenience script. | ||
| - `scripts/dev_setup.sh` — convenience script that creates/activates a virtualenv, installs deps (prefers `uv`), runs `ruff` and `pytest`. | ||
| - `policies/adr-006.embedded-governance.yaml` — single source of truth for embedded governance thresholds and rules. | ||
|
|
||
| ### Architecture (high level) | ||
|
|
||
| ```mermaid | ||
| flowchart LR | ||
| subgraph App | ||
| PG[Policy Gateway (FastAPI)] | ||
| PG -->|proxies| vLLM[vLLM / LLM service] | ||
| end | ||
|
|
||
| subgraph K8s[Kubernetes] | ||
| PG_DEPLOY[Deployment: policy-gateway] | ||
| VLLM_DEPLOY[Deployment: vLLM] | ||
| INGRESS[Ingress] | ||
| CONFIGMAP[ConfigMap: adr-006.embedded-governance.yaml] | ||
| PG_DEPLOY -->|mounts| CONFIGMAP | ||
| # Project development environment | ||
|
|
||
| This document describes how to set up and work with the Governed Speed project development environment. It covers the recommended local developer workflow (devbox + virtualenv), the `uv` package manager preference, key environment variables, and quick run/test/lint commands. It also includes mermaid diagrams of the high-level architecture and the developer workflow. | ||
|
|
||
| ## Goals / contract | ||
|
|
||
| - Inputs: source files in the repository, a working devbox environment (or a local machine with Python 3.11+). | ||
| - Outputs: reproducible dev environment with isolated `.venv`, installed dev dependencies, runnable FastAPI policy gateway, and passing unit tests. | ||
| - Error modes: missing `uv` (falls back to `pip`), PEP 668-managed Python (create venv first), missing OS packages for binary wheels. | ||
|
|
||
| ## Key files & locations | ||
|
|
||
| - `services/policy-gateway/src/app.py` — policy gateway FastAPI app. | ||
| - `services/policy-gateway/src/policy_gateway/*` — hexagonal architecture code (ports, application, domain, infrastructure, interface). | ||
| - `services/policy-gateway/requirements-dev.txt` — development dependencies used by the convenience script. | ||
| - `scripts/dev_setup.sh` — convenience script that creates/activates a virtualenv, installs deps (prefers `uv`), runs `ruff` and `pytest`. | ||
| - `policies/adr-006.embedded-governance.yaml` — single source of truth for embedded governance thresholds and rules. | ||
|
|
||
| ## Architecture (high level) | ||
|
|
||
| ```mermaid | ||
| flowchart LR | ||
| subgraph App | ||
| PG[Policy Gateway (FastAPI)] | ||
| PG -->|proxies| vLLM[vLLM / LLM service] | ||
| end | ||
|
|
||
| subgraph K8s[Kubernetes] | ||
| PG_DEPLOY[Deployment: policy-gateway] | ||
| VLLM_DEPLOY[Deployment: vLLM] | ||
| INGRESS[Ingress] | ||
| CONFIGMAP[ConfigMap: adr-006.embedded-governance.yaml] | ||
| PG_DEPLOY -->|mounts| CONFIGMAP | ||
| INGRESS --> PG_DEPLOY | ||
| end | ||
|
|
||
| subgraph Infra | ||
| RES[Risk & Evidence Service] | ||
| PROM[Prometheus] | ||
| GRAF[Grafana] | ||
| end | ||
|
|
||
| PG_DEPLOY --- RES | ||
| PG_DEPLOY --- PROM | ||
| PROM --- GRAF | ||
| VLLM_DEPLOY --- PROM | ||
|
|
||
| note right of PG_DEPLOY | ||
| Gateway runs as a sidecar alongside the LLM in k8s. | ||
| Gateway enforces policies and forwards requests to vLLM. | ||
| end | ||
| ``` | ||
|
|
||
| ## Developer workflow (diagram) | ||
|
|
||
| ```mermaid | ||
| flowchart TD | ||
| Devbox[Devbox shell] | ||
| Devbox --> VENV[Create .venv] | ||
| VENV -->|uv present| UV[uv venv .venv] | ||
| VENV -->|uv absent| PYV[python3 -m venv .venv] | ||
| VENV --> Activate[Activate venv] | ||
| Activate --> Install[Install dev deps] | ||
| Install -->|prefer uv| UV_INSTALL[uv install -r requirements-dev.txt] | ||
| Install -->|fallback pip| PIP_INSTALL[pip install -r requirements-dev.txt] | ||
| Install --> Lint[Run ruff] | ||
| Install --> Test[Run pytest] | ||
| Test --> Run[Run uvicorn app:app] | ||
| Run --> Local[Local development / Integration testing] | ||
| CI[GitHub Actions CI] --> Install | ||
| CI --> Lint | ||
| CI --> Test | ||
| ``` | ||
|
|
||
| ## Environment variables used by the services | ||
|
|
||
| - `PAC_CONFIG` — path to gateway config (default: `/config/adr-006.embedded-governance.yaml`). | ||
| - `PAC_UPSTREAM_URL` — upstream LLM endpoint (default: `http://localhost:8000`). | ||
| - `STORAGE_PATH` — path for evidence storage (default: `/evidence`). | ||
| - `PAC_LLM_PROVIDER` — select LLM adapter at runtime (default: `http`). | ||
| - `http` — forward to `${PAC_UPSTREAM_URL}` | ||
| - `litellm` — use local `litellm` client adapter (requires package installed) | ||
|
|
||
| ## Streaming SSE endpoint usage | ||
|
|
||
| The Policy Gateway exposes `POST /proxy/completion/stream` which returns a | ||
| Server-Sent-Events stream (`text/event-stream`). Each event contains a | ||
| `data: <chunk>` line and a blank line terminator. Use this endpoint for | ||
| low-latency UI streaming or token-by-token rendering. | ||
|
|
||
| Quick curl test: | ||
|
|
||
| ```bash | ||
| curl -N -H "Content-Type: application/json" \ | ||
| -X POST http://localhost:8081/proxy/completion/stream \ | ||
| -d '{"prompt":"Summarize the recent meeting notes"}' | ||
| ``` | ||
|
|
||
| Browser example using Fetch + ReadableStream (preferred when POST isn't | ||
| supported by EventSource): | ||
|
|
||
| ```javascript | ||
| // POST to an SSE-compatible proxy or use a fetch streaming response | ||
| fetch('/proxy/completion/stream', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ prompt: 'Hello' }) }) | ||
| .then(resp => { | ||
| const reader = resp.body.getReader(); | ||
| const decoder = new TextDecoder(); | ||
| return reader.read().then(function process({ done, value }) { | ||
| if (done) return; | ||
| const chunk = decoder.decode(value, { stream: true }); | ||
| console.log('chunk:', chunk); | ||
| return reader.read().then(process); | ||
| }); | ||
| }) | ||
| .catch(err => console.error(err)); | ||
| ``` | ||
|
|
||
| Environment examples: | ||
|
|
||
| ```bash | ||
| # Default (HTTP forwarder) | ||
| export PAC_LLM_PROVIDER=http | ||
| export PAC_UPSTREAM_URL=http://localhost:8000 | ||
|
|
||
| # Use litellm adapter (ensure litellm installed in the runtime venv/container) | ||
| export PAC_LLM_PROVIDER=litellm | ||
| ``` | ||
|
|
||
| ## Recommended quickstart (zsh / devbox) | ||
|
|
||
| 1. Open devbox shell (recommended): | ||
|
|
||
| ```bash | ||
| devbox shell | ||
| ``` | ||
|
|
||
| 1. Run the convenience setup script (it will create `.venv`, prefer `uv` for installs, and run linter/tests): | ||
|
|
||
| ```bash | ||
| # From the repo root | ||
| chmod +x scripts/dev_setup.sh # one-time | ||
| ./scripts/dev_setup.sh | ||
| ``` | ||
|
|
||
| 1. If you want to start the API after setup: | ||
|
|
||
| ```bash | ||
| # Activate the venv created by the script (if not already active) | ||
| source .venv/bin/activate | ||
|
|
||
| # Run the gateway (ensure PYTHONPATH points to the service src dir) | ||
| export PYTHONPATH="$PWD/services/policy-gateway/src:${PYTHONPATH:-}" | ||
| uvicorn app:app --reload --port 8081 | ||
| ``` | ||
|
|
||
| 1. Run a single test file manually (example): | ||
|
|
||
| ```bash | ||
|
|
||
| # Project development environment | ||
|
|
||
| This document describes how to set up and work with the Governed Speed project development environment. It covers the recommended local developer workflow (devbox + virtualenv), the `uv` package manager preference, key environment variables, and quick run/test/lint commands. It also includes mermaid diagrams of the high-level architecture and the developer workflow. | ||
|
|
||
| ## Goals / contract | ||
|
|
||
| - Inputs: source files in the repository, a working devbox environment (or a local machine with Python 3.11+). | ||
| - Outputs: reproducible dev environment with isolated `.venv`, installed dev dependencies, runnable FastAPI policy gateway, and passing unit tests. | ||
| - Error modes: missing `uv` (falls back to `pip`), PEP 668-managed Python (create venv first), missing OS packages for binary wheels. | ||
|
|
||
| ## Key files & locations | ||
|
|
||
| - `services/policy-gateway/src/app.py` — policy gateway FastAPI app. | ||
| - `services/policy-gateway/src/policy_gateway/*` — hexagonal architecture code (ports, application, domain, infrastructure, interface). | ||
| - `services/policy-gateway/requirements-dev.txt` — development dependencies used by the convenience script. | ||
| - `scripts/dev_setup.sh` — convenience script that creates/activates a virtualenv, installs deps (prefers `uv`), runs `ruff` and `pytest`. | ||
| - `policies/adr-006.embedded-governance.yaml` — single source of truth for embedded governance thresholds and rules. | ||
|
|
||
| ## Architecture (high level) | ||
|
|
||
| ```mermaid | ||
| flowchart LR | ||
| subgraph App | ||
| PG[Policy Gateway (FastAPI)] | ||
| PG -->|proxies| vLLM[vLLM / LLM service] | ||
| end | ||
|
|
||
| subgraph K8s[Kubernetes] | ||
| PG_DEPLOY[Deployment: policy-gateway] | ||
| VLLM_DEPLOY[Deployment: vLLM] | ||
| INGRESS[Ingress] | ||
| CONFIGMAP[ConfigMap: adr-006.embedded-governance.yaml] | ||
| PG_DEPLOY -->|mounts| CONFIGMAP | ||
| INGRESS --> PG_DEPLOY | ||
| end | ||
|
|
||
| subgraph Infra | ||
| RES[Risk & Evidence Service] | ||
| PROM[Prometheus] | ||
| GRAF[Grafana] | ||
| end | ||
|
|
||
| PG_DEPLOY --- RES | ||
| PG_DEPLOY --- PROM | ||
| PROM --- GRAF | ||
| VLLM_DEPLOY --- PROM | ||
|
|
||
| note right of PG_DEPLOY | ||
| Gateway runs as a sidecar alongside the LLM in k8s. | ||
| Gateway enforces policies and forwards requests to vLLM. | ||
| end | ||
| ``` | ||
|
|
||
| ## Developer workflow (diagram) | ||
|
|
||
| ```mermaid | ||
| flowchart TD | ||
| Devbox[Devbox shell] | ||
| Devbox --> VENV[Create .venv] | ||
| VENV -->|uv present| UV[uv venv .venv] | ||
| VENV -->|uv absent| PYV[python3 -m venv .venv] | ||
| VENV --> Activate[Activate venv] | ||
| Activate --> Install[Install dev deps] | ||
| Install -->|prefer uv| UV_INSTALL[uv install -r services/policy-gateway/requirements-dev.txt] | ||
| Install -->|fallback pip| PIP_INSTALL[pip install -r services/policy-gateway/requirements-dev.txt] | ||
| Install --> Lint[Run ruff] | ||
| Install --> Test[Run pytest] | ||
| Test --> Run[Run uvicorn app:app] | ||
| Run --> Local[Local development / Integration testing] | ||
| CI[GitHub Actions CI] --> Install | ||
| CI --> Lint | ||
| CI --> Test | ||
| ``` | ||
|
|
||
| ## Environment variables used by the services | ||
|
|
||
| - `PAC_CONFIG` — path to gateway config (default: `/config/adr-006.embedded-governance.yaml`). | ||
| - `PAC_UPSTREAM_URL` — upstream LLM endpoint (default: `http://localhost:8000`). | ||
| - `STORAGE_PATH` — path for evidence storage (default: `/evidence`). | ||
|
|
||
| ## Recommended quickstart (zsh / devbox) | ||
|
|
||
| 1. Open devbox shell (recommended): | ||
|
|
||
| ```bash | ||
| devbox shell | ||
| ``` | ||
|
|
||
| 2. Run the convenience setup script (it will create `.venv`, prefer `uv` for installs, and run linter/tests): | ||
|
|
||
| ```bash | ||
| # From the repo root | ||
| chmod +x scripts/dev_setup.sh # one-time | ||
| ./scripts/dev_setup.sh | ||
| ``` | ||
|
|
||
| 3. If you want to start the API after setup: | ||
|
|
||
| ```bash | ||
| # Activate the venv created by the script (if not already active) | ||
| source .venv/bin/activate | ||
|
|
||
| # Run the gateway (ensure PYTHONPATH points to the service src dir) | ||
| export PYTHONPATH="$PWD/services/policy-gateway/src:${PYTHONPATH:-}" | ||
| uvicorn app:app --reload --port 8081 | ||
| ``` | ||
|
|
||
| 4. Run a single test file manually (example): | ||
|
|
||
| ```bash | ||
| pytest services/policy-gateway/tests/test_policy_service.py -q | ||
| ``` | ||
|
|
||
| ## Using `uv` (package manager) | ||
|
|
||
| - This repository prefers `uv` when available for fast installs and venv management. `scripts/dev_setup.sh` tries `uv` first and falls back to pip. | ||
| - Typical `uv` commands used by the script: | ||
| - `uv venv .venv` — create a venv | ||
| - `uv install -r services/policy-gateway/requirements-dev.txt` or `uv add -r ...` — install requirements | ||
|
|
||
| If `uv` is present on your machine, the script will use it automatically. If not present, the script uses `python3 -m venv` + `pip`. | ||
|
|
||
| ## CI recommendations | ||
|
|
||
| - Add a GitHub Actions workflow to replicate `scripts/dev_setup.sh` steps: | ||
| 1. create Python 3.11 environment | ||
| 2. create/activate venv or rely on Actions runner's python | ||
| 3. install `uv` if you want to use it (optional) or use `pip` to install `requirements-dev.txt` | ||
| 4. run `ruff` and `pytest` as part of PR validation | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| - If the script fails with a PEP 668 / "externally-managed-environment" error, create and activate a venv first: | ||
|
|
||
| ```bash | ||
| python3 -m venv .venv | ||
| source .venv/bin/activate | ||
| ./scripts/dev_setup.sh | ||
| ``` | ||
|
|
||
| - If imports fail in tests (e.g. `ModuleNotFoundError: No module named 'yaml'`) ensure `PyYAML` is installed in the active venv (the script installs it from `requirements-dev.txt`). | ||
|
|
||
| ## Notes and next steps | ||
|
|
||
| - The repository follows a hexagonal architecture for the policy gateway; domain models and HTTP schemas are kept separate. We added `to_response()` helpers on domain classes to centralize mapping (see `services/policy-gateway/src/policy_gateway/domain/models.py`). | ||
| - You can add a CI job and/or a `Makefile`/`just` entry that wraps `scripts/dev_setup.sh` for a single-command developer experience. | ||
|
|
||
| If you want, I can also: | ||
|
|
||
| - Add a GitHub Actions workflow that runs `scripts/dev_setup.sh` (or equivalent steps) on PRs. | ||
| - Extend the diagrams with more details (Kubernetes manifests, ingress, or observability dashboards). | ||
| 1. Run a single test file manually (example): | ||
|
|
||
| ```bash | ||
| pytest services/policy-gateway/tests/test_policy_service.py -q | ||
| ``` | ||
|
|
||
| ### Using `uv` (package manager) | ||
|
|
||
| - This repository prefers `uv` when available for fast installs and venv management. `scripts/dev_setup.sh` tries `uv` first and falls back to pip. | ||
| - Typical `uv` commands used by the script: | ||
| - `uv venv .venv` — create a venv | ||
| - `uv install -r services/policy-gateway/requirements-dev.txt` or `uv add -r ...` — install requirements | ||
|
|
||
| If `uv` is present on your machine, the script will use it automatically. If not present, the script uses `python3 -m venv` + `pip`. | ||
|
|
||
| ### CI recommendations | ||
|
|
||
| - Add a GitHub Actions workflow to replicate `scripts/dev_setup.sh` steps: | ||
| 1. create Python 3.11 environment | ||
| 1. create/activate venv or rely on Actions runner's python | ||
| 1. install `uv` if you want to use it (optional) or use `pip` to install `requirements-dev.txt` | ||
| 1. run `ruff` and `pytest` as part of PR validation | ||
|
|
||
| ### Troubleshooting | ||
|
|
||
| - If the script fails with a PEP 668 / "externally-managed-environment" error, create and activate a venv first: | ||
|
|
||
| ```bash | ||
| python3 -m venv .venv | ||
| source .venv/bin/activate | ||
| ./scripts/dev_setup.sh | ||
| ``` | ||
|
|
||
| - If imports fail in tests (e.g. `ModuleNotFoundError: No module named 'yaml'`) ensure `PyYAML` is installed in the active venv (the script installs it from `requirements-dev.txt`). |
There was a problem hiding this comment.
Major documentation duplication - consolidate repeated sections.
This document contains extensive duplication that will create maintenance burden and confusion:
- Lines 1-85 and 193-244: The entire "Goals / contract", "Key files & locations", and "Architecture" sections are duplicated nearly verbatim.
- Lines 162-191 and 273-348: Quickstart instructions repeated with minor variations.
- Lines 89-106 and 249-265: Developer workflow diagrams duplicated.
This appears to be the result of a merge conflict or accidental paste operation.
Recommendation: Consolidate to a single, well-organized structure:
# Project development environment
## Goals / contract
[single instance]
## Key files & locations
[single instance]
## Architecture
[single instance - one mermaid diagram]
## Developer workflow
[single instance - one mermaid diagram]
## Environment variables
[consolidated list including PAC_LLM_PROVIDER, streaming vars]
## Streaming SSE endpoint usage
[single instance with examples]
## Quickstart
[single instance with clear steps]
## Using uv
[single instance]
## CI recommendations
[single instance]
## Troubleshooting
[single instance]Remove the duplicate blocks and organize the unique content (especially the streaming and PAC_LLM_PROVIDER sections from lines 113-160) into the appropriate sections.
🤖 Prompt for AI Agents
In docs/Reference/ENVIRONMENT.md lines 1-377 there are large duplicated blocks
(Goals/contract, Key files, Architecture, Developer workflow, Quickstart, and
other sections) likely from a bad merge; remove the repeated copies so each
logical section appears only once, keep the most complete version of each
section (merge unique bits like the Streaming SSE examples and PAC_LLM_PROVIDER
details into the canonical sections), collapse duplicated mermaid diagrams into
single instances, ensure headings/order follow the recommended outline in the
review, and run a quick pass to fix numbering/formatting and remove any stray
pasted fragments.
| from policy_gateway.domain.models import ( | ||
| CompletionRequest as DomainCompletionRequest, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move duplicate imports to the module top.
The CompletionRequest domain model is imported inside two different functions. Move this import to the top of the file alongside other domain model imports to improve code organization and avoid duplication.
Apply this diff:
from policy_gateway.domain.models import (
CiCheckInput,
+ CompletionRequest as DomainCompletionRequest,
OutputDecisionInput,
PromptDecisionInput,
)Then remove the inline imports:
def proxy_completion(
body: CompletionRequest,
):
adapter = _build_llm_adapter()
- # map HTTP request -> domain request
- from policy_gateway.domain.models import (
- CompletionRequest as DomainCompletionRequest,
- )
-
domain_req = DomainCompletionRequest( def proxy_completion_stream(body: CompletionRequest):
"""Stream completion results as Server-Sent-Events (SSE)."""
adapter = _build_llm_adapter()
- from policy_gateway.domain.models import (
- CompletionRequest as DomainCompletionRequest,
- )
-
domain_req = DomainCompletionRequest(Also applies to: 85-87
🤖 Prompt for AI Agents
In services/policy-gateway/src/app.py around lines 64-66 and 85-87, the
CompletionRequest domain model is being imported inline inside two functions;
move the duplicate import to the module top with the other domain model imports
(add "from policy_gateway.domain.models import CompletionRequest as
DomainCompletionRequest" alongside existing imports near line ~64), then remove
the inline imports at both function locations so the functions use the top-level
DomainCompletionRequest import.
… improve dev setup script, refine LiteLLMAdapter and HTTPLLMAdapter for better error handling, and update OpenAPI spec to require content and model fields.
Summary
Testing
Codex Task
Summary by Sourcery
Restructure the FastAPI policy gateway into a hexagonal architecture by extracting domain models, application services, and ports, introducing a file‐based configuration adapter and HTTP schemas, and replacing inline decision logic with dependency‐injected service calls
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
Chores