feat: governed external API/data access tool (#1991)#2032
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📜 Recent 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). (13)
🧰 Additional context used📓 Path-based instructions (5)tests/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
tests/conformance/persistence/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/synthorg/persistence/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
src/synthorg/!(persistence)/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-05-05T09:04:46.195ZApplied to files:
🔇 Additional comments (4)
WalkthroughThis PR implements a governed external API/data access tool, enabling agents to call external APIs with security controls. It adds one-shot approval consumption tracking ( |
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 introduces a governed framework for external API and data access, replacing ad-hoc methods with a secure, first-class tool. By integrating with the existing connection catalog and approval infrastructure, it ensures that external requests are authenticated, rate-limited, and egress-constrained. Sensitive operations are automatically gated, requiring human intervention and ensuring that grants are consumed atomically to prevent unauthorized re-use. 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 the 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 counterproductive. 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. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
![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 introduces a governed external API/data access tool, providing agents with brokered credentials, rate limiting, and SSRF protection via DNS pinning. It features a one-shot approval mechanism for sensitive or write operations, with corresponding updates to the connection catalog, persistence layers (Postgres/SQLite), and frontend management UI. Feedback identifies a security vulnerability in header merging that could allow Host header injection, a mismatch between documentation and implementation regarding path-level security boundaries, and a performance concern involving the per-request instantiation of HTTP clients.
| merged_headers = self._broker_headers(conn, await self._credentials(conn)) | ||
| merged_headers = {**args.headers, **merged_headers} |
There was a problem hiding this comment.
The tool merges agent-supplied headers with brokered authentication headers using a simple dictionary update. This is case-sensitive at the dictionary level, which can lead to duplicate or conflicting headers in the outgoing request if the agent provides a header with a different case (e.g., authorization vs Authorization). Additionally, restricted headers such as Host, Content-Length, and Transfer-Encoding should be blocked to prevent Host header injection SSRF and other protocol-level interference. Overriding the Host header could allow an agent to reach different virtual hosts on the same server, potentially bypassing intended boundaries.
merged_headers = self._broker_headers(conn, await self._credentials(conn))
# Merge agent headers while protecting brokered headers and blocking restricted keys
restricted = {"host", "content-length", "transfer-encoding"}
auth_keys = {k.lower() for k in merged_headers}
final_headers = {
k: v for k, v in args.headers.items()
if k.lower() not in restricted and k.lower() not in auth_keys
}
merged_headers = {**final_headers, **merged_headers}| cannot use ``..`` traversal to reach paths outside the connection's | ||
| base-URL prefix. | ||
| """ |
There was a problem hiding this comment.
The docstring states that the tool "cannot use .. traversal to reach paths outside the connection's base-URL prefix," implying that the base_url path acts as a security boundary. However, the implementation only enforces host-level constraints for absolute URLs (line 234). An agent can bypass a path-level "prefix" by providing an absolute URL on the same host. If the base_url path is intended to be a security boundary, the tool should validate that the resolved URL starts with the base_url prefix. Otherwise, the docstring should be clarified to reflect that the constraint is host-level.
| transport=transport, | ||
| follow_redirects=False, | ||
| ) as client, | ||
| client.stream( |
There was a problem hiding this comment.
A new httpx.AsyncClient is instantiated for every request. This is a performance anti-pattern as it prevents the reuse of connection pools, resulting in the overhead of a full TCP/TLS handshake for every tool execution. While the request-scoped PinnedDnsTransport makes pooling across different hosts/IPs complex, the provider should ideally reuse a client instance where possible (e.g., for requests not requiring pinning) or use a custom transport that manages its own pooling while supporting per-request DNS pinning.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/persistence/postgres/approval_repo.py (1)
52-70:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMake
consumed_atwrite-once in the UPSERT.This
ON CONFLICTclause can clear a previously-setconsumed_at: a staleApprovalItemsaved later withconsumed_at=Nonewill overwrite an already-consumed row and resurrect the grant. That breaks the one-shot approval guarantee.Suggested SQL fix
ON CONFLICT (id) DO UPDATE SET action_type = EXCLUDED.action_type, title = EXCLUDED.title, description = EXCLUDED.description, requested_by = EXCLUDED.requested_by, risk_level = EXCLUDED.risk_level, source = EXCLUDED.source, status = EXCLUDED.status, expires_at = EXCLUDED.expires_at, decided_at = EXCLUDED.decided_at, decided_by = EXCLUDED.decided_by, decision_reason = EXCLUDED.decision_reason, task_id = EXCLUDED.task_id, evidence_package = EXCLUDED.evidence_package, metadata = EXCLUDED.metadata, - consumed_at = EXCLUDED.consumed_at + consumed_at = COALESCE(approvals.consumed_at, EXCLUDED.consumed_at)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/persistence/postgres/approval_repo.py` around lines 52 - 70, The UPSERT in _APPROVALS_UPSERT_SQL currently allows a later row with consumed_at = NULL to overwrite an existing consumed_at, resurrecting a consumed approval; change the ON CONFLICT update for consumed_at to be write-once by keeping the existing value when present, e.g. replace the line setting consumed_at with a conditional that preserves the current row's value (use COALESCE(approvals.consumed_at, EXCLUDED.consumed_at) or CASE WHEN approvals.consumed_at IS NULL THEN EXCLUDED.consumed_at ELSE approvals.consumed_at END) so consumed_at is only set if the existing value is NULL.src/synthorg/persistence/sqlite/approval_repo.py (1)
46-62:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPrevent upsert from reopening consumed approvals.
ON CONFLICTcurrently assignsconsumed_at = excluded.consumed_at, so a later save carryingNonecan clear a previously consumed grant. Preserve existing non-nullconsumed_atto keep one-shot approval consumption irreversible.Suggested fix
- consumed_at = excluded.consumed_at + consumed_at = COALESCE(approvals.consumed_at, excluded.consumed_at)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/persistence/sqlite/approval_repo.py` around lines 46 - 62, The upsert currently allows a later save with consumed_at = NULL to overwrite a previously set consumption; in the ON CONFLICT ... DO UPDATE block in approval_repo.py change the consumed_at assignment to preserve an existing non-null value (e.g. use COALESCE(consumed_at, excluded.consumed_at) or a CASE that keeps the existing consumed_at when it's not NULL) so once consumed_at is set it cannot be cleared by subsequent upserts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/api/_approval_expiration.py`:
- Around line 129-179: The current branch writes expired = item.model_copy(...)
and calls self._repo.save(expired) based only on cached state, which can clobber
a concurrent APPROVED update; instead, when self._repo is not None perform a
repo-side CAS transition (use the repo's transition_if or expire_if_pending
method to atomically change PENDING -> EXPIRED) and only proceed to update
self._items, emit APPROVAL_STATUS_TRANSITIONED / API_APPROVAL_EXPIRED, call
record_approval_decision and invoke self._on_expire if that CAS returned
success; if the CAS fails (no transition) treat as a no-op and return the
current repo/cache item rather than overwriting. Ensure you still handle the
_repo is None path the same way (local-only expiration).
In `@src/synthorg/api/controllers/connections.py`:
- Around line 143-145: UpdateConnectionRequest.sensitive currently accepts JSON
null and later maps None to _UNSET, causing malformed PATCHes to be treated as
omissions; change the model and validation so explicit null is rejected at the
boundary: make UpdateConnectionRequest.sensitive non-nullable (remove | None),
or add a Pydantic validator on UpdateConnectionRequest.sensitive that raises a
validation error when the value is None, and remove the code-path that maps None
to _UNSET in the patch processing (the logic that currently converts None to
_UNSET around the sensitive handling). Reference:
UpdateConnectionRequest.sensitive and the mapping logic that sets _UNSET for
sensitive.
In `@src/synthorg/tools/external_api/external_api_tool.py`:
- Around line 250-253: The _has_dot_segment function currently inspects raw URL
path segments so encoded dot-segments like "%2e" or "%2e%2e" are missed; update
it to percent-decode the path before splitting and checking for "." or ".." (use
urllib.parse.unquote on urlsplit(url).path) so any encoded traversal sequences
are detected and flagged by _has_dot_segment.
- Around line 368-396: The approval lookup currently accepts any
approved/unconsumed item with a matching signature allowing cross-principal
reuse; update the checks in the approval resolution logic (the branch using
_approval_store.get and the candidates loop that calls
_approval_store.list_items, signature.matches and
ApprovalSignature.from_metadata) to also validate the approval is bound to the
current caller/task identity: require that the approval item’s metadata (e.g.,
owner/principal/task id stored in item.metadata) matches the current caller
identity (derived from args.connection or the signature/caller context) before
accepting it; apply the same owner check when an explicit args.approval_id is
provided (in addition to the existing signature and consumed_at checks) and when
iterating candidates so only approvals whose metadata owner equals the current
caller/task are returned.
In `@tests/conformance/persistence/test_connection_repositories.py`:
- Around line 85-93: The test claims "survives upsert" but never performs an
upsert: after creating `sensitive =
_connection(name="conn-secret").model_copy(update={"sensitive": True})` and
saving it via `await backend.connections.save(sensitive)`, add a second save of
the same object (call `await backend.connections.save(sensitive)` again or save
a modified copy with same id) then re-fetch with `await
backend.connections.get(NotBlankStr("conn-secret"))` and re-assert
`fetched.sensitive is True` to validate the flag persists across an upsert.
In `@tests/unit/tools/external_api/test_signature.py`:
- Around line 63-69: The test test_credential_headers_excluded currently only
varies the "Accept" header (which should be part of the signature) and thus
never exercises credential-header exclusion; update the test so it asserts that
changing credential headers (e.g., "Authorization" or other auth-related
headers) does NOT change the signature while still asserting that changing
normal headers like "Accept" does change it. Concretely, add an assertion using
_sig(headers={"Authorization": "tokenA"}) == _sig(headers={"Authorization":
"tokenB"}) to verify exclusion, and keep the existing inequality check for _sig
with different "Accept" values.
---
Outside diff comments:
In `@src/synthorg/persistence/postgres/approval_repo.py`:
- Around line 52-70: The UPSERT in _APPROVALS_UPSERT_SQL currently allows a
later row with consumed_at = NULL to overwrite an existing consumed_at,
resurrecting a consumed approval; change the ON CONFLICT update for consumed_at
to be write-once by keeping the existing value when present, e.g. replace the
line setting consumed_at with a conditional that preserves the current row's
value (use COALESCE(approvals.consumed_at, EXCLUDED.consumed_at) or CASE WHEN
approvals.consumed_at IS NULL THEN EXCLUDED.consumed_at ELSE
approvals.consumed_at END) so consumed_at is only set if the existing value is
NULL.
In `@src/synthorg/persistence/sqlite/approval_repo.py`:
- Around line 46-62: The upsert currently allows a later save with consumed_at =
NULL to overwrite a previously set consumption; in the ON CONFLICT ... DO UPDATE
block in approval_repo.py change the consumed_at assignment to preserve an
existing non-null value (e.g. use COALESCE(consumed_at, excluded.consumed_at) or
a CASE that keeps the existing consumed_at when it's not NULL) so once
consumed_at is set it cannot be cleared by subsequent upserts.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e523be8-7699-4ca0-b9a2-556770af5a34
📒 Files selected for processing (81)
docs/design/integrations.mddocs/design/tools.mdscripts/schema_drift_baseline.txtsrc/synthorg/api/_approval_expiration.pysrc/synthorg/api/approval_store.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/approval/protocol.pysrc/synthorg/core/approval.pysrc/synthorg/core/enums.pysrc/synthorg/engine/_security_factory.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/integrations/connections/_oauth_rotation.pysrc/synthorg/integrations/connections/catalog.pysrc/synthorg/integrations/connections/models.pysrc/synthorg/integrations/health/checks/generic_http.pysrc/synthorg/observability/events/external_api.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/persistence/approval_protocol.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/persistence/postgres/connection_repo.pysrc/synthorg/persistence/postgres/revisions/20260521000001_external_api_governed_access.sqlsrc/synthorg/persistence/postgres/schema.sqlsrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/sqlite/revisions/20260521000001_external_api_governed_access.sqlsrc/synthorg/persistence/sqlite/schema.sqlsrc/synthorg/security/action_type_mapping.pysrc/synthorg/security/action_types.pysrc/synthorg/security/risk_scorer.pysrc/synthorg/security/rules/risk_classifier.pysrc/synthorg/security/timeout/risk_tier_classifier.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/definitions/external_api.pysrc/synthorg/settings/enums.pysrc/synthorg/tools/_dns_pinning.pysrc/synthorg/tools/external_api/__init__.pysrc/synthorg/tools/external_api/_args.pysrc/synthorg/tools/external_api/_credentials.pysrc/synthorg/tools/external_api/_runtime.pysrc/synthorg/tools/external_api/_signature.pysrc/synthorg/tools/external_api/errors.pysrc/synthorg/tools/external_api/external_api_tool.pysrc/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/tools/external_api/provider.pysrc/synthorg/tools/external_api/provider_factory.pysrc/synthorg/tools/permissions.pysrc/synthorg/workers/runtime_builder.pytests/conformance/persistence/test_approval_repository.pytests/conformance/persistence/test_connection_repositories.pytests/e2e/test_external_api_governance_e2e.pytests/unit/api/test_approval_store.pytests/unit/approval/test_protocol.pytests/unit/core/test_approval.pytests/unit/core/test_enums.pytests/unit/hr/test_registry_autonomy.pytests/unit/meta/test_approval_repo.pytests/unit/observability/test_events.pytests/unit/security/test_action_types.pytests/unit/tools/external_api/test_credentials.pytests/unit/tools/external_api/test_errors.pytests/unit/tools/external_api/test_external_api_tool.pytests/unit/tools/external_api/test_httpx_provider.pytests/unit/tools/external_api/test_signature.pyweb/src/__tests__/helpers/factories.tsweb/src/__tests__/stores/connections.test.tsweb/src/api/types/enum-values.gen.tsweb/src/api/types/openapi.gen.tsweb/src/mocks/handlers/approvals.tsweb/src/mocks/handlers/connections.tsweb/src/pages/approvals/ApprovalCard.stories.tsxweb/src/pages/approvals/ApprovalDetailDrawer.stories.tsxweb/src/pages/approvals/ApprovalTimeline.stories.tsxweb/src/pages/connections/ConnectionCard.stories.tsxweb/src/pages/connections/ConnectionFormModal.stories.tsxweb/src/pages/connections/ConnectionFormModal.tsxweb/src/pages/mcp-catalog/McpInstallWizard.stories.tsxweb/src/pages/oauth-apps/OauthAppCard.stories.tsxweb/src/pages/settings/utils.tsweb/src/stores/approvals.tsweb/src/utils/constants.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (22)
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README + public docs sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers. Seedata/README.md.
Files:
docs/design/integrations.mddocs/design/tools.md
**/*.{md,d2}
📄 CodeRabbit inference engine (CLAUDE.md)
D2 for architecture / nested containers, mermaid for flowcharts / sequence / pipelines. Markdown tables for tabular data. D2 theme 200 (Dark Mauve), D2 CLI pinned to v0.7.1 in CI.
Files:
docs/design/integrations.mddocs/design/tools.md
src/synthorg/!(persistence)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL.
Files:
src/synthorg/security/rules/risk_classifier.pysrc/synthorg/observability/events/external_api.pysrc/synthorg/security/timeout/risk_tier_classifier.pysrc/synthorg/security/risk_scorer.pysrc/synthorg/settings/enums.pysrc/synthorg/tools/external_api/_runtime.pysrc/synthorg/tools/permissions.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/tools/external_api/__init__.pysrc/synthorg/approval/protocol.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/security/action_type_mapping.pysrc/synthorg/core/enums.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/tools/external_api/errors.pysrc/synthorg/core/approval.pysrc/synthorg/settings/definitions/external_api.pysrc/synthorg/tools/external_api/_credentials.pysrc/synthorg/tools/external_api/_args.pysrc/synthorg/tools/external_api/provider_factory.pysrc/synthorg/security/action_types.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/tools/external_api/_signature.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/tools/_dns_pinning.pysrc/synthorg/integrations/health/checks/generic_http.pysrc/synthorg/tools/external_api/provider.pysrc/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/integrations/connections/_oauth_rotation.pysrc/synthorg/engine/_security_factory.pysrc/synthorg/integrations/connections/models.pysrc/synthorg/tools/external_api/external_api_tool.pysrc/synthorg/workers/runtime_builder.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/integrations/connections/catalog.pysrc/synthorg/api/approval_store.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use DB > env > code default viaSettingsService/ConfigResolver(Cat-1) or env > code default (Cat-2,read_only_post_init); Cat-3 bootstrap secrets are pure env at the boot site. YAML is a company-template ingestion format, not a precedence tier. Noos.environ.getoutside startup; pre-init Cat-2 reads usesettings.bootstrap_resolver.resolve_init_value.
No hardcoded numeric values; numerics live insettings/definitions/. Allowlist: 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the formNAME: int|float|Final|Final[int]|Final[float] = literal. Enforced byscripts/check_no_magic_numbers.py.
Comments WHY only; no reviewer citations / issue back-refs / migration framing. Enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py.
Nofrom __future__ import annotations(3.14 has PEP 649). PEP 758 except:except A, B:no parens unless binding.
Type hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines.
Errors:<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeError/etc directly. Enforced bycheck_domain_error_hierarchy.py.
Pydantic v2 frozen +extra="forbid"on every frozen model project-wide (gatecheck_frozen_model_extra_forbid.py;@computed_fieldauto-exempt, per-line# lint-allow: frozen-extra-forbid -- <reason>forextra="allow"/"ignore"boundaries);@computed_fieldfor derived;NotBlankStrfor identifiers.
Args models at every system boundary;parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.py.
Immutability:model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries.
Async:asyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError).
Clock seam:clock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out st...
Files:
src/synthorg/security/rules/risk_classifier.pysrc/synthorg/observability/events/external_api.pysrc/synthorg/security/timeout/risk_tier_classifier.pysrc/synthorg/security/risk_scorer.pysrc/synthorg/settings/enums.pysrc/synthorg/tools/external_api/_runtime.pysrc/synthorg/tools/permissions.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/tools/external_api/__init__.pysrc/synthorg/approval/protocol.pysrc/synthorg/persistence/approval_protocol.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/security/action_type_mapping.pysrc/synthorg/core/enums.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/tools/external_api/errors.pysrc/synthorg/core/approval.pysrc/synthorg/settings/definitions/external_api.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/tools/external_api/_credentials.pysrc/synthorg/tools/external_api/_args.pysrc/synthorg/tools/external_api/provider_factory.pysrc/synthorg/security/action_types.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/tools/external_api/_signature.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/tools/_dns_pinning.pysrc/synthorg/integrations/health/checks/generic_http.pysrc/synthorg/tools/external_api/provider.pysrc/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/integrations/connections/_oauth_rotation.pysrc/synthorg/engine/_security_factory.pysrc/synthorg/integrations/connections/models.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/tools/external_api/external_api_tool.pysrc/synthorg/workers/runtime_builder.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/integrations/connections/catalog.pysrc/synthorg/api/approval_store.pysrc/synthorg/persistence/postgres/connection_repo.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/security/rules/risk_classifier.pysrc/synthorg/observability/events/external_api.pysrc/synthorg/security/timeout/risk_tier_classifier.pysrc/synthorg/security/risk_scorer.pysrc/synthorg/settings/enums.pysrc/synthorg/tools/external_api/_runtime.pysrc/synthorg/tools/permissions.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/tools/external_api/__init__.pysrc/synthorg/approval/protocol.pysrc/synthorg/persistence/approval_protocol.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/security/action_type_mapping.pysrc/synthorg/core/enums.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/tools/external_api/errors.pysrc/synthorg/core/approval.pysrc/synthorg/settings/definitions/external_api.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/tools/external_api/_credentials.pysrc/synthorg/tools/external_api/_args.pysrc/synthorg/tools/external_api/provider_factory.pysrc/synthorg/security/action_types.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/tools/external_api/_signature.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/tools/_dns_pinning.pysrc/synthorg/integrations/health/checks/generic_http.pysrc/synthorg/tools/external_api/provider.pysrc/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/integrations/connections/_oauth_rotation.pysrc/synthorg/engine/_security_factory.pysrc/synthorg/integrations/connections/models.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/tools/external_api/external_api_tool.pysrc/synthorg/workers/runtime_builder.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/integrations/connections/catalog.pysrc/synthorg/api/approval_store.pysrc/synthorg/persistence/postgres/connection_repo.py
web/src/**/*.{js,jsx,ts,tsx,mts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{js,jsx,ts,tsx,mts}: Always usecreateLoggerfrom@/lib/logger; never bareconsole.warn/console.error/console.debugin application code. Variable name must always belog. Onlylogger.tsitself may use bare console methods. Uselog.debug()(DEV-only, stripped in production),log.warn(),log.error().
Pass dynamic/untrusted values as separate args to logger calls (not interpolated into the message string) so they go throughsanitizeArg
Attacker-controlled fields inside structured objects must be wrapped insanitizeForLog()before embedding in log calls
Error-code constants (MANDATORY): importErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported from the generatedweb/src/api/types/error-codes.gen.ts). Discriminate onErrorCode.<NAME>, never on raw integer literals.
Use@eslint-react/web-api-no-leaked-fetchto detectfetch()in effects withoutAbortControllercleanup
Files:
web/src/pages/approvals/ApprovalCard.stories.tsxweb/src/pages/approvals/ApprovalDetailDrawer.stories.tsxweb/src/pages/oauth-apps/OauthAppCard.stories.tsxweb/src/mocks/handlers/approvals.tsweb/src/pages/connections/ConnectionCard.stories.tsxweb/src/pages/approvals/ApprovalTimeline.stories.tsxweb/src/pages/mcp-catalog/McpInstallWizard.stories.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/settings/utils.tsweb/src/pages/connections/ConnectionFormModal.stories.tsxweb/src/api/types/enum-values.gen.tsweb/src/utils/constants.tsweb/src/mocks/handlers/connections.tsweb/src/__tests__/stores/connections.test.tsweb/src/pages/connections/ConnectionFormModal.tsxweb/src/stores/approvals.tsweb/src/api/types/openapi.gen.ts
web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{jsx,tsx}: Use@eslint-react/no-leaked-conditional-renderingto catch the{count && <Foo />}bug where0renders verbatim. ForReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...).
Use@eslint-react/globalsto restrictwindow/document/localStorage/ etc. inside render. Hoist offenders into auseCallbackevent handler, auseEffect, or auseSyncExternalStore-backed hook.
Files:
web/src/pages/approvals/ApprovalCard.stories.tsxweb/src/pages/approvals/ApprovalDetailDrawer.stories.tsxweb/src/pages/oauth-apps/OauthAppCard.stories.tsxweb/src/pages/connections/ConnectionCard.stories.tsxweb/src/pages/approvals/ApprovalTimeline.stories.tsxweb/src/pages/mcp-catalog/McpInstallWizard.stories.tsxweb/src/pages/connections/ConnectionFormModal.stories.tsxweb/src/pages/connections/ConnectionFormModal.tsx
web/src/**/*.{ts,tsx,mts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,mts}: Use@typescript-eslint/no-floating-promisesto forbid unawaited promises so async work cannot survive the test that scheduled it and trip the active-handle gate
Use@typescript-eslint/no-misused-promises(withchecksVoidReturn: { attributes: false }) to forbid passing async functions where the callsite ignores the returned promise. React 19asyncevent handlers stay allowed via theattributes: falseexemption.
Files:
web/src/pages/approvals/ApprovalCard.stories.tsxweb/src/pages/approvals/ApprovalDetailDrawer.stories.tsxweb/src/pages/oauth-apps/OauthAppCard.stories.tsxweb/src/mocks/handlers/approvals.tsweb/src/pages/connections/ConnectionCard.stories.tsxweb/src/pages/approvals/ApprovalTimeline.stories.tsxweb/src/pages/mcp-catalog/McpInstallWizard.stories.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/settings/utils.tsweb/src/pages/connections/ConnectionFormModal.stories.tsxweb/src/api/types/enum-values.gen.tsweb/src/utils/constants.tsweb/src/mocks/handlers/connections.tsweb/src/__tests__/stores/connections.test.tsweb/src/pages/connections/ConnectionFormModal.tsxweb/src/stores/approvals.tsweb/src/api/types/openapi.gen.ts
web/src/**/*.stories.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Storybook 10 is ESM-only; essentials are built into core, but
@storybook/addon-docsis now separate; imports moved tostorybook/testandstorybook/actions
Files:
web/src/pages/approvals/ApprovalCard.stories.tsxweb/src/pages/approvals/ApprovalDetailDrawer.stories.tsxweb/src/pages/oauth-apps/OauthAppCard.stories.tsxweb/src/pages/connections/ConnectionCard.stories.tsxweb/src/pages/approvals/ApprovalTimeline.stories.tsxweb/src/pages/mcp-catalog/McpInstallWizard.stories.tsxweb/src/pages/connections/ConnectionFormModal.stories.tsx
web/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Reuse
web/src/components/ui/design tokens only in web components; detail inweb/CLAUDE.md.
Files:
web/src/pages/approvals/ApprovalCard.stories.tsxweb/src/pages/approvals/ApprovalDetailDrawer.stories.tsxweb/src/pages/oauth-apps/OauthAppCard.stories.tsxweb/src/mocks/handlers/approvals.tsweb/src/pages/connections/ConnectionCard.stories.tsxweb/src/pages/approvals/ApprovalTimeline.stories.tsxweb/src/pages/mcp-catalog/McpInstallWizard.stories.tsxweb/src/__tests__/helpers/factories.tsweb/src/pages/settings/utils.tsweb/src/pages/connections/ConnectionFormModal.stories.tsxweb/src/api/types/enum-values.gen.tsweb/src/utils/constants.tsweb/src/mocks/handlers/connections.tsweb/src/__tests__/stores/connections.test.tsweb/src/pages/connections/ConnectionFormModal.tsxweb/src/stores/approvals.tsweb/src/api/types/openapi.gen.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers:@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% min.
Windows: unit tests useWindowsSelectorEventLoopPolicy(3.14 IOCP teardown race). Subprocess tests override back.
Test doubles: ladder in conventions.md section 12.1.FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags. BareMagicMockat typed boundary (constructor / fn arg / annotated local / typed fixture return) blocked byscripts/check_mock_spec.py(zero-tolerance).
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)). Flaky: NEVER skip/xfail; fix fundamentally. Useasyncio.Event().wait()notsleep(large).
Files:
tests/unit/tools/external_api/test_signature.pytests/unit/core/test_approval.pytests/unit/meta/test_approval_repo.pytests/unit/observability/test_events.pytests/conformance/persistence/test_connection_repositories.pytests/unit/api/test_approval_store.pytests/unit/tools/external_api/test_errors.pytests/unit/tools/external_api/test_httpx_provider.pytests/unit/hr/test_registry_autonomy.pytests/unit/security/test_action_types.pytests/conformance/persistence/test_approval_repository.pytests/unit/approval/test_protocol.pytests/unit/core/test_enums.pytests/e2e/test_external_api_governance_e2e.pytests/unit/tools/external_api/test_credentials.pytests/unit/tools/external_api/test_external_api_tool.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/tools/external_api/test_signature.pytests/unit/core/test_approval.pytests/unit/meta/test_approval_repo.pytests/unit/observability/test_events.pytests/conformance/persistence/test_connection_repositories.pytests/unit/api/test_approval_store.pytests/unit/tools/external_api/test_errors.pytests/unit/tools/external_api/test_httpx_provider.pytests/unit/hr/test_registry_autonomy.pytests/unit/security/test_action_types.pytests/conformance/persistence/test_approval_repository.pytests/unit/approval/test_protocol.pytests/unit/core/test_enums.pytests/e2e/test_external_api_governance_e2e.pytests/unit/tools/external_api/test_credentials.pytests/unit/tools/external_api/test_external_api_tool.py
web/src/mocks/handlers/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/mocks/handlers/**/*.ts: MSW handlers (MANDATORY):web/src/mocks/handlers/must mirrorweb/src/api/endpoints/*.ts1:1 with a default happy-path handler for every exported endpoint. UseonUnhandledRequest: 'error'in test setup; tests override per-case viaserver.use(...), nevervi.mock('@/api/endpoints/*').
Use typed envelope helpers (successFor,paginatedFor,voidSuccess) to keep MSW handlers in lockstep with endpoint return types
Files:
web/src/mocks/handlers/approvals.tsweb/src/mocks/handlers/connections.ts
src/**/observability/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Telemetry: opt-in, off by default. Every event property must be in
_ALLOWED_PROPERTIES. See telemetry.md.
Files:
src/synthorg/observability/events/external_api.pysrc/synthorg/observability/prometheus_labels.py
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/persistence/**/*.py: New repository protocols inherit from one or more generic categories inpersistence/_generics.py(SingletonRepository,IdKeyedRepository,FilteredQueryRepository,AppendOnlyRepository,StatefulRepository,MVCCRepository); bespoke methods only under ADR-0001 D7.
Repository CRUD:save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples.
Datetime in persistence:parse_iso_utc/format_iso_utcfrompersistence._shared(reject naive);normalize_utcfor already-typed.
Files:
src/synthorg/persistence/approval_protocol.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/persistence/postgres/connection_repo.py
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres). Enforced bycheck_dual_backend_test_parity.py.
Files:
tests/conformance/persistence/test_connection_repositories.pytests/conformance/persistence/test_approval_repository.py
web/src/api/types/**/*.gen.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Generated DTO types (MANDATORY): NEVER hand-edit
web/src/api/types/*.gen.ts. Regenerate withuv run python scripts/generate_dto_types_ts.py. Import DTOs via the barrel (import type { AgentConfig } from '@/api/types').
Files:
web/src/api/types/enum-values.gen.tsweb/src/api/types/openapi.gen.ts
web/src/utils/constants.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
WS wire protocol (MANDATORY): the client-server contract lives in
web/src/utils/constants.ts(WS_PROTOCOL_VERSION,WS_MAX_MESSAGE_SIZE,WS_HEARTBEAT_INTERVAL_MS,WS_PONG_TIMEOUT_MS,LOG_SANITIZE_MAX_LENGTH) and MUST stay in lockstep withsrc/synthorg/api/ws_models.py/src/synthorg/api/controllers/ws.py. Bump the protocol version on both sides together for breaking payload changes.
Files:
web/src/utils/constants.ts
web/src/{components,utils}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
NEVER write
getXIcon(value): LucideIconfactories called inside JSX bodies. Export a<XIcon value={...} />wrapper that does the lookup viacreateElementinside the wrapper body. Wrapper components live in their own file, not alongside utility exports.
Files:
web/src/utils/constants.ts
web/src/{stores,**/*.test.{ts,tsx}}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Active-handle gate (MANDATORY): every unit test runs under
web/test-infra/active-handle-tracker.ts, which fails any test that leaks an event-loop-holding resource. A new store that schedules timers / attaches listeners MUST expose a teardown hook and register it in the globalafterEach; otherwise the gate fails the first test that triggers the schedule.
Files:
web/src/__tests__/stores/connections.test.ts
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Conversational propose:
POST /meta/chat/proposeis opt-in (meta.chief_of_staff.propose_enabled);ChiefOfStaffProposerbuilt bybuild_chief_of_staff_proposer(ENFORCED manifest entry) and 503s when ANY of provider, persistence, or work pipeline missing. Human content wrapped viawrap_untrusted(TAG_TASK_DATA, ...)(SEC-1).
Files:
src/synthorg/api/controllers/connections.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/api/approval_store.py
src/synthorg/api/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Construction phase wires synchronous services; on_startup wires services needing connected persistence. Agent registry built BEFORE auto_wire_meetings; tunnel_provider wired unconditionally.
Files:
src/synthorg/api/_approval_expiration.pysrc/synthorg/api/approval_store.py
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: List reads (fetch*) must seterror: string | nullon the store instead of toasting
Test teardown (MANDATORY): any new store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEach. The globalafterEachinweb/src/test-setup.tsxalready callsuseToastStore.getState().dismissAll(),cancelPendingPersist(), anduseThemeStore.getState().teardown().
Files:
web/src/stores/approvals.ts
web/src/{api/endpoints,stores}/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Cursor pagination (MANDATORY): list endpoints must use opaque cursor-based paging via
PaginationMeta. Stores must keepnextCursor+hasMorein state (not offset arithmetic) and early-return when!hasMore || !nextCursor. Display counts must come fromdata.length.
Files:
web/src/stores/approvals.ts
🧠 Learnings (7)
📚 Learning: 2026-05-16T18:36:31.446Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/reference/conventions.md:787-789
Timestamp: 2026-05-16T18:36:31.446Z
Learning: In Aureliolo/synthorg, do not require adding `<!--RS:...-->` “Doc Numeric Claims (MANDATORY)” numeric macros for Python version numbers mentioned in documentation prose (e.g., “Python 3.14”, “Python 3.15”). The `scripts/check_doc_numeric_macros.py` gate only applies to `README.md`, `docs/index.md`, `docs/roadmap/index.md`, `docs/architecture/decisions.md`, and `docs/reference/convention-gates.md`, and it only flags digits adjacent to specific stat nouns (tests/providers/agents/stars/releases), not language version mentions like “Python 3.14”.
Applied to files:
docs/design/integrations.mddocs/design/tools.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo, account for the CI gate `check_doc_numeric_macros.py`: it skips fenced code blocks entirely, and it only flags digits that are adjacent to these stat nouns: `tests`, `providers`, `agents`, `stars`, `releases`. Therefore, numeric examples such as CLI flag values (e.g., `--num-workers=4` in fenced bash blocks) and prose version numbers (e.g., `3.14`/`3.15`) are not expected to trigger this check; prioritize changes only when digits appear next to one of the listed nouns (e.g., “5 tests”, “10 stars”, etc.).
Applied to files:
docs/design/integrations.mddocs/design/tools.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing markdown files for the "Doc Numeric Claims (MANDATORY)" RS-marker rule, only require/flag missing RS markers in the files that are actually in-scope for the rule. The scope is enforced via an identical _SCOPED_FILES allowlist in scripts/check_doc_numeric_macros.py and scripts/inject_runtime_stats.py, and currently includes: README.md; docs/index.md; docs/roadmap/index.md; docs/architecture/decisions.md; docs/reference/convention-gates.md. For any other markdown files (e.g., docs/getting_started.md, docs/guides/*), missing RS markers for numeric claims are no-ops and should NOT be flagged.
Applied to files:
docs/design/integrations.mddocs/design/tools.md
📚 Learning: 2026-05-16T18:36:35.250Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1944
File: docs/getting_started.md:109-109
Timestamp: 2026-05-16T18:36:35.250Z
Learning: When reviewing Markdown in the synthorg repo against the `check_doc_numeric_macros.py` gate, account for its documented behavior: it skips fenced code blocks entirely, and it only flags digits that are adjacent to specific stat nouns (`tests`, `providers`, `agents`, `stars`, `releases`). As a result, CLI-style numbers (e.g., `--num-workers=4`) inside fenced bash code blocks should never be treated as violations of this gate; only non-fenced text needs checking, and only around those specific nouns.
Applied to files:
docs/design/integrations.mddocs/design/tools.md
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
src/synthorg/security/rules/risk_classifier.pytests/unit/tools/external_api/test_signature.pysrc/synthorg/observability/events/external_api.pytests/unit/core/test_approval.pysrc/synthorg/security/timeout/risk_tier_classifier.pysrc/synthorg/security/risk_scorer.pysrc/synthorg/settings/enums.pysrc/synthorg/tools/external_api/_runtime.pysrc/synthorg/tools/permissions.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/tools/external_api/__init__.pytests/unit/meta/test_approval_repo.pysrc/synthorg/approval/protocol.pytests/unit/observability/test_events.pysrc/synthorg/persistence/approval_protocol.pytests/conformance/persistence/test_connection_repositories.pytests/unit/api/test_approval_store.pytests/unit/tools/external_api/test_errors.pytests/unit/tools/external_api/test_httpx_provider.pysrc/synthorg/engine/agent_engine.pytests/unit/hr/test_registry_autonomy.pytests/unit/security/test_action_types.pysrc/synthorg/security/action_type_mapping.pysrc/synthorg/core/enums.pytests/conformance/persistence/test_approval_repository.pytests/unit/approval/test_protocol.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/tools/external_api/errors.pysrc/synthorg/core/approval.pysrc/synthorg/settings/definitions/external_api.pysrc/synthorg/persistence/sqlite/connection_repo.pysrc/synthorg/tools/external_api/_credentials.pytests/unit/core/test_enums.pysrc/synthorg/tools/external_api/_args.pysrc/synthorg/tools/external_api/provider_factory.pysrc/synthorg/security/action_types.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/tools/external_api/_signature.pytests/e2e/test_external_api_governance_e2e.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/tools/_dns_pinning.pysrc/synthorg/integrations/health/checks/generic_http.pysrc/synthorg/tools/external_api/provider.pysrc/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/integrations/connections/_oauth_rotation.pysrc/synthorg/engine/_security_factory.pysrc/synthorg/integrations/connections/models.pytests/unit/tools/external_api/test_credentials.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/tools/external_api/external_api_tool.pysrc/synthorg/workers/runtime_builder.pysrc/synthorg/engine/agent_engine_factories.pytests/unit/tools/external_api/test_external_api_tool.pysrc/synthorg/integrations/connections/catalog.pysrc/synthorg/api/approval_store.pysrc/synthorg/persistence/postgres/connection_repo.py
📚 Learning: 2026-05-17T11:45:11.839Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1952
File: src/synthorg/settings/definitions/api.py:594-638
Timestamp: 2026-05-17T11:45:11.839Z
Learning: In SynthOrg (Aureliolo/synthorg) pre-alpha, apply the strict no-backward-compat policy: any setting-key rename must be fully completed in the same change/PR with all repo callers updated, and you should not keep legacy aliases or compatibility fallbacks. When reviewing, do not flag a setting-key rename as a breaking upgrade hazard if the rename is repo-wide and fully implemented within the same PR.
Applied to files:
src/synthorg/settings/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/definitions/external_api.py
📚 Learning: 2026-05-17T11:45:11.839Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1952
File: src/synthorg/settings/definitions/api.py:594-638
Timestamp: 2026-05-17T11:45:11.839Z
Learning: In this repository, SynthOrg is pre-alpha and uses a strict no-backward-compat policy for setting-key renames. When reviewing code under src/synthorg/settings, do NOT flag a setting-key rename as an “upgrade-safety” issue if the rename is complete/atomic in the same PR: all callers/usages of the old key are updated simultaneously, and the PR does not keep any legacy aliases, compatibility fallbacks, or migration/rollback paths for the old key.
Applied to files:
src/synthorg/settings/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/definitions/external_api.py
🔇 Additional comments (71)
docs/design/integrations.md (1)
27-30: LGTM!src/synthorg/security/rules/risk_classifier.py (1)
24-24: LGTM!web/src/pages/approvals/ApprovalCard.stories.tsx (1)
22-22: LGTM!web/src/pages/approvals/ApprovalDetailDrawer.stories.tsx (1)
22-22: LGTM!src/synthorg/persistence/sqlite/revisions/20260521000001_external_api_governed_access.sql (1)
17-21: LGTM!web/src/pages/oauth-apps/OauthAppCard.stories.tsx (1)
20-20: LGTM!web/src/mocks/handlers/approvals.ts (1)
32-32: LGTM!src/synthorg/observability/events/external_api.py (1)
1-13: LGTM!web/src/pages/connections/ConnectionCard.stories.tsx (1)
19-19: LGTM!scripts/schema_drift_baseline.txt (1)
198-198: LGTM!tests/unit/core/test_approval.py (1)
139-169: LGTM!src/synthorg/persistence/postgres/revisions/20260521000001_external_api_governed_access.sql (1)
17-19: LGTM!src/synthorg/security/timeout/risk_tier_classifier.py (1)
45-45: LGTM!src/synthorg/security/risk_scorer.py (1)
202-204: LGTM!web/src/pages/approvals/ApprovalTimeline.stories.tsx (1)
21-21: LGTM!src/synthorg/settings/enums.py (1)
36-36: LGTM!src/synthorg/tools/external_api/_runtime.py (1)
19-28: LGTM!web/src/pages/mcp-catalog/McpInstallWizard.stories.tsx (1)
49-49: LGTM!src/synthorg/tools/permissions.py (1)
97-97: LGTM!web/src/__tests__/helpers/factories.ts (1)
248-248: LGTM!src/synthorg/settings/definitions/__init__.py (1)
17-17: LGTM!Also applies to: 44-44
src/synthorg/tools/external_api/__init__.py (1)
1-9: LGTM!tests/unit/meta/test_approval_repo.py (1)
34-35: LGTM!web/src/pages/settings/utils.ts (1)
42-42: LGTM!web/src/pages/connections/ConnectionFormModal.stories.tsx (1)
74-74: LGTM!src/synthorg/approval/protocol.py (1)
78-91: LGTM!tests/unit/observability/test_events.py (1)
343-344: LGTM!src/synthorg/persistence/approval_protocol.py (1)
29-29: LGTM!Also applies to: 63-64, 133-158
web/src/api/types/enum-values.gen.ts (1)
647-647: LGTM!Also applies to: 770-770
tests/unit/api/test_approval_store.py (1)
242-291: LGTM!tests/unit/tools/external_api/test_errors.py (1)
1-94: LGTM!tests/unit/tools/external_api/test_httpx_provider.py (1)
1-158: LGTM!src/synthorg/engine/agent_engine.py (1)
113-113: LGTM!Also applies to: 202-202, 222-222
web/src/utils/constants.ts (1)
154-154: LGTM!Also applies to: 185-185
tests/unit/hr/test_registry_autonomy.py (1)
86-87: LGTM!tests/unit/security/test_action_types.py (1)
14-14: LGTM!Also applies to: 31-31
src/synthorg/security/action_type_mapping.py (1)
31-31: LGTM!docs/design/tools.md (1)
12-12: LGTM!Also applies to: 30-30, 454-454, 463-463, 478-478
src/synthorg/core/enums.py (1)
477-477: LGTM!Also applies to: 585-585
tests/conformance/persistence/test_approval_repository.py (1)
545-621: LGTM!tests/unit/approval/test_protocol.py (1)
66-66: LGTM!src/synthorg/observability/prometheus_labels.py (1)
291-291: LGTM!web/src/mocks/handlers/connections.ts (1)
31-31: LGTM!src/synthorg/tools/external_api/errors.py (1)
1-87: LGTM!src/synthorg/core/approval.py (1)
52-57: LGTM!Also applies to: 77-77, 127-142
web/src/__tests__/stores/connections.test.ts (1)
51-51: LGTM!Also applies to: 123-123, 133-133
web/src/pages/connections/ConnectionFormModal.tsx (1)
20-20: LGTM!Also applies to: 53-57, 66-66, 84-84, 295-295, 305-305, 428-435
src/synthorg/settings/definitions/external_api.py (1)
1-97: LGTM!src/synthorg/persistence/sqlite/connection_repo.py (1)
54-54: LGTM!Also applies to: 73-73, 106-106, 157-157, 159-159, 173-173, 189-189
src/synthorg/tools/external_api/_credentials.py (1)
1-63: LGTM!tests/unit/core/test_enums.py (1)
120-121: LGTM!Also applies to: 124-124
src/synthorg/tools/external_api/_args.py (1)
1-98: LGTM!src/synthorg/tools/external_api/provider_factory.py (1)
1-38: LGTM!src/synthorg/security/action_types.py (1)
37-37: LGTM!src/synthorg/persistence/sqlite/schema.sql (1)
970-970: LGTM!Also applies to: 1145-1147
src/synthorg/tools/external_api/_signature.py (1)
17-80: LGTM!src/synthorg/persistence/postgres/schema.sql (1)
975-975: LGTM!Also applies to: 1213-1213
tests/e2e/test_external_api_governance_e2e.py (1)
43-234: LGTM!src/synthorg/tools/_dns_pinning.py (1)
29-145: LGTM!src/synthorg/integrations/health/checks/generic_http.py (1)
4-4: LGTM!Also applies to: 19-19, 103-106
src/synthorg/tools/external_api/provider.py (1)
1-85: LGTM!src/synthorg/tools/external_api/httpx_provider.py (1)
1-96: LGTM!src/synthorg/integrations/connections/_oauth_rotation.py (1)
1-220: LGTM!src/synthorg/engine/_security_factory.py (1)
44-45: LGTM!Also applies to: 285-325
src/synthorg/integrations/connections/models.py (1)
104-106: LGTM!Also applies to: 125-125
tests/unit/tools/external_api/test_credentials.py (1)
1-69: LGTM!src/synthorg/persistence/sqlite/approval_repo.py (1)
5-5: LGTM!Also applies to: 145-149, 236-237, 303-304, 410-410, 447-447, 499-499, 568-568, 683-735
web/src/api/types/openapi.gen.ts (1)
5963-5967: LGTM!Also applies to: 6941-6942, 7249-7250, 11470-11470, 12234-12234, 12487-12487
src/synthorg/integrations/connections/catalog.py (1)
14-14: LGTM!Also applies to: 69-69, 193-193, 217-217, 319-319, 372-372, 466-466, 500-501, 512-512, 539-539
src/synthorg/api/approval_store.py (1)
40-40: LGTM!Also applies to: 70-70, 668-716
src/synthorg/persistence/postgres/connection_repo.py (1)
55-56: LGTM!Also applies to: 76-76, 91-91, 136-136, 149-149, 152-152, 167-167
| def test_credential_headers_excluded(self) -> None: | ||
| # Auth headers are injected later, not part of the signed shape; the | ||
| # agent-supplied headers are what's signed. Different agent headers | ||
| # change the signature, but that is the caller's request shape. | ||
| assert _sig(headers={"Accept": "application/json"}) != _sig( | ||
| headers={"Accept": "text/plain"}, | ||
| ) |
There was a problem hiding this comment.
Test intent doesn’t match assertion for credential-header exclusion.
Line 63-Line 69 claims credential/auth headers are excluded, but the assertion only changes Accept, which should be signed. This leaves the credential-header exclusion contract untested on a security-sensitive path.
Suggested test adjustment
def test_credential_headers_excluded(self) -> None:
- # Auth headers are injected later, not part of the signed shape; the
- # agent-supplied headers are what's signed. Different agent headers
- # change the signature, but that is the caller's request shape.
- assert _sig(headers={"Accept": "application/json"}) != _sig(
- headers={"Accept": "text/plain"},
- )
+ # Credential headers should not affect signature matching.
+ assert _sig(
+ headers={"Accept": "application/json", "Authorization": "Bearer token-a"},
+ ) == _sig(
+ headers={"Accept": "application/json", "Authorization": "Bearer token-b"},
+ )
+
+ # Non-credential headers remain part of the signed request shape.
+ assert _sig(headers={"Accept": "application/json"}) != _sig(
+ headers={"Accept": "text/plain"},
+ )📝 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.
| def test_credential_headers_excluded(self) -> None: | |
| # Auth headers are injected later, not part of the signed shape; the | |
| # agent-supplied headers are what's signed. Different agent headers | |
| # change the signature, but that is the caller's request shape. | |
| assert _sig(headers={"Accept": "application/json"}) != _sig( | |
| headers={"Accept": "text/plain"}, | |
| ) | |
| def test_credential_headers_excluded(self) -> None: | |
| # Credential headers should not affect signature matching. | |
| assert _sig( | |
| headers={"Accept": "application/json", "Authorization": "Bearer token-a"}, | |
| ) == _sig( | |
| headers={"Accept": "application/json", "Authorization": "Bearer token-b"}, | |
| ) | |
| # Non-credential headers remain part of the signed request shape. | |
| assert _sig(headers={"Accept": "application/json"}) != _sig( | |
| headers={"Accept": "text/plain"}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/tools/external_api/test_signature.py` around lines 63 - 69, The
test test_credential_headers_excluded currently only varies the "Accept" header
(which should be part of the signature) and thus never exercises
credential-header exclusion; update the test so it asserts that changing
credential headers (e.g., "Authorization" or other auth-related headers) does
NOT change the signature while still asserting that changing normal headers like
"Accept" does change it. Concretely, add an assertion using
_sig(headers={"Authorization": "tokenA"}) == _sig(headers={"Authorization":
"tokenB"}) to verify exclusion, and keep the existing inequality check for _sig
with different "Accept" values.
… one-shot/cross-principal hardening, external_api egress hardening, sensitive-flag boundary + tests
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/persistence/postgres/approval_repo.py (1)
635-678:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
consumed_atbefore writing it.
consume_if_approved()sends the caller-provideddatetimestraight toTIMESTAMPTZ, unlike the SQLite path which goes through UTC normalization. A naive timestamp here can be stored using the session timezone and make Postgres stamp a different instant than SQLite for the same approval consumption.As per coding guidelines, `src/synthorg/persistence/**/*.py`: Datetime in persistence: `parse_iso_utc` / `format_iso_utc` from `persistence._shared` (reject naive); `normalize_utc` for already-typed.Suggested patch
-from synthorg.persistence._shared import coerce_row_timestamp, validate_pagination_args +from synthorg.persistence._shared import ( + coerce_row_timestamp, + normalize_utc, + validate_pagination_args, +) @@ - params = (consumed_at, approval_id, ApprovalStatus.APPROVED.value) + params = ( + normalize_utc(consumed_at), + approval_id, + ApprovalStatus.APPROVED.value, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/synthorg/persistence/postgres/approval_repo.py` around lines 635 - 678, The DB write uses the caller-provided consumed_at directly, which can be naive or in a non-UTC zone and lead to inconsistent TIMESTAMPTZ values; call normalize_utc(consumed_at) (or parse_iso_utc if the input can be a string) before building params so the value is validated and converted to an aware UTC datetime; update consume_if_approved to import normalize_utc from persistence._shared, replace params = (consumed_at, ...) with the normalized value, and keep the same SQL/logic around ApprovalStatus.APPROVED.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/synthorg/tools/external_api/external_api_tool.py`:
- Around line 80-85: ApprovalSignature.build currently hashes the raw
args.headers which allows unsendable framing headers (defined by
_RESTRICTED_REQUEST_HEADERS) to affect approval signatures; fix by
normalizing/sanitizing headers with the same logic used in _merge_agent_headers
(i.e., lowercasing header names and removing any in _RESTRICTED_REQUEST_HEADERS
and hop-by-hop names) before computing the signature so the hashed headers match
what will actually be sent; update ApprovalSignature.build and any other
signature-creation call sites that currently pass raw headers to use this
sanitized header set (refer to ApprovalSignature.build, _merge_agent_headers,
and _RESTRICTED_REQUEST_HEADERS).
In `@tests/conformance/persistence/test_connection_repositories.py`:
- Around line 94-101: The test currently only checks that refetched.sensitive
remains True after an upsert but doesn't verify the metadata was actually
updated; modify the assertion after calling backend.connections.save and
fetching via backend.connections.get(NotBlankStr("conn-secret")) to assert that
refetched.metadata (or refetched.metadata["owner"]) equals "secops" in addition
to asserting refetched.sensitive is True so the test proves both that the row
was updated and the sensitive flag was preserved.
In `@tests/unit/tools/external_api/test_external_api_tool.py`:
- Around line 406-436: The test must assert the intruder created a new, distinct
approval record instead of reusing the owner's approval_id: after parking the
owner's request (variable parked) and after calling intruder.execute (variable
result), fetch the intruder's parked approval id from
result.metadata["approval_id"] and assert it is not equal to
parked.metadata["approval_id"] (or the saved item.id); update
test_other_principal_cannot_consume_grant to include this inequality check so
the test verifies the intruder gets a fresh principal-scoped approval id.
---
Outside diff comments:
In `@src/synthorg/persistence/postgres/approval_repo.py`:
- Around line 635-678: The DB write uses the caller-provided consumed_at
directly, which can be naive or in a non-UTC zone and lead to inconsistent
TIMESTAMPTZ values; call normalize_utc(consumed_at) (or parse_iso_utc if the
input can be a string) before building params so the value is validated and
converted to an aware UTC datetime; update consume_if_approved to import
normalize_utc from persistence._shared, replace params = (consumed_at, ...) with
the normalized value, and keep the same SQL/logic around
ApprovalStatus.APPROVED.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79bb2636-2154-48d5-a373-380d16191831
📒 Files selected for processing (9)
src/synthorg/api/_approval_expiration.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/tools/external_api/external_api_tool.pysrc/synthorg/tools/external_api/httpx_provider.pytests/conformance/persistence/test_connection_repositories.pytests/unit/tools/external_api/test_external_api_tool.pytests/unit/tools/external_api/test_signature.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). (14)
- GitHub Check: Deploy Preview
- GitHub Check: Build Backend
- GitHub Check: Lighthouse Site
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Test Unit
- GitHub Check: Dashboard Test
- GitHub Check: Test Conformance (SQLite)
- GitHub Check: Test E2E
- GitHub Check: Test Integration
- GitHub Check: CodSpeed Python benchmarks
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Test markers:@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% min.
Windows: unit tests useWindowsSelectorEventLoopPolicy(3.14 IOCP teardown race). Subprocess tests override back.
Test doubles: ladder in conventions.md section 12.1.FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags. BareMagicMockat typed boundary (constructor / fn arg / annotated local / typed fixture return) blocked byscripts/check_mock_spec.py(zero-tolerance).
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)). Flaky: NEVER skip/xfail; fix fundamentally. Useasyncio.Event().wait()notsleep(large).
Files:
tests/conformance/persistence/test_connection_repositories.pytests/unit/tools/external_api/test_signature.pytests/unit/tools/external_api/test_external_api_tool.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/conformance/persistence/test_connection_repositories.pytests/unit/tools/external_api/test_signature.pytests/unit/tools/external_api/test_external_api_tool.py
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres). Enforced bycheck_dual_backend_test_parity.py.
Files:
tests/conformance/persistence/test_connection_repositories.py
src/synthorg/!(persistence)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL.
Files:
src/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/tools/external_api/external_api_tool.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use DB > env > code default viaSettingsService/ConfigResolver(Cat-1) or env > code default (Cat-2,read_only_post_init); Cat-3 bootstrap secrets are pure env at the boot site. YAML is a company-template ingestion format, not a precedence tier. Noos.environ.getoutside startup; pre-init Cat-2 reads usesettings.bootstrap_resolver.resolve_init_value.
No hardcoded numeric values; numerics live insettings/definitions/. Allowlist: 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants of the formNAME: int|float|Final|Final[int]|Final[float] = literal. Enforced byscripts/check_no_magic_numbers.py.
Comments WHY only; no reviewer citations / issue back-refs / migration framing. Enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py.
Nofrom __future__ import annotations(3.14 has PEP 649). PEP 758 except:except A, B:no parens unless binding.
Type hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines.
Errors:<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeError/etc directly. Enforced bycheck_domain_error_hierarchy.py.
Pydantic v2 frozen +extra="forbid"on every frozen model project-wide (gatecheck_frozen_model_extra_forbid.py;@computed_fieldauto-exempt, per-line# lint-allow: frozen-extra-forbid -- <reason>forextra="allow"/"ignore"boundaries);@computed_fieldfor derived;NotBlankStrfor identifiers.
Args models at every system boundary;parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.py.
Immutability:model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries.
Async:asyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError).
Clock seam:clock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out st...
Files:
src/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/tools/external_api/external_api_tool.py
⚙️ CodeRabbit configuration file
This project uses Python 3.14+ with PEP 758 except syntax: "except A, B:" (comma-separated, no parentheses) is correct and mandatory -- do NOT flag it as a typo or suggest parenthesized form. The "except builtins.MemoryError, RecursionError: raise" pattern is intentional project convention for system-error propagation. When evaluating the 50-line function limit, count only the function body excluding the signature lines, decorators, and docstring. Functions 1-5 lines over due to docstrings or multi-line signatures should not be flagged. Do not suggest extracting single-use helper functions called exactly once -- this reduces readability without improving maintainability.
Files:
src/synthorg/tools/external_api/httpx_provider.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/persistence/postgres/approval_repo.pysrc/synthorg/tools/external_api/external_api_tool.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Conversational propose:
POST /meta/chat/proposeis opt-in (meta.chief_of_staff.propose_enabled);ChiefOfStaffProposerbuilt bybuild_chief_of_staff_proposer(ENFORCED manifest entry) and 503s when ANY of provider, persistence, or work pipeline missing. Human content wrapped viawrap_untrusted(TAG_TASK_DATA, ...)(SEC-1).
Files:
src/synthorg/api/controllers/connections.pysrc/synthorg/api/_approval_expiration.py
src/synthorg/api/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Construction phase wires synchronous services; on_startup wires services needing connected persistence. Agent registry built BEFORE auto_wire_meetings; tunnel_provider wired unconditionally.
Files:
src/synthorg/api/_approval_expiration.py
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/persistence/**/*.py: New repository protocols inherit from one or more generic categories inpersistence/_generics.py(SingletonRepository,IdKeyedRepository,FilteredQueryRepository,AppendOnlyRepository,StatefulRepository,MVCCRepository); bespoke methods only under ADR-0001 D7.
Repository CRUD:save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples.
Datetime in persistence:parse_iso_utc/format_iso_utcfrompersistence._shared(reject naive);normalize_utcfor already-typed.
Files:
src/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/persistence/postgres/approval_repo.py
🧠 Learnings (1)
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
tests/conformance/persistence/test_connection_repositories.pysrc/synthorg/tools/external_api/httpx_provider.pytests/unit/tools/external_api/test_signature.pysrc/synthorg/api/controllers/connections.pysrc/synthorg/api/_approval_expiration.pysrc/synthorg/persistence/sqlite/approval_repo.pysrc/synthorg/persistence/postgres/approval_repo.pytests/unit/tools/external_api/test_external_api_tool.pysrc/synthorg/tools/external_api/external_api_tool.py
🔇 Additional comments (5)
tests/unit/tools/external_api/test_signature.py (1)
63-78: LGTM!src/synthorg/api/controllers/connections.py (1)
145-161: LGTM!Also applies to: 355-359
src/synthorg/api/_approval_expiration.py (1)
123-200: LGTM!src/synthorg/tools/external_api/external_api_tool.py (2)
13-13: LGTM!Also applies to: 210-215, 258-267
404-449: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2032 +/- ##
==========================================
+ Coverage 84.95% 85.00% +0.05%
==========================================
Files 2016 2030 +14
Lines 119582 120135 +553
Branches 10084 10133 +49
==========================================
+ Hits 101587 102118 +531
- Misses 15488 15491 +3
- Partials 2507 2526 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…t UTC-normalize (sqlite parity), exclude restricted headers from approval signature, strengthen upsert + cross-principal tests
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - Introduced conversational interface for direct clarify and propose interactions. - Cost management now includes forecast gates, hard ceilings, and Pareto considerations. - Added living documentation engine combining wiki and retrieval-augmented generation features. - Real intake engine is now operational for live data processing. - Virtual desktop tool with vision verification gate available for enhanced workspace control. ### What's new - Per-project reproducible environments for consistent setups. - Headless browser testing tool integrated for automated UI validation. - Governed external API and data access tool introduced. - Hardened external-remote git backend with sandbox mounts and push-queue dispatching. - Adversarial red-team gate subsystem for enhanced security testing. - Self-extending toolkit to dynamically expand capabilities. - Stakes-aware model routing enables prioritized processing. - Task-board entry adapter connects live runtime with project management. - Persistent project workspace with pluggable git backend and per-project push queues implemented. - Knowledge and provenance substrate added to track data lineage. - Scoring and data contract framework for golden-company benchmark evaluations. ### Under the hood - Desktop Dockerfile pinned by digest to improve build stability and documented publishing gap fixed. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.7](v0.8.6...v0.8.7) (2026-05-22) ### Features * conversational interface v1 - 1:1 clarify + propose ([#2019](#2019)) ([216ef94](216ef94)), closes [#1968](#1968) * cost as a first-class dial (forecast gate, hard ceiling, Pareto) ([#2029](#2029)) ([700a59e](700a59e)), closes [#1982](#1982) * **env:** reproducible per-project environments ([#2039](#2039)) ([d2c0ef9](d2c0ef9)), closes [#1994](#1994) * **evals:** [#1980](#1980) spine -- scoring + data contract for golden-company benchmark ([#2025](#2025)) ([53108e8](53108e8)) * goal/objective entry adapter ([#1964](#1964)) ([#2022](#2022)) ([cb15c3c](cb15c3c)) * governed external API/data access tool ([#1991](#1991)) ([#2032](#2032)) ([e08b451](e08b451)) * harden external-remote git backend + per-project sandbox mount + push-queue dispatch ([#2020](#2020)) ([#2030](#2030)) ([2fa2e1e](2fa2e1e)) * headless browser testing tool ([#1992](#1992)) ([#2024](#2024)) ([277b52a](277b52a)) * knowledge + provenance substrate ([#2036](#2036)) ([48c897b](48c897b)) * living documentation engine (dual-purpose wiki + RAG namespace) ([#2028](#2028)) ([3d10da9](3d10da9)), closes [#1976](#1976) * real intake engine online ([#2017](#2017)) ([9d8eb34](9d8eb34)) * **redteam:** adversarial red-team gate subsystem ([#1986](#1986)) ([#2026](#2026)) ([d2207e9](d2207e9)) * self-extending toolkit ([#1995](#1995)) ([#2035](#2035)) ([5ffc545](5ffc545)) * stakes-aware model routing ([#1998](#1998)) ([#2038](#2038)) ([9b98312](9b98312)) * task-board entry adapter to live runtime ([#1963](#1963)) ([#2023](#2023)) ([a8f1eea](a8f1eea)) * virtual desktop tool and vision verifier gate ([#2031](#2031)) ([dfe8b42](dfe8b42)), closes [#1993](#1993) * **workspace:** persistent project workspace + pluggable git backend + per-project push queue ([#2021](#2021)) ([ee58ee7](ee58ee7)) ### Bug Fixes * pin desktop Dockerfile by digest (Scorecard [#309](#309)) + document publish gap ([#2034](#2034)) ([8fda188](8fda188)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: synthorg-repo-bot[bot] <279117679+synthorg-repo-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary
Adds
external_api, a first-class governed tool path for external API/data access, built as a thin wrapper over existing infrastructure (NOT a new network stack), per EPIC #1987.NetworkPolicy+ DNS-pinning httpx transport (sharedtools/_dns_pinning.py, extracted from the generic-HTTP health check) +..path-traversal rejection +follow_redirects=False.@with_connection_rate_limitdecorator; exhaustion returns a graceful result, never a retry.ApprovalSignature), and on resume consumes the grant exactly once via an atomicconsume_if_approvedcompare-and-set (new nullableapprovals.consumed_atcolumn). A replay or a divergent re-issue cannot reuse a spent grant.ToolCategory.EXTERNAL_DATA+ActionType.EXTERNAL_DATA_REQUEST, preset-unlisted so the tool is the single gate under SEMI/SUPERVISED; FULL auto-approves, LOCKED routes through SecOps.ExternalAccessProviderprotocol + factory +external_api.provider_typediscriminator (httpx default).registry_with_external_api_toolin_make_tool_invoker; boot-scopedExternalApiRuntimeonAgentEngine(fail-open).Connection.sensitiveflag exposed through the connections REST API + the web dashboard form.Test plan
build_auth_headersfor every auth method, the new model invariants (consumed_at=> APPROVED; pinned IP/hostname pairing),consume_if_approvedCAS intests/unit/api/test_approval_store.py.consume_if_approvedonce-only +sensitive/consumed_atround-trip (SQLite locally; Postgres in CI).tests/e2e/test_external_api_governance_e2e.py): full park -> approve -> resume -> consume -> replay-rejected lifecycle, egress confinement, graceful rate limiting (run locally, green).Review coverage
Pre-reviewed by 18 agents via
/pre-pr-review. All valid findings implemented:consumed_at/pinned-pair model validators, web WS shape-guardconsumed_atcheck,_classify_riskfailure logging, design-doc updates (docs/design/tools.md+integrations.md), egress-block logging, status-code constraint, and added credential/signature/HEAD unit tests. Both pre-existing >800-line files (catalog.py,approval_store.py) split under the limit via verbatim mixins. One finding dismissed as a false positive (except A, B:is valid PEP 758 for Python 3.14); one skipped with confirmation (broad-except refactor of unrelated pre-existing approval_store methods that already re-raise correctly).Closes #1991