fix: repo-wide security hardening from ZAP, Scorecard, and CodeQL audit#357
fix: repo-wide security hardening from ZAP, Scorecard, and CodeQL audit#357
Conversation
- Pin sandbox Dockerfile images (node:22-slim, python:3.14-slim) by digest - Add docker/sandbox to Dependabot for automated digest updates - Add --ignore-scripts to wrangler install for supply-chain safety - Add NotFoundException handler so unmatched routes return 404 (not 500) - Add Cross-Origin-Resource-Policy and Cache-Control response headers - Add API_ROUTE_NOT_FOUND event constant - Clarify ZAP DAST auto-issue behavior in workflow comment - Add unit tests for 404 handler and all security response headers - Dismiss CodeQL false positives (#10, #22, #23) with documented reasoning - Enable required status checks (ci-pass) and PR reviews in protect-main ruleset Closes #356
…clusion - Create docs/security.md with comprehensive security architecture docs covering application security, container hardening, supply chain, and CI/CD - Add Security section to landing page with 3 clickable cards - Make Core Pillars, Architecture, and Security cards clickable (link to docs) - Add Security link to landing page footer - Exclude library reference (docs/api/*) from docs search via front matter - Add Security to mkdocs.yml nav - Update CLAUDE.md documentation section - Fix check-yaml hook: add --unsafe for mkdocs.yml Python tags Closes #356
Split check-yaml into two hooks: safe_load for all YAML files, --unsafe only for mkdocs.yml which requires !!python/name tags for Material emoji extensions.
- Wire API_ROUTE_NOT_FOUND event constant into handle_not_found (was dead code — now logs with domain-specific event) - Add HTTPException catch-all handler to preserve correct status codes for 405, 429, 400, 413, 503 (previously all returned 500) - Harden 401/403 handlers with static error messages instead of echoing exc.detail (information leakage prevention) - Add HSTS header to parametrized security headers test - Add tests for Litestar PermissionDeniedException, NotAuthorizedException, ValidationException, and MethodNotAllowedException handlers - Fix landing page scanner description accuracy - Add Security row to README documentation table - Align sandbox Dockerfile python tag with backend (3.14.3-slim) - Add MRO resolution comment to EXCEPTION_HANDLERS dict Pre-reviewed by 7 agents, 10 findings addressed.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded repository security hardening: Dependabot Docker config, CI/workflow annotations and npm install change, tightened pre-commit YAML checks, pinned sandbox Docker base images, added security docs and site nav, added security response headers, explicit NotFound/HTTP exception handlers and event, and expanded unit tests and docs front-matter. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant App as App (Litestar)
participant Router as Router/Handler
participant Obs as Observability
participant Response as HTTP Response
Client->>App: HTTP request (e.g., /nonexistent or /openapi.json)
App->>Router: route resolution / handler invocation
alt NotFoundException
Router->>Obs: emit API_ROUTE_NOT_FOUND (api.route.not_found)
Obs-->>Router: ack
Router-->>Response: 404 "Not found" + security headers
Response-->>Client: 404 response
else HTTPException
Router->>Obs: log HTTPException (status, headers)
note right of Router: If 5xx -> scrub detail\nElse -> preserve message or use HTTPStatus phrase
Router-->>Response: status code, headers, sanitized detail
Response-->>Client: HTTPException response
else Other uncaught errors (5xx)
Router->>Obs: log server error context
Router-->>Response: 500 "An internal server error occurred" (scrubbed)
Response-->>Client: 500 response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, 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 significantly strengthens the repository's security posture by addressing findings from ZAP DAST, OSSF Scorecard, and CodeQL audits. It focuses on improving API resilience against common web vulnerabilities, enhancing the security of container images and the software supply chain, and providing clear, comprehensive security documentation. The changes aim to create a more robust and secure development and deployment environment. Highlights
Changelog
Ignored Files
Activity
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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #357 +/- ##
=======================================
Coverage 93.90% 93.90%
=======================================
Files 447 447
Lines 20803 20819 +16
Branches 2010 2011 +1
=======================================
+ Hits 19535 19551 +16
Misses 981 981
Partials 287 287 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of security hardening measures across the repository, based on findings from ZAP, OSSF Scorecard, and CodeQL audits. Key changes include adding security-related HTTP headers, improving exception handling to prevent information leakage and correctly map status codes, pinning Docker base images to SHA digests, and adding Dependabot for Docker dependencies. The PR also includes significant documentation updates, most notably a new security.md page detailing the project's security posture, and updates to the landing page to highlight these security features. The changes are well-tested with new unit tests for the new exception handlers and security headers. Overall, this is an excellent contribution that significantly improves the security and robustness of the application. I have one minor suggestion regarding a potential documentation inaccuracy.
| ### Image Provenance | ||
|
|
||
| - All base images **pinned by SHA-256 digest** (no mutable tags) | ||
| - **Dependabot** auto-updates digests daily for all three Dockerfiles |
There was a problem hiding this comment.
The documentation states that Dependabot auto-updates digests for all three Dockerfiles. This pull request adds the configuration for docker/sandbox, but it's unclear if the other two Dockerfiles (docker/backend and docker/web) are covered. To ensure documentation accuracy, please either add the missing Dependabot configurations for the other Dockerfiles or update this line to reflect that only the sandbox Dockerfile is currently covered.
Greptile SummaryThis PR applies repo-wide security hardening informed by ZAP DAST, OSSF Scorecard, and CodeQL audits across 30 files. The changes are well-scoped and systematically addressed with new tests covering all the behavioral changes. Key changes:
Issue found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
REQ[Incoming Request] --> ROUTE{Route matched?}
ROUTE -- No --> NF[NotFoundException]
ROUTE -- Yes --> HANDLER[Controller Handler]
NF --> HNF["handle_not_found()\n→ 404 'Not found'\nlog: API_ROUTE_NOT_FOUND"]
HANDLER --> EXC{Exception raised?}
EXC -- No --> RESP[Normal Response]
EXC -- Yes --> ETYPE{Exception type}
ETYPE -- RecordNotFoundError --> H404["handle_record_not_found()\n→ 404 'Resource not found'"]
ETYPE -- DuplicateRecordError --> H409["handle_duplicate_record()\n→ 409 'Resource already exists'"]
ETYPE -- PersistenceError --> H500P["handle_persistence_error()\n→ 500 (scrubbed)"]
ETYPE -- NotAuthorizedException --> H401["handle_not_authorized()\n→ 401 exc.detail or phrase"]
ETYPE -- PermissionDeniedException --> H403["handle_permission_denied()\n→ 403 exc.detail or phrase"]
ETYPE -- ValidationException --> H400["handle_validation_error()\n→ 400 'Validation error'"]
ETYPE -- "HTTPException (405, 429…)" --> HHTTP["handle_http_exception()\n5xx → scrubbed\n4xx → exc.detail or phrase"]
ETYPE -- ApiError --> HAPI["handle_api_error()\n5xx → class default\n4xx → str(exc)"]
ETYPE -- "Exception (catch-all)" --> H500["handle_unexpected()\n→ 500 'Internal server error'"]
RESP --> HEADERS["Global Response Headers\nX-Content-Type-Options, X-Frame-Options\nHSTS, Referrer-Policy, Permissions-Policy\nCross-Origin-Resource-Policy ✨\nCache-Control: no-store ✨"]
H404 --> HEADERS
HNF --> HEADERS
H409 --> HEADERS
H500P --> HEADERS
H401 --> HEADERS
H403 --> HEADERS
H400 --> HEADERS
HHTTP --> HEADERS
HAPI --> HEADERS
H500 --> HEADERS
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/ai_company/api/exception_handlers.py
Line: 138-175
Comment:
**PR description contradicts actual 401/403 behavior**
The PR description states *"Static error messages for 401/403 (prevent `exc.detail` leakage)"*, but both `handle_permission_denied` (line 144) and `handle_not_authorized` (line 171) still return `exc.detail` unchanged when it is set:
```python
detail = exc.detail or HTTPStatus.FORBIDDEN.phrase # 403 – still exposes exc.detail
detail = exc.detail or HTTPStatus.UNAUTHORIZED.phrase # 401 – still exposes exc.detail
```
The new tests `test_permission_denied_preserves_detail` and `test_not_authorized_preserves_detail` explicitly assert that `exc.detail` IS returned to the caller. The actual change was purely mechanical — replacing the hardcoded fallback strings (`"Forbidden"` / `"Authentication required"`) with `HTTPStatus` enum values. No detail scrubbing was introduced.
This matters because future security auditors may reference the PR description when assessing the codebase's information-leakage posture and incorrectly conclude that 401/403 responses are scrubbed. If the intent is to keep the detail passthrough for 401/403 (as the module docstring now accurately documents), the PR description should be corrected to reflect that. If the intent was to scrub the detail, these handlers need to be updated:</p>
```python
def handle_permission_denied(...):
_log_error(request, exc, status=403)
return Response(
content=ApiResponse[None](error=HTTPStatus.FORBIDDEN.phrase),
status_code=403,
)
def handle_not_authorized(...):
_log_error(request, exc, status=401)
return Response(
content=ApiResponse[None](error=HTTPStatus.UNAUTHORIZED.phrase),
status_code=401,
)
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 00646f6 |
There was a problem hiding this comment.
Pull request overview
Repo-wide security hardening across the API, CI/workflows, container supply chain, and documentation based on ZAP/Scorecard/CodeQL audit findings.
Changes:
- Harden Litestar API exception handling (404 for unmatched routes, preserve HTTPException status codes; static 401/403 messages) and add tests.
- Add/validate security-related response headers and expand security-focused documentation and site navigation.
- Tighten supply-chain/CI posture (digest-pinned sandbox Dockerfile images, Dependabot coverage, safer npm install in Pages preview, pre-commit YAML safety scoping).
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/api/test_exception_handlers.py |
Adds regression tests for new Litestar exception-handler mappings. |
tests/unit/api/test_app.py |
Adds assertions for security headers on the OpenAPI schema endpoint. |
src/ai_company/observability/events/api.py |
Adds a new structured logging event constant for route-not-found. |
src/ai_company/api/exception_handlers.py |
Adds 404 handler for Litestar NotFoundException and a catch-all HTTPException handler; hardens 401/403 messages. |
src/ai_company/api/app.py |
Adds global CORP and Cache-Control response headers. |
site/src/pages/index.astro |
Makes landing page cards clickable and adds a new Security section + footer link. |
mkdocs.yml |
Adds Security page to docs navigation. |
docs/security.md |
Introduces a comprehensive security architecture/hardening page. |
docs/api/tools.md |
Excludes library-reference page from doc-site search. |
docs/api/templates.md |
Excludes library-reference page from doc-site search. |
docs/api/security.md |
Excludes library-reference page from doc-site search. |
docs/api/providers.md |
Excludes library-reference page from doc-site search. |
docs/api/persistence.md |
Excludes library-reference page from doc-site search. |
docs/api/observability.md |
Excludes library-reference page from doc-site search. |
docs/api/memory.md |
Excludes library-reference page from doc-site search. |
docs/api/index.md |
Excludes library-reference index from doc-site search. |
docs/api/hr.md |
Excludes library-reference page from doc-site search. |
docs/api/engine.md |
Excludes library-reference page from doc-site search. |
docs/api/core.md |
Excludes library-reference page from doc-site search. |
docs/api/config.md |
Excludes library-reference page from doc-site search. |
docs/api/communication.md |
Excludes library-reference page from doc-site search. |
docs/api/budget.md |
Excludes library-reference page from doc-site search. |
docs/api/api.md |
Excludes library-reference page from doc-site search. |
docker/sandbox/Dockerfile |
Pins sandbox base images by digest (and Python patch version). |
README.md |
Adds Security link to docs table. |
CLAUDE.md |
Documents new security documentation entry. |
.pre-commit-config.yaml |
Scopes check-yaml --unsafe to mkdocs.yml only and excludes it from the safe YAML check. |
.github/workflows/pages-preview.yml |
Adds --ignore-scripts to CI-only wrangler install. |
.github/workflows/dast.yml |
Updates commentary around ZAP issue permissions. |
.github/dependabot.yml |
Adds Dependabot updates for docker/sandbox Dockerfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| status = exc.status_code | ||
| _log_error(request, exc, status=status) | ||
| if status >= _SERVER_ERROR_THRESHOLD: | ||
| msg = "Internal server error" | ||
| else: | ||
| msg = exc.detail or HTTPStatus(status).phrase | ||
| return Response( | ||
| content=ApiResponse[None](error=msg), | ||
| status_code=status, | ||
| ) |
| ResponseHeader( | ||
| name="Cache-Control", | ||
| value="no-store", | ||
| ), |
| def test_method_not_allowed_maps_to_405(self) -> None: | ||
| """MethodNotAllowedException returns 405 via HTTPException handler.""" | ||
|
|
||
| @get("/test") | ||
| async def handler() -> None: | ||
| raise MethodNotAllowedException | ||
|
|
||
| with TestClient(_make_app(handler)) as client: | ||
| resp = client.get("/test") | ||
| assert resp.status_code == 405 |
| permissions: | ||
| contents: read | ||
| issues: write # Required by API scan action to create issues for scan findings | ||
| # The ZAP action creates/updates a single GitHub issue per repo with scan | ||
| # findings (not one per run). Removing issues:write would cause the action | ||
| # to fail. The auto-created issue is updated in-place on subsequent runs. | ||
| issues: write |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pages-preview.yml:
- Around line 236-238: The workflow currently runs the command "npm i --no-save
--ignore-scripts wrangler@3.114.17" which does not produce a reproducible
dependency tree; to fix this either commit and use a lockfile for that tool and
install with a deterministic flow (e.g., include a package-lock.json and run npm
ci or run the install in a small repo-scoped package.json for wrangler) or
replace the npm install with downloading a pre-built wrangler artifact and
verify its checksum before execution; update the workflow to use the chosen
approach and remove the non-lockfile npm install to ensure deterministic
installs.
In `@docs/security.md`:
- Around line 157-167: Remove the "Branch protection — prevents direct commits
to `main`" bullet from the "Pre-Commit Hooks" section and add that line (or a
short sentence stating that branch protection is enforced on the remote/GitHub)
to the existing "Branch Protection" section so the control boundary is accurate;
update the "Pre-Commit Hooks" list in docs/security.md (section heading
"Pre-Commit Hooks") to include only locally-run tools (gitleaks, hadolint, ruff,
commitizen, large file prevention) and place the branch protection description
under the "Branch Protection" heading.
In `@site/src/pages/index.astro`:
- Around line 257-266: Update the overstated copy inside the anchor block with
heading "Continuous Security" (the <a ...> block that contains the
<h3>Continuous Security</h3> and the paragraph) so it no longer claims the
scanners run on "every commit, PR, and deployment"; replace that sentence with
softer wording such as "across the SDLC" or similar to match docs/security.md;
ensure the change is made to the paragraph node (the <p
class="text-gray-400..."> element) so the homepage wording aligns with the mixed
schedules described in docs/security.md.
In `@src/ai_company/api/app.py`:
- Around line 465-468: Remove the global ResponseHeader("Cache-Control",
"no-store") from the app-level response_headers and instead set Cache-Control on
the specific routes: implement a path-specific response hook or use the handler
for the /health endpoint to set "Cache-Control: no-store", and set a short
"Cache-Control: public, max-age=..." on the OpenAPI document/Docs endpoint; find
the existing ResponseHeader entry (ResponseHeader with name="Cache-Control") in
app creation and delete it, then add per-route middleware or modify the /health
handler and the OpenAPI route handler to set the appropriate Cache-Control
headers.
In `@tests/unit/api/test_app.py`:
- Around line 35-57: The test currently treats "Cache-Control" as a global
header in test_security_response_headers; remove ("Cache-Control", "no-store")
from the parametrized list and instead add explicit, path-specific assertions:
call test_client.get("/health") and assert response.headers.get("Cache-Control")
== "no-store", and call test_client.get("/docs/openapi.json") and assert
response.headers.get("Cache-Control") contains a short "max-age=" value (i.e.,
not "no-store"); update test_security_response_headers to only cover the other
static headers and add the two dedicated checks using the existing
test_client.get calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bf9d0ff-fd31-4fe8-a0fc-f279450506be
📒 Files selected for processing (30)
.github/dependabot.yml.github/workflows/dast.yml.github/workflows/pages-preview.yml.pre-commit-config.yamlCLAUDE.mdREADME.mddocker/sandbox/Dockerfiledocs/api/api.mddocs/api/budget.mddocs/api/communication.mddocs/api/config.mddocs/api/core.mddocs/api/engine.mddocs/api/hr.mddocs/api/index.mddocs/api/memory.mddocs/api/observability.mddocs/api/persistence.mddocs/api/providers.mddocs/api/security.mddocs/api/templates.mddocs/api/tools.mddocs/security.mdmkdocs.ymlsite/src/pages/index.astrosrc/ai_company/api/app.pysrc/ai_company/api/exception_handlers.pysrc/ai_company/observability/events/api.pytests/unit/api/test_app.pytests/unit/api/test_exception_handlers.py
📜 Review details
⏰ 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). (5)
- GitHub Check: Agent
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Greptile Review
- GitHub Check: Test (Python 3.14)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649
Useexcept A, B:(no parentheses) for exception handling — PEP 758 syntax for Python 3.14, enforced by ruff
All public functions require type hints — mypy strict mode enforced
Use Google-style docstrings on all public classes and functions — enforced by ruff D rules
Create new objects instead of mutating existing ones — usecopy.deepcopy()for non-Pydantic collections and wrap withMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; separate mutable-via-copy models for runtime state that evolves
Use@computed_fieldfor derived values in Pydantic models instead of storing + validating redundant fields
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Use Pydantic v2 withBaseModel,model_validator,computed_field,ConfigDictfor all model definitions
Use PEP 649 lazy annotations instead offrom __future__ import annotationsin Python 3.14 files
Files:
src/ai_company/observability/events/api.pytests/unit/api/test_app.pysrc/ai_company/api/app.pytests/unit/api/test_exception_handlers.pysrc/ai_company/api/exception_handlers.py
src/ai_company/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/ai_company/**/*.py: Usefrom ai_company.observability import get_loggerand createlogger = get_logger(__name__)in every module with business logic
Never useimport logging,logging.getLogger(), orprint()in application code
Always useloggeras the variable name for loggers, not_loggerorlog
Use event name constants from domain-specific modules underai_company.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider)
Log using structured kwargs:logger.info(EVENT, key=value)— never use string formatting likelogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
Use DEBUG level for object creation, internal flow, and entry/exit of key functions
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Usemodel_copy(update=...)for mutable-via-copy runtime state models instead of direct mutations
Never mix static config fields with mutable runtime fields in one Pydantic model
Library reference is auto-generated indocs/api/from docstrings via mkdocstrings + Griffe (AST-based, no imports)
Files:
src/ai_company/observability/events/api.pysrc/ai_company/api/app.pysrc/ai_company/api/exception_handlers.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Line length: 88 characters (enforced by ruff)
Files:
src/ai_company/observability/events/api.pytests/unit/api/test_app.pysrc/ai_company/api/app.pytests/unit/api/test_exception_handlers.pysrc/ai_company/api/exception_handlers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, and@pytest.mark.slowto categorize tests
Useasyncio_mode = 'auto'for pytest async testing — no manual@pytest.mark.asyncioneeded
Prefer@pytest.mark.parametrizefor testing similar cases in pytest
Files:
tests/unit/api/test_app.pytests/unit/api/test_exception_handlers.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation source in
docs/is built with Zensical and includes design spec (docs/design/— 7 pages), architecture (docs/architecture/), roadmap (docs/roadmap/), and reference (docs/reference/)
Files:
docs/api/templates.mddocs/api/security.mddocs/security.mddocs/api/communication.mddocs/api/index.mddocs/api/observability.mddocs/api/persistence.mddocs/api/engine.mddocs/api/memory.mddocs/api/providers.mddocs/api/config.mddocs/api/tools.mddocs/api/hr.mddocs/api/core.mddocs/api/api.mddocs/api/budget.md
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Fix everything valid found by review agents — never skip valid issues. Fix pre-existing issues in surrounding code, suggestions, and findings adjacent to PR changes.
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Applies to docs/**/*.md : Documentation source in `docs/` is built with Zensical and includes design spec (`docs/design/` — 7 pages), architecture (`docs/architecture/`), roadmap (`docs/roadmap/`), and reference (`docs/reference/`)
Applied to files:
CLAUDE.mdREADME.mdmkdocs.ymlsite/src/pages/index.astro
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: REST API reference is auto-generated in `docs/rest-api.md` and `docs/_generated/api-reference.html` by `scripts/export_openapi.py`
Applied to files:
CLAUDE.mdREADME.mdmkdocs.yml
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Always read the relevant `docs/design/` page before implementing any feature — the design spec is the starting point for architecture, data models, and behavior
Applied to files:
CLAUDE.mdsite/src/pages/index.astro
📚 Learning: 2026-03-13T19:11:39.641Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.641Z
Learning: Applies to src/ai_company/**/*.py : Use event name constants from domain-specific modules under `ai_company.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`)
Applied to files:
src/ai_company/observability/events/api.py
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Run pre-commit hooks: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Applies to .github/.{trivyignore.yaml,grype.yaml} : CVE triage in `.github/.trivyignore.yaml` and `.github/.grype.yaml` for Trivy and Grype security scanning
Applied to files:
.pre-commit-config.yamldocs/security.md
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Applies to src/ai_company/**/*.py : Library reference is auto-generated in `docs/api/` from docstrings via mkdocstrings + Griffe (AST-based, no imports)
Applied to files:
docs/api/index.md
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Applies to docker/Dockerfile : Docker backend uses 3-stage build (builder → setup → distroless runtime), Chainguard Python, non-root (UID 65532), CIS-hardened
Applied to files:
docker/sandbox/Dockerfile
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Applies to docker/Dockerfile.web : Docker web uses `nginxinc/nginx-unprivileged` image with Vue 3 SPA (PrimeVue + Tailwind CSS), SPA routing, and API/WebSocket proxy to backend
Applied to files:
docker/sandbox/Dockerfile
📚 Learning: 2026-03-13T19:11:39.642Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Docker images tagged with version from `pyproject.toml` (`[tool.commitizen].version`), semver, and SHA in CI
Applied to files:
docker/sandbox/Dockerfile
🧬 Code graph analysis (2)
tests/unit/api/test_app.py (1)
tests/unit/api/conftest.py (1)
test_client(626-650)
src/ai_company/api/exception_handlers.py (3)
src/ai_company/api/dto.py (1)
ApiResponse(31-49)src/ai_company/api/errors.py (1)
ApiError(8-20)src/ai_company/persistence/errors.py (3)
RecordNotFoundError(20-26)DuplicateRecordError(29-30)PersistenceError(8-9)
🪛 Checkov (3.2.508)
docker/sandbox/Dockerfile
[low] 1-22: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🔇 Additional comments (24)
docker/sandbox/Dockerfile (1)
1-3: Good hardening with immutable base image pinning.Digest-pinning both Node and Python base images is the right supply-chain control and aligns with the PR security objective.
.github/dependabot.yml (1)
66-78: Sandbox Dependabot coverage is correctly wired.This closes the update-automation gap for
/docker/sandboxand is consistent with your existing docker Dependabot policy.src/ai_company/observability/events/api.py (1)
43-43: Good addition of a dedicated 404 event constant.This keeps route-not-found logging structured and consistent with the existing API event taxonomy.
Based on learnings: "Use event name constants from domain-specific modules under
ai_company.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider)."docs/api/communication.md (1)
1-4: Front matter exclusion is correctly configured.
search.exclude: trueis valid and matches the intended API reference search behavior.docs/api/engine.md (1)
1-4: Search exclusion block looks correct.The front matter is minimal and properly scoped for docs indexing control.
docs/api/budget.md (1)
1-4: YAML front matter is valid and correctly applied.This change is consistent with excluding generated API docs from site search.
docs/api/memory.md (1)
1-4: Looks good.The search-exclusion front matter is correctly structured and should work as intended.
docs/api/observability.md (1)
1-4: Search metadata update is correct.Front matter syntax and key placement are valid.
docs/api/core.md (1)
1-4: Change is clean and consistent.The exclusion flag is correctly represented in YAML front matter.
docs/api/hr.md (1)
1-4: Approved — front matter is correctly added.No issues with structure or intent in this changed segment.
docs/api/security.md (1)
1-4: LGTM! Appropriate search exclusion for auto-generated API reference.The YAML front matter correctly excludes this auto-generated API reference page from search indexing, following MkDocs Material best practices. This prevents low-level implementation details from cluttering search results while keeping the content accessible via navigation.
docs/api/templates.md (1)
1-4: LGTM! Consistent search exclusion applied.The YAML front matter is correctly formatted and consistent with other API reference pages in this PR.
docs/api/api.md (1)
1-4: LGTM! Consistent search exclusion applied.docs/api/persistence.md (1)
1-4: LGTM! Consistent search exclusion applied.docs/api/config.md (1)
1-4: LGTM! Consistent search exclusion applied.docs/api/index.md (1)
1-4: LGTM! Consistent search exclusion applied.Excluding the Library Reference index page along with all the individual API reference pages maintains consistency.
.github/workflows/dast.yml (1)
22-25: LGTM! Excellent clarifying documentation.The comment clearly explains the ZAP action's issue management behavior and why the
issues: writepermission is required. This prevents future confusion about why this permission is needed for a security scanning workflow.docs/api/providers.md (1)
1-4: LGTM! Consistent search exclusion applied.src/ai_company/api/exception_handlers.py (2)
143-143: Good scrub of auth-facing error messages.Using fixed 401/403 bodies here avoids reflecting
exc.detailback to clients while still leaving server-side context in logs.Also applies to: 169-169
197-214: ForwardHTTPExceptionheaders to preserveAllowandRetry-Aftersemantics.The current implementation rebuilds the
Responsewithout preserving exception headers. In Litestar 2.21.1,HTTPExceptionexposes headers via itsheadersattribute (dict[str, str] | None), which contains values likeAllowon 405 orRetry-Afteron 429. Passheaders=exc.headerswhen constructing the newResponseto retain these headers..pre-commit-config.yaml (1)
17-21: Good narrowing of unsafe YAML parsing.Scoping
--unsafetomkdocs.ymlkeeps!!pythonsupport where it's needed without weakening YAML validation for the rest of the repo.tests/unit/api/test_exception_handlers.py (2)
163-217: Good coverage of the new exception mappings.These cases validate the new 404/401/403/400 behavior and confirm the scrubbed/static messages instead of leaking framework details.
219-231: Exercise the real 405 path and add one non-routingHTTPExceptioncase.Line 224 raises
MethodNotAllowedExceptioninside a matchedGEThandler, so this never covers the router path that produces 405 for an unsupported verb. Issue#356also calls out preserving 429 from the newHTTPExceptioncatch-all; useclient.post("/test")here and add one 429 assertion so the suite proves arbitrary framework statuses survive the handler.🧪 Suggested fix for the 405 path
def test_method_not_allowed_maps_to_405(self) -> None: - """MethodNotAllowedException returns 405 via HTTPException handler.""" + """Unsupported methods return 405 via the HTTPException handler.""" `@get`("/test") - async def handler() -> None: - raise MethodNotAllowedException + async def handler() -> str: + return "ok" with TestClient(_make_app(handler)) as client: - resp = client.get("/test") + resp = client.post("/test") assert resp.status_code == 405 body = resp.json() assert body["success"] is False assert body["error"] == "Method Not Allowed"site/src/pages/index.astro (1)
146-209: Nice discoverability improvement.Making the pillar/architecture cards clickable and adding a footer entry for Security lowers the effort to reach the design and hardening docs from the landing page.
Also applies to: 409-409
| # Scorecard: npm install cannot be hash-pinned for CI-only tools; | ||
| # --ignore-scripts mitigates post-install script supply-chain risk. | ||
| npm i --no-save --ignore-scripts wrangler@3.114.17 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does npm install --no-save --ignore-scripts wrangler@3.114.17 pin transitive dependencies or verify them against a repository-controlled checksum without a lockfile?
💡 Result:
No.
wrangler@3.114.17pins only the top-level package version you asked for. Its transitive dependencies are still resolved at install time from whatever semver rangeswrangler(and its dependencies) declare, unless you use a lockfile (package-lock.json/npm-shrinkwrap.json) to freeze the full tree (includingresolved+integrityper package). [1][2]--no-savejust avoids writingwranglerinto yourpackage.json; it doesn’t by itself “pin” the transitive graph.--ignore-scriptsonly prevents lifecycle scripts from running. [1]- Without a lockfile committed to your repo, there is no repo-controlled checksum to verify against. npm will still do integrity checking for what it downloads/uses from cache/registry metadata, but that’s not the same as verifying against a checksum you control in Git. [2][3]
- If you want verification beyond HTTPS + registry metadata, npm also supports verifying registry signatures (when available/configured), but that’s separate from (and not a substitute for) a repository lockfile for reproducibility. [4]
Sources: npm install docs [1], package-lock format (including integrity) [2], npm v5 integrity/cache notes [3], npm registry signature verification docs [4].
wrangler@3.114.17 is not reproducibly pinned—only the top-level version is fixed, while transitive dependencies are resolved at install time without a lockfile or repo-controlled checksums.
--ignore-scripts mitigates one supply-chain vector but does not achieve deterministic installation. Without a committed package-lock.json, the full dependency tree varies per run. Consider either checking in a lockfile for this isolated tool, or downloading and verifying a pre-built artifact before execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pages-preview.yml around lines 236 - 238, The workflow
currently runs the command "npm i --no-save --ignore-scripts wrangler@3.114.17"
which does not produce a reproducible dependency tree; to fix this either commit
and use a lockfile for that tool and install with a deterministic flow (e.g.,
include a package-lock.json and run npm ci or run the install in a small
repo-scoped package.json for wrangler) or replace the npm install with
downloading a pre-built wrangler artifact and verify its checksum before
execution; update the workflow to use the chosen approach and remove the
non-lockfile npm install to ensure deterministic installs.
| ### Pre-Commit Hooks | ||
|
|
||
| Every commit is checked locally before it reaches the remote: | ||
|
|
||
| - **gitleaks** — secret detection on every commit | ||
| - **hadolint** — Dockerfile linting | ||
| - **ruff** — Python linting and formatting | ||
| - **commitizen** — conventional commit message enforcement | ||
| - **Large file prevention** — blocks files over 1 MB | ||
| - **Branch protection** — prevents direct commits to `main` | ||
|
|
There was a problem hiding this comment.
Don't list branch protection as a pre-commit hook.
This section says these checks run locally before code reaches the remote, but branch protection is enforced on GitHub after a push or PR. Move that bullet to the Branch Protection section so the control boundary stays accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/security.md` around lines 157 - 167, Remove the "Branch protection —
prevents direct commits to `main`" bullet from the "Pre-Commit Hooks" section
and add that line (or a short sentence stating that branch protection is
enforced on the remote/GitHub) to the existing "Branch Protection" section so
the control boundary is accurate; update the "Pre-Commit Hooks" list in
docs/security.md (section heading "Pre-Commit Hooks") to include only
locally-run tools (gitleaks, hadolint, ruff, commitizen, large file prevention)
and place the branch protection description under the "Branch Protection"
heading.
| ResponseHeader( | ||
| name="Cache-Control", | ||
| value="no-store", | ||
| ), |
There was a problem hiding this comment.
Don't set Cache-Control: no-store globally.
Placing this in response_headers makes every app response non-cacheable. That misses the narrower policy this hardening is aiming for (no-store on /health, short max-age on the OpenAPI document) and needlessly disables caching for other safe GET responses. Move Cache-Control to a path-specific hook/handler instead of the global header list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/api/app.py` around lines 465 - 468, Remove the global
ResponseHeader("Cache-Control", "no-store") from the app-level response_headers
and instead set Cache-Control on the specific routes: implement a path-specific
response hook or use the handler for the /health endpoint to set "Cache-Control:
no-store", and set a short "Cache-Control: public, max-age=..." on the OpenAPI
document/Docs endpoint; find the existing ResponseHeader entry (ResponseHeader
with name="Cache-Control") in app creation and delete it, then add per-route
middleware or modify the /health handler and the OpenAPI route handler to set
the appropriate Cache-Control headers.
| @pytest.mark.parametrize( | ||
| ("header", "expected"), | ||
| [ | ||
| ("X-Content-Type-Options", "nosniff"), | ||
| ("X-Frame-Options", "DENY"), | ||
| ("Referrer-Policy", "strict-origin-when-cross-origin"), | ||
| ("Permissions-Policy", "geolocation=(), camera=(), microphone=()"), | ||
| ( | ||
| "Strict-Transport-Security", | ||
| "max-age=63072000; includeSubDomains", | ||
| ), | ||
| ("Cross-Origin-Resource-Policy", "same-origin"), | ||
| ("Cache-Control", "no-store"), | ||
| ], | ||
| ) | ||
| def test_security_response_headers( | ||
| self, | ||
| test_client: TestClient[Any], | ||
| header: str, | ||
| expected: str, | ||
| ) -> None: | ||
| response = test_client.get("/docs/openapi.json") | ||
| assert response.headers.get(header) == expected |
There was a problem hiding this comment.
Make Cache-Control assertions path-specific.
This test treats Cache-Control as a global header, but issue #356 requires endpoint-specific behavior: /health should be no-store while the OpenAPI schema should use a short max-age. Keeping ("Cache-Control", "no-store") in this table both misses the /health contract and locks the docs route to the wrong policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/test_app.py` around lines 35 - 57, The test currently treats
"Cache-Control" as a global header in test_security_response_headers; remove
("Cache-Control", "no-store") from the parametrized list and instead add
explicit, path-specific assertions: call test_client.get("/health") and assert
response.headers.get("Cache-Control") == "no-store", and call
test_client.get("/docs/openapi.json") and assert
response.headers.get("Cache-Control") contains a short "max-age=" value (i.e.,
not "no-store"); update test_security_response_headers to only cover the other
static headers and add the two dedicated checks using the existing
test_client.get calls.
- Restore exc.detail passthrough for 401/403 handlers (SynthOrg's own guard/middleware messages are safe and useful for the frontend) - Add error=str(exc) to handle_not_found logging for consistency - Forward exc.headers in handle_http_exception (preserves Allow, Retry-After) - Add 4 new tests: 5xx scrubbing, empty-detail phrase fallback, 401/403 custom detail regression tests (fixes Codecov patch coverage gap) - Update MRO comment with integer status-code key nuance - Add API_ROUTE_NOT_FOUND to CLAUDE.md event constants - Move "Branch protection" from Pre-Commit Hooks to Branch Protection section in docs/security.md (CodeRabbit) - Soften scanner cadence copy on landing page (CodeRabbit)
- Use HTTPStatus.UNAUTHORIZED.phrase / HTTPStatus.FORBIDDEN.phrase as fallbacks instead of hardcoded strings (consistency with handle_http_exception) - Guard HTTPStatus(status) lookup with try/except for non-standard codes - Rewrite 405 test to exercise actual router-level MethodNotAllowed (POST-only route + GET request) instead of raising exception manually - Remove unused MethodNotAllowedException import
There was a problem hiding this comment.
Pull request overview
Repo-wide security hardening across the API, CI, containers, and docs, driven by audit findings (ZAP/DAST, OSSF Scorecard, CodeQL) and aiming to improve error handling correctness, reduce false positives, and document the security posture.
Changes:
- Harden Litestar exception handling (404 for unmatched routes, catch-all for
HTTPExceptionto preserve status codes) and add tests for key mappings. - Add/verify security response headers and expand security documentation + site navigation to surface it.
- Supply-chain/CI hardening: digest-pin sandbox base images, expand Dependabot coverage, tweak pre-commit YAML validation, and mitigate npm install risk in Pages preview.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/api/test_exception_handlers.py |
Adds coverage for Litestar exception mappings (404/401/403/400/405) and HTTPException scrubbing behavior. |
tests/unit/api/test_app.py |
Verifies security-related response headers on OpenAPI schema endpoint. |
src/ai_company/observability/events/api.py |
Adds structured logging event constant for route-not-found. |
src/ai_company/api/exception_handlers.py |
Adds NotFoundException → 404 handler and HTTPException catch-all while preserving status codes. |
src/ai_company/api/app.py |
Adds Cross-Origin-Resource-Policy and Cache-Control to default response headers. |
site/src/pages/index.astro |
Makes key landing-page cards clickable and adds a new Security section + footer link. |
mkdocs.yml |
Adds Security page to MkDocs nav. |
docs/security.md |
Introduces comprehensive security documentation page. |
docs/api/tools.md |
Excludes generated API reference page from search indexing. |
docs/api/templates.md |
Excludes generated API reference page from search indexing. |
docs/api/security.md |
Excludes generated API reference page from search indexing. |
docs/api/providers.md |
Excludes generated API reference page from search indexing. |
docs/api/persistence.md |
Excludes generated API reference page from search indexing. |
docs/api/observability.md |
Excludes generated API reference page from search indexing. |
docs/api/memory.md |
Excludes generated API reference page from search indexing. |
docs/api/index.md |
Excludes library reference landing page from search indexing. |
docs/api/hr.md |
Excludes generated API reference page from search indexing. |
docs/api/engine.md |
Excludes generated API reference page from search indexing. |
docs/api/core.md |
Excludes generated API reference page from search indexing. |
docs/api/config.md |
Excludes generated API reference page from search indexing. |
docs/api/communication.md |
Excludes generated API reference page from search indexing. |
docs/api/budget.md |
Excludes generated API reference page from search indexing. |
docs/api/api.md |
Excludes generated API reference page from search indexing. |
docker/sandbox/Dockerfile |
Pins sandbox base images by digest (supply-chain hardening). |
README.md |
Adds Security documentation link to top-level docs table. |
CLAUDE.md |
Documents Security docs location and adds new API event constant to logging guidance. |
.pre-commit-config.yaml |
Scopes check-yaml --unsafe to mkdocs.yml while excluding it from the safe YAML check. |
.github/workflows/pages-preview.yml |
Adds --ignore-scripts to wrangler install step to reduce supply-chain risk. |
.github/workflows/dast.yml |
Updates comments around ZAP issue permissions behavior. |
.github/dependabot.yml |
Adds Dependabot updates for docker/sandbox. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # The ZAP action creates/updates a single GitHub issue per repo with scan | ||
| # findings (not one per run). Removing issues:write would cause the action | ||
| # to fail. The auto-created issue is updated in-place on subsequent runs. | ||
| issues: write |
| # Scorecard: npm install cannot be hash-pinned for CI-only tools; | ||
| # --ignore-scripts mitigates post-install script supply-chain risk. | ||
| npm i --no-save --ignore-scripts wrangler@3.114.17 |
| ResponseHeader( | ||
| name="Cache-Control", | ||
| value="no-store", | ||
| ), |
- Add test for non-standard HTTPStatus fallback branch (codecov coverage) - Assert Allow header in 405 test per RFC 9110 (Greptile feedback)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ai_company/api/exception_handlers.py`:
- Around line 144-147: The 401/403 handlers currently echo caller-supplied
exc.detail back to clients; update both handlers (the one building Response(...
status_code=403) and the similar block at 171-174) to return fixed messages
using HTTPStatus.FORBIDDEN.phrase and HTTPStatus.UNAUTHORIZED.phrase
respectively instead of exc.detail, and mirror the generic HTTPException path by
forwarding exc.headers when present (i.e., pass headers=exc.headers to Response)
while still wrapping the static phrase in ApiResponse[None](error=...).
In `@tests/unit/api/test_exception_handlers.py`:
- Around line 245-257: The test test_method_not_allowed_maps_to_405 currently
only asserts status and body but must also verify header propagation: update
this test (the POST handler and client GET using TestClient and _make_app) to
assert that the response includes an Allow header with the allowed method(s)
(e.g., "POST") in addition to existing assertions; likewise, update the
corresponding 429 test (the test around lines 276-287 that asserts 429) to
assert a representative Retry-After header is present and correctly formatted so
header forwarding is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06c782a0-ad2e-42c3-a15c-814a4f42ace0
📒 Files selected for processing (2)
src/ai_company/api/exception_handlers.pytests/unit/api/test_exception_handlers.py
📜 Review details
⏰ 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). (5)
- GitHub Check: Agent
- GitHub Check: Build Backend
- GitHub Check: Build Web
- GitHub Check: Greptile Review
- GitHub Check: Test (Python 3.14)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649
Useexcept A, B:(no parentheses) for exception handling — PEP 758 syntax for Python 3.14, enforced by ruff
All public functions require type hints — mypy strict mode enforced
Use Google-style docstrings on all public classes and functions — enforced by ruff D rules
Create new objects instead of mutating existing ones — usecopy.deepcopy()for non-Pydantic collections and wrap withMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; separate mutable-via-copy models for runtime state that evolves
Use@computed_fieldfor derived values in Pydantic models instead of storing + validating redundant fields
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Use Pydantic v2 withBaseModel,model_validator,computed_field,ConfigDictfor all model definitions
Use PEP 649 lazy annotations instead offrom __future__ import annotationsin Python 3.14 files
Files:
tests/unit/api/test_exception_handlers.pysrc/ai_company/api/exception_handlers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, and@pytest.mark.slowto categorize tests
Useasyncio_mode = 'auto'for pytest async testing — no manual@pytest.mark.asyncioneeded
Prefer@pytest.mark.parametrizefor testing similar cases in pytest
Files:
tests/unit/api/test_exception_handlers.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Line length: 88 characters (enforced by ruff)
Files:
tests/unit/api/test_exception_handlers.pysrc/ai_company/api/exception_handlers.py
src/ai_company/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/ai_company/**/*.py: Usefrom ai_company.observability import get_loggerand createlogger = get_logger(__name__)in every module with business logic
Never useimport logging,logging.getLogger(), orprint()in application code
Always useloggeras the variable name for loggers, not_loggerorlog
Use event name constants from domain-specific modules underai_company.observability.events(e.g.,PROVIDER_CALL_STARTfromevents.provider)
Log using structured kwargs:logger.info(EVENT, key=value)— never use string formatting likelogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
Use DEBUG level for object creation, internal flow, and entry/exit of key functions
For non-Pydantic internal collections (registries,BaseTool), usecopy.deepcopy()at construction and wrap withMappingProxyTypefor read-only enforcement
Fordict/listfields in frozen Pydantic models, rely onfrozen=Truefor field reassignment prevention andcopy.deepcopy()at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization)
Usemodel_copy(update=...)for mutable-via-copy runtime state models instead of direct mutations
Never mix static config fields with mutable runtime fields in one Pydantic model
Library reference is auto-generated indocs/api/from docstrings via mkdocstrings + Griffe (AST-based, no imports)
Files:
src/ai_company/api/exception_handlers.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Fix everything valid found by review agents — never skip valid issues. Fix pre-existing issues in surrounding code, suggestions, and findings adjacent to PR changes.
🧬 Code graph analysis (1)
src/ai_company/api/exception_handlers.py (3)
src/ai_company/api/dto.py (1)
ApiResponse(31-49)src/ai_company/api/errors.py (1)
ApiError(8-20)src/ai_company/persistence/errors.py (3)
RecordNotFoundError(20-26)DuplicateRecordError(29-30)PersistenceError(8-9)
🔇 Additional comments (1)
src/ai_company/api/exception_handlers.py (1)
202-225: Nice status-preservingHTTPExceptionfallback.This closes the 405/429 → 500 hole cleanly while still scrubbing 5xx bodies.
| detail = exc.detail or HTTPStatus.FORBIDDEN.phrase | ||
| return Response( | ||
| content=ApiResponse[None](error=detail), | ||
| status_code=403, |
There was a problem hiding this comment.
401/403 handlers still expose auth details.
The hardening goal here was to return static messages for auth failures, but these
branches still surface caller-supplied exc.detail. That means values like
"Write access denied" and "Invalid JWT token" are sent back to the client again,
and the matching tests are currently locking that behavior in. Return fixed
Forbidden / Unauthorized responses here instead, and mirror the header-forwarding
behavior you added for the generic HTTPException path.
Also applies to: 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/api/exception_handlers.py` around lines 144 - 147, The 401/403
handlers currently echo caller-supplied exc.detail back to clients; update both
handlers (the one building Response(... status_code=403) and the similar block
at 171-174) to return fixed messages using HTTPStatus.FORBIDDEN.phrase and
HTTPStatus.UNAUTHORIZED.phrase respectively instead of exc.detail, and mirror
the generic HTTPException path by forwarding exc.headers when present (i.e.,
pass headers=exc.headers to Response) while still wrapping the static phrase in
ApiResponse[None](error=...).
| def test_method_not_allowed_maps_to_405(self) -> None: | ||
| """Router-level MethodNotAllowed returns 405 via HTTPException handler.""" | ||
|
|
||
| @post("/test") | ||
| async def handler() -> str: | ||
| return "ok" | ||
|
|
||
| with TestClient(_make_app(handler)) as client: | ||
| resp = client.get("/test") | ||
| assert resp.status_code == 405 | ||
| body = resp.json() | ||
| assert body["success"] is False | ||
| assert body["error"] == "Method Not Allowed" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The new 405/429 cases don't pin header propagation.
Both tests only assert status/body, so a regression that drops forwarded headers would
still pass. Add explicit header checks here — Allow on the 405 case and a
representative Retry-After header on the 429 case — so the hardening acceptance
criterion is actually covered.
Also applies to: 276-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/api/test_exception_handlers.py` around lines 245 - 257, The test
test_method_not_allowed_maps_to_405 currently only asserts status and body but
must also verify header propagation: update this test (the POST handler and
client GET using TestClient and _make_app) to assert that the response includes
an Allow header with the allowed method(s) (e.g., "POST") in addition to
existing assertions; likewise, update the corresponding 429 test (the test
around lines 276-287 that asserts 429) to assert a representative Retry-After
header is present and correctly formatted so header forwarding is covered.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/api/test_exception_handlers.py (1)
277-288: 🧹 Nitpick | 🔵 TrivialConsider adding a dedicated header propagation test for 429.
This test correctly verifies the HTTP phrase fallback for empty detail, but since the exception is raised without headers, it doesn't exercise
Retry-Afterpropagation. The 405 test proves header forwarding works, but adding an explicit 429 test withRetry-Afterwould fully cover the acceptance criteria.🧪 Suggested additional test for header forwarding
def test_http_exception_429_preserves_retry_after(self) -> None: """HTTPException with Retry-After header forwards it to the response.""" `@get`("/test") async def handler() -> None: raise HTTPException(status_code=429, headers={"Retry-After": "60"}) with TestClient(_make_app(handler)) as client: resp = client.get("/test") assert resp.status_code == 429 assert resp.headers.get("retry-after") == "60"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/test_exception_handlers.py` around lines 277 - 288, Add a new unit test to explicitly verify Retry-After header propagation for 429: create a test function like test_http_exception_429_preserves_retry_after that registers a handler raising HTTPException(status_code=429, headers={"Retry-After": "60"}) (similar to existing test_http_exception_empty_detail_uses_phrase), call the endpoint via TestClient(_make_app(handler)), assert resp.status_code == 429 and assert resp.headers.get("retry-after") == "60" to ensure the exception handling code forwards the Retry-After header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/api/test_exception_handlers.py`:
- Around line 290-307: Remove the unnecessary type ignore in the
test_http_exception_nonstandard_status_uses_fallback test: update the assertion
that checks resp.content.error (from handle_http_exception) by deleting the
trailing "# type: ignore[union-attr]" so mypy can infer the attribute type
correctly; ensure the assertion remains assert resp.content.error == "Request
error" and run tests.
---
Duplicate comments:
In `@tests/unit/api/test_exception_handlers.py`:
- Around line 277-288: Add a new unit test to explicitly verify Retry-After
header propagation for 429: create a test function like
test_http_exception_429_preserves_retry_after that registers a handler raising
HTTPException(status_code=429, headers={"Retry-After": "60"}) (similar to
existing test_http_exception_empty_detail_uses_phrase), call the endpoint via
TestClient(_make_app(handler)), assert resp.status_code == 429 and assert
resp.headers.get("retry-after") == "60" to ensure the exception handling code
forwards the Retry-After header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d324e8b8-ff7c-435f-b891-8e6b38fe4160
📒 Files selected for processing (1)
tests/unit/api/test_exception_handlers.py
📜 Review details
⏰ 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: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649
Useexcept A, B:(no parentheses) for exception handling — PEP 758 syntax for Python 3.14, enforced by ruff
All public functions require type hints — mypy strict mode enforced
Use Google-style docstrings on all public classes and functions — enforced by ruff D rules
Create new objects instead of mutating existing ones — usecopy.deepcopy()for non-Pydantic collections and wrap withMappingProxyTypefor read-only enforcement
Use frozen Pydantic models for config/identity; separate mutable-via-copy models for runtime state that evolves
Use@computed_fieldfor derived values in Pydantic models instead of storing + validating redundant fields
UseNotBlankStrfromcore.typesfor all identifier/name fields (including optional and tuple variants) instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Use Pydantic v2 withBaseModel,model_validator,computed_field,ConfigDictfor all model definitions
Use PEP 649 lazy annotations instead offrom __future__ import annotationsin Python 3.14 files
Files:
tests/unit/api/test_exception_handlers.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, and@pytest.mark.slowto categorize tests
Useasyncio_mode = 'auto'for pytest async testing — no manual@pytest.mark.asyncioneeded
Prefer@pytest.mark.parametrizefor testing similar cases in pytest
Files:
tests/unit/api/test_exception_handlers.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Line length: 88 characters (enforced by ruff)
Files:
tests/unit/api/test_exception_handlers.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T19:11:39.642Z
Learning: Fix everything valid found by review agents — never skip valid issues. Fix pre-existing issues in surrounding code, suggestions, and findings adjacent to PR changes.
🧬 Code graph analysis (1)
tests/unit/api/test_exception_handlers.py (1)
src/ai_company/api/exception_handlers.py (1)
handle_http_exception(202-225)
🪛 GitHub Actions: CI
tests/unit/api/test_exception_handlers.py
[error] 307-307: mypy: Unused 'type: ignore' comment. [unused-ignore]
🔇 Additional comments (6)
tests/unit/api/test_exception_handlers.py (6)
6-12: LGTM!The new imports are appropriate for the added test cases —
postfor the 405 test and the Litestar exception classes for verifying exception-to-response mappings.
163-176: LGTM!Good coverage for the
NotFoundExceptionhandler — confirms unmatched routes return 404 with the expected body structure rather than falling through to 500.
177-229: LGTM!Solid test coverage for both
PermissionDeniedException(403) andNotAuthorizedException(401) — verifying both the default fallback phrases and custom detail preservation.
231-243: LGTM!Confirms
ValidationExceptionmaps to 400 with the static "Validation error" message.
245-258: LGTM!The 405 test now includes the
Allowheader assertion on line 258, which verifies header propagation as required by the security hardening acceptance criteria.
260-275: LGTM!Good test for 5xx detail scrubbing — confirms sensitive upstream details are replaced with the generic message.
There was a problem hiding this comment.
Pull request overview
Repo-wide security hardening in response to ZAP/Scorecard/CodeQL audit findings, focusing on safer HTTP error handling, stronger default response headers, and supply-chain/CI documentation updates.
Changes:
- Harden Litestar exception handling to correctly map framework exceptions (404/405/429/etc.) and improve structured logging for route misses.
- Add/extend security defaults and supply-chain maintenance (response headers, digest-pinned sandbox images, Dependabot coverage, CI install flags).
- Expand and integrate security documentation across MkDocs and the Astro landing page.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/api/test_exception_handlers.py | Adds unit coverage for 404/401/403/400/405 and HTTPException behavior. |
| tests/unit/api/test_app.py | Verifies security response headers on the OpenAPI endpoint. |
| src/ai_company/observability/events/api.py | Adds API_ROUTE_NOT_FOUND event constant for structured logging. |
| src/ai_company/api/exception_handlers.py | Adds explicit handlers for Litestar NotFound/HTTPException and tweaks auth/forbidden messaging. |
| src/ai_company/api/app.py | Adds global CORP and Cache-Control security headers. |
| site/src/pages/index.astro | Adds a Security section and makes feature cards clickable, plus footer Security link. |
| mkdocs.yml | Adds Security page to MkDocs navigation. |
| docs/security.md | Introduces comprehensive security documentation page. |
| docs/api/tools.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/templates.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/security.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/providers.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/persistence.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/observability.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/memory.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/index.md | Excludes library reference index from search indexing. |
| docs/api/hr.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/engine.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/core.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/config.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/communication.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/budget.md | Excludes auto-generated API reference page from search indexing. |
| docs/api/api.md | Excludes auto-generated API reference page from search indexing. |
| docker/sandbox/Dockerfile | Pins sandbox base images by SHA-256 digest. |
| README.md | Adds Security link to documentation table. |
| CLAUDE.md | Documents new Security docs page and references API_ROUTE_NOT_FOUND in logging guidance. |
| .pre-commit-config.yaml | Scopes check-yaml --unsafe to mkdocs.yml only. |
| .github/workflows/pages-preview.yml | Adds --ignore-scripts for wrangler install in deploy step (Scorecard mitigation). |
| .github/workflows/dast.yml | Updates commentary around ZAP issue permissions usage. |
| .github/dependabot.yml | Adds Dependabot updates for /docker/sandbox. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scrubbed message; 4xx responses pass through the exception detail | ||
| (authored by SynthOrg's guards/middleware and user-safe). Detailed | ||
| error context is logged server-side for all status codes. |
| def handle_permission_denied( | ||
| request: Request[Any, Any, Any], | ||
| exc: PermissionDeniedException, | ||
| ) -> Response[ApiResponse[None]]: | ||
| """Map ``PermissionDeniedException`` to 403.""" | ||
| _log_error(request, exc, status=403) | ||
| detail = exc.detail or "Forbidden" | ||
| detail = exc.detail or HTTPStatus.FORBIDDEN.phrase | ||
| return Response( | ||
| content=ApiResponse[None](error=detail), | ||
| status_code=403, | ||
| ) | ||
|
|
||
|
|
||
| def handle_validation_error( | ||
| request: Request[Any, Any, Any], | ||
| exc: ValidationException, | ||
| ) -> Response[ApiResponse[None]]: | ||
| """Map ``ValidationException`` to 400.""" | ||
| _log_error(request, exc, status=400) | ||
| return Response( | ||
| content=ApiResponse[None]( | ||
| error="Validation error", | ||
| ), | ||
| status_code=400, | ||
| ) | ||
|
|
||
|
|
||
| def handle_not_authorized( | ||
| request: Request[Any, Any, Any], | ||
| exc: NotAuthorizedException, | ||
| ) -> Response[ApiResponse[None]]: | ||
| """Map ``NotAuthorizedException`` to 401.""" | ||
| _log_error(request, exc, status=401) | ||
| detail = exc.detail or "Authentication required" | ||
| detail = exc.detail or HTTPStatus.UNAUTHORIZED.phrase | ||
| return Response( | ||
| content=ApiResponse[None](error=detail), | ||
| status_code=401, | ||
| ) |
| ResponseHeader( | ||
| name="Cross-Origin-Resource-Policy", | ||
| value="same-origin", | ||
| ), | ||
| ResponseHeader( | ||
| name="Cache-Control", | ||
| value="no-store", | ||
| ), |
| # Scorecard: npm install cannot be hash-pinned for CI-only tools; | ||
| # --ignore-scripts mitigates post-install script supply-chain risk. | ||
| npm i --no-save --ignore-scripts wrangler@3.114.17 | ||
| npx wrangler pages deploy _site --project-name=synthorg-pr-preview --branch="pr-${PR_NUMBER}" |
🤖 I have created a release *beep* *boop* --- ## [0.1.3](v0.1.2...v0.1.3) (2026-03-13) ### Features * add Mem0 memory backend adapter ([#345](#345)) ([2788db8](2788db8)), closes [#206](#206) * centralized single-writer TaskEngine with full CRUD API ([#328](#328)) ([9c1a3e1](9c1a3e1)) * incremental AgentEngine → TaskEngine status sync ([#331](#331)) ([7a68d34](7a68d34)), closes [#323](#323) * web dashboard pages — views, components, tests, and review fixes ([#354](#354)) ([b165ec4](b165ec4)) * web dashboard with Vue 3 + PrimeVue + Tailwind CSS ([#347](#347)) ([06416b1](06416b1)) ### Bug Fixes * harden coordination pipeline with validators, logging, and fail-fast ([#333](#333)) ([2f10d49](2f10d49)), closes [#205](#205) * repo-wide security hardening from ZAP, Scorecard, and CodeQL audit ([#357](#357)) ([27eb288](27eb288)) ### CI/CD * add pip-audit, hadolint, OSSF Scorecard, ZAP DAST, and pre-push hooks ([#350](#350)) ([2802d20](2802d20)) * add workflow_dispatch trigger to PR Preview for Dependabot PRs ([#326](#326)) ([4c7b6d9](4c7b6d9)) * bump astral-sh/setup-uv from 7.4.0 to 7.5.0 in the minor-and-patch group ([#335](#335)) ([98dd8ca](98dd8ca)) ### Maintenance * bump the minor-and-patch group across 1 directory with 3 updates ([#352](#352)) ([031b1c9](031b1c9)) * **deps:** bump devalue from 5.6.3 to 5.6.4 in /site in the npm_and_yarn group across 1 directory ([#324](#324)) ([9a9c600](9a9c600)) * migrate docs build from MkDocs to Zensical ([#330](#330)) ([fa8bf1d](fa8bf1d)), closes [#329](#329) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Repo-wide security hardening based on audit findings from ZAP DAST, OSSF Scorecard, and CodeQL alerts.
NotFoundException→ 404 handler (prevents 500 on unmatched routes), addCross-Origin-Resource-PolicyandCache-Controlresponse headers, addHTTPExceptioncatch-all handler preserving correct status codes (405, 429, etc.)--ignore-scriptsto wrangler CI install, add Dependabot coverage fordocker/sandboxexc.detailleakage), wireAPI_ROUTE_NOT_FOUNDevent constant for structured logging, add MRO resolution commentdocs/security.mdpage, landing page Security section with clickable cards, library reference search exclusion, README Security link, scopedcheck-yaml --unsafetomkdocs.ymlonlyci-pass) and PR reviewsCloses #356
Test plan
uv run ruff check src/ tests/— all checks passeduv run mypy src/ tests/— no issues in 917 filesuv run pytest tests/ -n auto --cov=ai_company --cov-fail-under=80— 7465 passed, 94.69% coveragetest_unmatched_route_returns_404,test_method_not_allowed_maps_to_405,test_litestar_permission_denied_maps_to_403,test_litestar_not_authorized_maps_to_401,test_litestar_validation_exception_maps_to_400,test_security_response_headers[Strict-Transport-Security]Review coverage
Pre-reviewed by 7 specialized agents (code-reviewer, python-reviewer, pr-test-analyzer, silent-failure-hunter, logging-audit, security-reviewer, docs-consistency). 10 findings implemented across all severity levels.