refactor: code-structure cleanup (sub-tasks D + F + G + H + I)#1859
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned Files
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.py⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-05-05T09:04:46.195ZApplied to files:
🔇 Additional comments (10)
WalkthroughThis PR centralizes case-insensitive comparisons via compare_ci and replaces manual lowercasing across many modules; unifies escalation processors into HumanDecisionProcessor(mode) with updated factory, resolver, docs, and tests; hardens SettingsChangeDispatcher lifecycle and runtime resolver integration with locked async resets tracked in _post_done_tasks; migrates the web frontend from generated OpenAPI types to hand-maintained runtime constants and explicit interfaces while removing generation tooling; and adds a protocol-audit script and reference plus baseline and test updates. |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized case-insensitive string comparison utility (compare_ci) and refactors various parts of the codebase to use it, replacing manual .lower() calls. It also replaces the WinnerSelectProcessor and HybridDecisionProcessor with a single HumanDecisionProcessor that uses a mode discriminator, and removes generated OpenAPI TypeScript types in favor of hand-maintained runtime value arrays and derived types. I have identified two issues: a potential NameError due to missing from __future__ import annotations when using NotBlankStr in type hints, and an inaccurate count of ASGI-layer call sites in the compare_ci docstring.
| @@ -1,6 +1,7 @@ | |||
| """Decision processor strategies for the escalation queue.""" | |||
| """Decision processor strategy for the escalation queue.""" | |||
There was a problem hiding this comment.
The type hint NotBlankStr (used at lines 108, 165, and 213) is imported only within a TYPE_CHECKING block. Without from __future__ import annotations, this will cause a NameError at runtime when the class is defined because Python evaluates type annotations at definition time. Adding the future import is the recommended fix to enable postponed evaluation of annotations.
| """Decision processor strategy for the escalation queue.""" | |
| from __future__ import annotations | |
| """Decision processor strategy for the escalation queue.""" |
| The helper is ``str``-only by design. Three ASGI-layer call | ||
| sites that compare bytes-typed header names | ||
| (``synthorg.api.auth.csrf`` and ``synthorg.api.etag``) keep | ||
| the inline ``name.lower() == b"..."`` pattern: byte equality is |
There was a problem hiding this comment.
The docstring mentions "Three ASGI-layer call sites" that are exempt from using compare_ci, but the PR description and the actual changes in csrf.py and etag.py show that there are four such sites (two in each file). This should be updated to maintain accuracy.
| The helper is ``str``-only by design. Three ASGI-layer call | |
| sites that compare bytes-typed header names | |
| (``synthorg.api.auth.csrf`` and ``synthorg.api.etag``) keep | |
| the inline ``name.lower() == b"..."`` pattern: byte equality is | |
| The helper is ``str``-only by design. Four ASGI-layer call | |
| sites that compare bytes-typed header names | |
| (``synthorg.api.auth.csrf`` and ``synthorg.api.etag``) keep | |
| the inline ``name.lower() == b"..."`` pattern: byte equality is |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/api/endpoints/reports.ts (1)
33-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate report-period values before returning a narrowed union.
Line 33–37 currently trusts the wire payload as
ReportPeriod[]via generics only. If backend sends an unexpected value, it will still flow through as a typed union and can break downstream exhaustive logic.Suggested fix
export const REPORT_PERIOD_VALUES = ['daily', 'weekly', 'monthly'] as const export type ReportPeriod = (typeof REPORT_PERIOD_VALUES)[number] + +function isReportPeriod(value: string): value is ReportPeriod { + return (REPORT_PERIOD_VALUES as readonly string[]).includes(value) +} @@ - const response = await apiClient.get<PaginatedResponse<ReportPeriod>>( + const response = await apiClient.get<PaginatedResponse<string>>( '/reports/periods', { signal: options.signal }, ) - return unwrapPaginated<ReportPeriod>(response).data + const periods = unwrapPaginated<string>(response).data + if (!periods.every(isReportPeriod)) { + throw new Error('Invalid report period received from API') + } + return periods }🤖 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/api/endpoints/reports.ts` around lines 33 - 37, The code trusts the wire payload from apiClient.get as ReportPeriod[] via the generic and returns unwrapPaginated(response).data directly; instead validate each returned value against the allowed ReportPeriod union before narrowing. Add an isValidReportPeriod(value): boolean that checks the permissible enum/string members, run it over the array returned from unwrapPaginated(response).data, and either throw a descriptive error (or filter out invalid values and log a warning) so downstream exhaustive logic isn’t given invalid members; update the code paths that call apiClient.get and unwrapPaginated to use this validation (referencing apiClient.get, unwrapPaginated, and the ReportPeriod type).
🤖 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/protocols-audit.md`:
- Line 384: The markdown issue is the malformed inline code span in the title
string `[CLEANUP] Protocols audit follow-up: REVIEW pass for `_PrivatePrefixed`
typing seams`; fix it by rephrasing or escaping backticks so MD038 is
satisfied—for example, wrap the identifier in a single valid code span like
`[CLEANUP] Protocols audit follow-up: REVIEW pass for `_PrivatePrefixed` typing
seams` -> `[CLEANUP] Protocols audit follow-up: REVIEW pass for
`_PrivatePrefixed` typing seams` (replace with a valid backtick pairing such as
`[CLEANUP] Protocols audit follow-up: REVIEW pass for `_PrivatePrefixed` typing
seams` or use inline code without adjacent backticks/spacing issues or escape
the inner backtick), targeting the title text in protocols-audit.md where the
`_PrivatePrefixed` token appears.
- Around line 34-45: Replace the hard-coded numeric values and percentage with
runtime-stats markers pulled from the runtime_stats.yaml file: change the table
cells and the sentence "`@runtime_checkable` decoration: 209 of 237 (88%). The
28 without..." to use <!--RS:TOTAL-->, <!--RS:KEEP-->,
<!--RS:MAKE_RUNTIME_CHECKABLE-->, <!--RS:REMOVE-->, <!--RS:REVIEW-->,
<!--RS:RUNTIME_CHECKABLE_COUNT-->/<!--RS:TOTAL--> and
<!--RS:RUNTIME_CHECKABLE_PERCENT--> (or similarly named keys present in
runtime_stats.yaml); ensure the inline list of the 28 entries also uses
<!--RS:WITHOUT_DECORATOR_COUNT--> marker so all public numerics render from the
YAML via <!--RS:NAME--> markers.
In `@src/synthorg/hr/registry.py`:
- Line 205: get_by_name uses find_by_name_ci (normalized/casefold comparison)
but get_by_names uses a simple .lower() path so batch vs single lookups can
disagree; make get_by_names use the same normalization semantics as get_by_name
by either calling find_by_name_ci for each requested name or by applying the
same normalization function (casefold + trim) used by find_by_name_ci to the
lookup keys and stored names so both get_by_name and get_by_names behave
identically (update functions: get_by_names and any helper that lowercases to
use the same normalization routine or invoke find_by_name_ci).
In `@web/src/__tests__/pages/new-surfaces.test.tsx`:
- Around line 5-8: The comment saying "Workflow-executions and webhooks handlers
are defined locally" is misleading because no MSW handlers or server.use() calls
exist; either add the local handlers (define handler functions for the
workflow-executions and webhooks endpoints and register them with
server.use(...), or remove/replace that line to state that those handlers come
from the global test setup); look for the exact comment string
"Workflow-executions and webhooks handlers are defined locally" in
new-surfaces.test.tsx and update it or add MSW handler definitions and
registration accordingly.
In `@web/src/api/types/enums.ts`:
- Around line 10-13: The exported const tuples (e.g., SENIORITY_LEVEL_VALUES)
should use the stricter TypeScript pattern to prevent type widening; replace
each bare "as const" on the *_VALUES arrays with "as const satisfies readonly
string[]" (for every tuple exported in this file) so the tuple remains readonly
and each element is constrained to string literals at compile time, leaving the
existing derived union types (e.g., SeniorityLevel = (typeof
SENIORITY_LEVEL_VALUES)[number]) unchanged; update all 16 *_VALUES symbols
similarly (e.g., SENIORITY_LEVEL_VALUES and the other exported *_VALUES arrays)
to follow this pattern.
---
Outside diff comments:
In `@web/src/api/endpoints/reports.ts`:
- Around line 33-37: The code trusts the wire payload from apiClient.get as
ReportPeriod[] via the generic and returns unwrapPaginated(response).data
directly; instead validate each returned value against the allowed ReportPeriod
union before narrowing. Add an isValidReportPeriod(value): boolean that checks
the permissible enum/string members, run it over the array returned from
unwrapPaginated(response).data, and either throw a descriptive error (or filter
out invalid values and log a warning) so downstream exhaustive logic isn’t given
invalid members; update the code paths that call apiClient.get and
unwrapPaginated to use this validation (referencing apiClient.get,
unwrapPaginated, and the ReportPeriod type).
🪄 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: c988da98-d853-464b-aa55-c707b2441ad1
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.gitignoreCLAUDE.mddocs/reference/protocols-audit.mdscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/auth/csrf.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/etag.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/core/normalization.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/registry.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/tools/network_validator.pysrc/synthorg/tools/web/http_request.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/core/test_normalization.pytests/unit/settings/test_dispatcher.pyweb/package.jsonweb/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
💤 Files with no reviewable changes (2)
- .gitignore
- web/package.json
📜 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: CodSpeed Python Pass
- GitHub Check: Build Backend
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (11)
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,js,jsx}: Always usecreateLoggerfrom@/lib/logger; never use bareconsole.warn,console.error, orconsole.debugin application code
Logger variable name must always belog(e.g.,const log = createLogger('module-name'))
Pass dynamic or untrusted values to logger methods as separate arguments (not interpolated into the message string) so they go throughsanitizeArg
Wrap attacker-controlled fields inside structured objects insanitizeForLog()before embedding them in logs
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Callers MUST NOT wrap store mutation calls in try/catch; the store owns the error UX
Any new caller of health/readiness endpoints must handle the 503 path explicitly
Any new WS payload handler that ingests untrusted strings MUST route throughsanitizeWsString()orsanitizeWsEnum()helpers
ImportErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported fromweb/src/api/types/error-codes.gen.ts); discriminate onErrorCode.<NAME>, never on raw integer literals
Never export agetXIcon(value): LucideIconfactory that returns a component reference called inside JSX render bodies; export a<XIcon value={...} {...lucideProps} />wrapper component instead usingcreateElementfor the lookup
Detectfetch()in effects withoutAbortControllercleanup using the@eslint-react/web-api-no-leaked-fetchrule
Catch the{count && <Foo />}bug using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...)
Restrictwindow,document,localStorage, and other globals inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handler,useEffect, oruseSyncExternalStore-backed hookReuse
web/src/components/ui/and design tokens only for Web Dashboard Design System
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Test setup must boot with
onUnhandledRequest: 'error'; tests override per-case viaserver.use(...), nevervi.mock('@/api/endpoints/*')
Files:
web/src/__tests__/pages/new-surfaces.test.tsx
src/synthorg/!(persistence)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL
Files:
src/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/core/normalization.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/etag.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/tools/network_validator.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration precedence: DB > env > YAML > code default via
SettingsService/ConfigResolver; noos.environ.getoutside startupNo hardcoded numeric values; numerics live in
settings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2No
from __future__ import annotations(3.14 has PEP 649); use PEP 758 except forexcept A, B:(no parens unless binding)Type hints required on public functions; mypy strict mode enforced
Use Google-style docstrings
Line length 88; functions <50 lines; files <800 lines
Errors must follow
<Domain><Condition>Errorpattern inheriting fromDomainError, never directly fromException/RuntimeError/etcPydantic v2: use frozen +
extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes); use@computed_fieldfor derived; useNotBlankStrfor identifiersArgs models at every system boundary; use
parse_typed()for every external dict ingestion enforced bycheck_boundary_typed.pyImmutability: use
model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundariesAsync: use
asyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError)Clock seam: declare
clock: Clock | None = None; tests injectFakeClockLifecycle: services own
_lifecycle_lock; timed-out stops mark unrestartableUntrusted content (SEC-1): use
wrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTMLImport logger via
from synthorg.observability import get_logger; variable always namedlogger. Neverimport loggingorprint()in app codeEvent names from
observability.events.<domain>constants; use structured kwargs (logger.info(EVENT, key=value))Error paths log WARNING/ERROR with context before raising; state transitions log INFO via
*_STATUS_TRANSITIONEDAFTER persistence writeSecret-log redaction (SEC-1): never log
error=str(exc)or interpolate{exc}; use `error_type=type(exc).__na...
Files:
src/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/core/normalization.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/etag.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/tools/network_validator.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/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/core/normalization.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/etag.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/tools/network_validator.py
src/synthorg/providers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Provider calls go through
BaseCompletionProvider(retry + rate limit); never implement retry in driver subclasses. Retryable:RateLimitError,Provider{Timeout,Connection,Internal}Error
Files:
src/synthorg/providers/drivers/litellm_driver.py
{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers
Files:
docs/reference/protocols-audit.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers: use
@pytest.mark.{unit,integration,e2e,slow}. Async tests useauto. Global timeout 30s. Coverage minimum 80%Mock-spec: every Mock declares
spec=ConcreteClass; baseline atscripts/mock_spec_baseline.txtUse
FakeClockfromtests._shared.fake_clock; inject viaclock=Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...))Flaky tests: NEVER skip/xfail; fix fundamentally. Use
asyncio.Event().wait()notsleep(large)
Files:
tests/unit/core/test_normalization.pytests/unit/settings/test_dispatcher.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.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/core/test_normalization.pytests/unit/settings/test_dispatcher.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.py
web/src/{api,stores}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Display counts come from
data.length; the wire envelope no longer carriestotalfield for paginated responses
Files:
web/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/api/endpoints/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state)
Files:
web/src/api/endpoints/reports.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Read `docs/design/` page before implementing; deviations need approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: No region/currency/locale privileged; use metric units; use British English
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Every convention PR must ship its enforcement gate per docs/reference/convention-gates.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Timeout/slow test failures indicate source-code regression; never edit `tests/baselines/unit_timing.json`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: After issue implementation: branch + commit + push (no auto-PR); use `/pre-pr-review` instead of auto-creating PR; after PR creation use `/aurelio-review-pr` for external feedback
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Fix EVERYTHING valid in review; no deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: xdist: `-n 8 --dist=loadfile` auto-applied via pyproject `addopts` (`loadfile` prevents 3.14+Windows ProactorEventLoop leak)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Git commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: Squash merge: PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: GitHub queries: use `gh issue list` via Bash, NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: After every squash merge → run `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: For Web: follow `web/CLAUDE.md` guidelines
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T10:31:08.336Z
Learning: For CLI: use `go -C cli` (never `cd cli`); follow `cli/CLAUDE.md` guidelines
📚 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/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/core/normalization.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/human_strategy.pytests/unit/core/test_normalization.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/api/auth/csrf.pytests/unit/settings/test_dispatcher.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/etag.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pytests/unit/communication/conflict_resolution/test_human_strategy.pysrc/synthorg/tools/network_validator.py
🪛 markdownlint-cli2 (0.22.1)
docs/reference/protocols-audit.md
[warning] 384-384: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 384-384: Spaces inside code span elements
(MD038, no-space-in-code)
🔇 Additional comments (43)
CLAUDE.md (1)
46-46: Reference index update looks goodAdding
docs/reference/protocols-audit.mdto the load-on-demand reference list is consistent with the new document and improves discoverability.web/src/__tests__/pages/new-surfaces.test.tsx (1)
36-99: MSW test setup is correctly configured.Verification confirms the global test setup in
web/src/test-setup.tsxincludesserver.listen({ onUnhandledRequest: 'error' })on line 87, satisfying the coding guideline requirement. The test file uses this global configuration, ensuring proper error handling for unmatched requests.src/synthorg/communication/conflict_resolution/human_strategy.py (1)
91-92: Default decision processor migration looks correct.Line 133 now explicitly defaults to
HumanDecisionProcessor(mode="winner"), which keeps resolver defaults unambiguous after removing the legacy processor classes.Also applies to: 129-135
src/synthorg/communication/conflict_resolution/escalation/factory.py (1)
314-315: Decision-strategy registry update is clean and deterministic.The winner/hybrid keys now map to
HumanDecisionProcessorwith explicit mode selection, preserving existing factory semantics while removing duplicate processor classes.src/synthorg/communication/conflict_resolution/escalation/protocol.py (1)
178-185: Protocol contract docs are now aligned with the implementation.The updated DecisionProcessor docstring clearly states mode-dependent accepted decision shapes and outcomes, which improves integration clarity for callers.
tests/unit/communication/conflict_resolution/test_human_strategy.py (1)
98-99: Mode-specific test migration is solid.These updates correctly validate winner and hybrid behaviour via
HumanDecisionProcessor(mode=...), keeping behavioural coverage intact after class consolidation.Also applies to: 139-140, 163-174, 213-214
src/synthorg/communication/conflict_resolution/escalation/__init__.py (1)
34-35: Package exports are correctly updated for the processor refactor.
HumanDecisionProcessoris now imported and exposed through__all__, matching the consolidated escalation processor design.Also applies to: 56-57
tests/unit/communication/conflict_resolution/escalation/test_factory_registry.py (1)
303-313: Factory tests now assert the right post-refactor contract.Validating both
HumanDecisionProcessortype andprocessor.modefor winner/hybrid is the correct guard against strategy wiring regressions.src/synthorg/communication/conflict_resolution/escalation/processors.py (1)
68-101: Processor consolidation is well implemented.The
HumanDecisionProcessormode discriminator, runtime validation, and explicit winner/hybrid branching provide a clean replacement for the previous split classes.Also applies to: 118-150
src/synthorg/api/auth/service.py (1)
55-95: Docstring expansion is clear and consistent with implementation.The added class-level guidance on async boundaries, thread-safety, and scope boundaries is precise and matches the concrete method behaviour in this class.
src/synthorg/core/normalization.py (1)
47-73:compare_cihelper looks correct and well-scoped.Good consolidation on top of
normalize_identifier; this keeps case-insensitive equality semantics consistent across call sites.src/synthorg/engine/routing/scorer.py (1)
15-16: Role matching migration tocompare_ciis sound.This change preserves scoring logic while standardising case-insensitive matching through the shared normalisation helper.
Also applies to: 328-330
src/synthorg/api/controllers/_department_health.py (1)
23-23: Department matching update is clean and consistent.Using
compare_cihere removes duplicated normalisation logic and aligns controller behaviour with the new shared helper.Also applies to: 107-107
src/synthorg/tools/network_validator.py (1)
22-23: HTTPS scheme comparison refactor is correct.This keeps semantics intact while moving another case-insensitive check onto the shared normalisation path.
Also applies to: 411-411
src/synthorg/api/etag.py (1)
216-217: ASGI bytes-header exemption comments are appropriate.The added notes are clear and accurately document why
compare_ciis intentionally not used at these byte-typed header sites.Also applies to: 361-362
src/synthorg/api/auth/csrf.py (1)
179-180: Comment-only update is good and consistent with ASGI header handling.The exemption notes are accurate and make the byte-vs-string boundary explicit for future maintainers.
Also applies to: 207-208
src/synthorg/engine/hybrid_helpers.py (1)
13-13: Replan decision parsing change is solid.Using
compare_cihere keeps boolean-string interpretation aligned with the new shared comparison strategy.Also applies to: 392-392
src/synthorg/api/auth/middleware.py (1)
27-27: Bearer-scheme parsing refactor is correct.This migration to
compare_ci()keeps the auth-header check consistent without changing control flow.Also applies to: 194-195
src/synthorg/tools/web/http_request.py (1)
15-15: Host-header normalisation update looks good.Using
compare_ci()for dedup keeps header handling consistent and avoids scattered manual lowercasing.Also applies to: 284-286
src/synthorg/communication/meeting/participant.py (1)
10-10:"all"participant matching change is solid.The
compare_ci()use keeps this sentinel path aligned with the new centralised normalisation approach.Also applies to: 250-252
src/synthorg/api/controllers/settings.py (1)
37-37:enable_correlationparsing refactor is correct.This keeps boolean-string handling consistent with the shared normalisation helper.
Also applies to: 623-623
src/synthorg/tools/html_parse_guard.py (1)
17-17:aria-hiddencomparison update looks good.Using
compare_ci()here is consistent and keeps hidden-element detection logic tidy.Also applies to: 339-340
src/synthorg/providers/drivers/litellm_driver.py (1)
47-47:Retry-Afterheader lookup refactor is sound.The switch to
compare_ci()preserves behaviour and improves consistency for case-insensitive header handling.Also applies to: 724-725
src/synthorg/settings/subscribers/backup_subscriber.py (1)
5-5: Backup enabled-flag normalisation change is good.
compare_ci()keeps this toggle path consistent with the rest of the refactor.Also applies to: 114-114
src/synthorg/api/services/_org_agent_mutations.py (1)
19-19: Agent mutation comparison migrations are clean and consistent.The updated
compare_ci()call sites keep update/delete/reorder matching aligned with the shared normalisation strategy.Also applies to: 171-172, 238-239, 278-280, 292-293, 335-336, 347-348
src/synthorg/hr/registry.py (1)
281-282: Nice use of the shared comparator for department filteringThis keeps CI comparison behaviour centralised and avoids drift from ad-hoc string logic.
src/synthorg/api/services/org_mutations.py (1)
188-189: Lookup refactor looks goodUsing
find_by_name_ciin both helper methods reduces duplicated matching logic and keeps behaviour consistent across call sites.Also applies to: 214-214
src/synthorg/integrations/webhooks/verifiers/github_hmac.py (1)
6-6: Good centralisation of case-insensitive header matchingThis removes ad-hoc casing logic while preserving the verifier’s security-critical digest flow.
Also applies to: 26-27
src/synthorg/integrations/webhooks/verifiers/generic_hmac.py (1)
6-6: LGTM on the header lookup refactor
compare_ciusage keeps matching semantics consistent with the other webhook verifier.Also applies to: 53-54
src/synthorg/hr/training/source_selectors/department_diversity.py (1)
64-70: Strong improvement to observability and comparison consistencyStructured invalid-config logging plus
compare_cirole partitioning makes behaviour clearer and easier to audit.Also applies to: 123-124
src/synthorg/api/services/_org_department_mutations.py (1)
19-19: Good CI-comparison consolidation across department mutationsThe shared comparator usage improves consistency across update/delete/conflict checks.
Also applies to: 189-190, 250-251, 290-290
tests/unit/core/test_normalization.py (1)
76-115: Great addition of contract-level tests forcompare_ciThe parametrised + property-based coverage gives strong confidence that helper behaviour remains aligned with
normalize_identifier.src/synthorg/hr/training/source_selectors/role_top_performers.py (1)
55-61: Looks good: structured config logging + standardised role matchingBoth changes improve consistency with the other training selectors.
Also applies to: 90-90
src/synthorg/settings/dispatcher.py (4)
106-111: LGTM on the fire-and-forget task retention pattern.The strong-reference set with a discard callback correctly prevents garbage collection mid-flight while avoiding unbounded growth. The RUF006 compliance is well-documented.
335-352: Sound approach for async state reset under lifecycle lock.The callback correctly handles the "no running loop" edge case (interpreter shutdown) and the
discardcallback for cleanup is appropriate. The task identity check in_reset_running_under_lockprevents clearing state for a stale task if a newstart()occurred in the interim.
354-366: Correct serialisation of crash-recovery state reset.The
if self._task is taskguard at line 364 is essential—it prevents a race where:
- Task A crashes, schedules reset
stop()completes,start()spawns Task B- Reset for Task A finally runs but incorrectly clears Task B's state
The identity check ensures only the crashed task's cleanup proceeds.
490-490: Correct migration tocompare_ci.The logic
not compare_ci(str(restart_raw), "false")preserves the fail-safe semantics: restart_required defaults toTrueunless the value is explicitly"false"(case-insensitive).tests/unit/settings/test_dispatcher.py (2)
529-535: LGTM on test synchronisation with async reset.Waiting on
_post_done_tasksbefore asserting ensures deterministic behaviour aligned with the new lifecycle-locked reset in the dispatcher.
641-643: Consistent pattern applied to consecutive-errors test.Same synchronisation fix as above, ensuring the assertion observes the post-lock state.
scripts/long_running_loops_kill_switch_baseline.txt (1)
31-31: Baseline line number correctly updated.The
while True:in_poll_loopis now at line 382, matching this entry.scripts/loop_bound_init_baseline.txt (1)
34-34: Baseline entry correctly reflects_lifecycle_locklocation.The
asyncio.Lock()assignment is at line 105 in the dispatcher's__init__.scripts/no_magic_numbers_baseline.txt (1)
780-781: Baseline coordinates correctly updated for dispatcher constants.Lines 780-781 track
_BOOTSTRAP_MAX_CONSECUTIVE_ERRORSand_STOP_DRAIN_TIMEOUT_SECONDSat their new positions after the_post_done_tasksdocblock insertion.web/src/api/endpoints/reports.ts (1)
7-20: Good API-shape consolidation after generated-type removal.Local
ReportResponse/GenerateReportRequestdefinitions and usage ingenerateReportare clear and keep endpoint contracts readable.Also applies to: 40-48
| ## Summary | ||
|
|
||
| | Total | Recommendation | | ||
| |---|---| | ||
| | **237** | Protocol classes inventoried | | ||
| | **193** | KEEP | | ||
| | **0** | MAKE-RUNTIME-CHECKABLE (no `isinstance` sites surveyed; deferred to per-area cleanup PRs) | | ||
| | **31** | REMOVE candidates (flag-only; must be re-verified at cleanup time) | | ||
| | **13** | REVIEW (`_PrivatePrefixed` typing seams; intent inspection needed) | | ||
|
|
||
| `@runtime_checkable` decoration: 209 of 237 (88%). The 28 without the decorator are listed inline below; absence is not a defect on its own; only a few need it. | ||
|
|
There was a problem hiding this comment.
Source summary numerics via runtime-stats markers
Lines 34–45 hard-code public numeric claims (totals, counts, percentage). These should be rendered from data/runtime_stats.yaml using <!--RS:NAME--> markers so the values stay authoritative and gateable.
As per coding guidelines: “Numerics in README and public docs must be sourced from data/runtime_stats.yaml via <!--RS:NAME--> markers”.
🤖 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/protocols-audit.md` around lines 34 - 45, Replace the
hard-coded numeric values and percentage with runtime-stats markers pulled from
the runtime_stats.yaml file: change the table cells and the sentence
"`@runtime_checkable` decoration: 209 of 237 (88%). The 28 without..." to use
<!--RS:TOTAL-->, <!--RS:KEEP-->, <!--RS:MAKE_RUNTIME_CHECKABLE-->,
<!--RS:REMOVE-->, <!--RS:REVIEW-->,
<!--RS:RUNTIME_CHECKABLE_COUNT-->/<!--RS:TOTAL--> and
<!--RS:RUNTIME_CHECKABLE_PERCENT--> (or similarly named keys present in
runtime_stats.yaml); ensure the inline list of the 28 entries also uses
<!--RS:WITHOUT_DECORATOR_COUNT--> marker so all public numerics render from the
YAML via <!--RS:NAME--> markers.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1859 +/- ##
==========================================
- Coverage 84.89% 84.86% -0.03%
==========================================
Files 1806 1806
Lines 105078 105211 +133
Branches 9200 9214 +14
==========================================
+ Hits 89205 89288 +83
- Misses 13647 13688 +41
- Partials 2226 2235 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as spam.
This comment was marked as spam.
Six valid findings from CodeRabbit + Gemini, two skipped as invalid. Fixed: - src/synthorg/core/normalization.py: docstring 'Three' updated to 'Four' ASGI byte-header call sites (gemini) - src/synthorg/hr/registry.py: get_by_names now uses normalize_identifier so batch and single lookups share casefold + whitespace-strip semantics with get_by_name (coderabbit) - docs/reference/protocols-audit.md: escape malformed inline code span on line 384 (MD038) (coderabbit) - web/src/api/types/enums.ts: 'as const satisfies readonly string[]' on all 16 *_VALUES tuples to prevent silent type widening (coderabbit) - web/src/api/endpoints/reports.ts: validate /reports/periods payload against REPORT_PERIOD_VALUES before narrowing (coderabbit) - web/src/__tests__/pages/new-surfaces.test.tsx: drop misleading 'defined locally' comment; handlers come from global setup (coderabbit) Skipped (factually wrong): - gemini: 'NameError due to missing future annotations' on processors.py: Python 3.14+ has PEP 649 deferred eval; the project explicitly forbids 'from __future__ import annotations'. - coderabbit: 'use RS markers for numerics' on protocols-audit.md: file is intentionally not in scripts/check_doc_numeric_macros.py _SCOPED_FILES; audit numbers are a date-pinned snapshot, not live runtime stats.
Aureliolo
force-pushed
the
chore/issue-1850-cleanup
branch
from
1c13d22 to
6c6e84f
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/communication/conflict_resolution/escalation/processors.py (1)
118-150: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winFail fast for unsupported decision variants in the non-winner branch
Line 124-Line 150 currently treats every non-
WinnerDecisionas reject. IfEscalationDecisionis extended later, a new variant will be misclassified asREJECTED_BY_HUMANinstead of failing loudly.Proposed hardening
from synthorg.communication.conflict_resolution.escalation.models import ( EscalationDecision, + RejectDecision, WinnerDecision, ) @@ - # Union ``EscalationDecision`` is exhaustive (``WinnerDecision`` - # | ``RejectDecision``); reaching this branch means the decision - # is a ``RejectDecision``. - if self._mode == "winner": - # Raised as ValueError (rather than TypeError) because the - # caller is the REST layer validating payload shapes; the - # escalations controller translates this into a 422 - # ValidationError. - msg = ( - "HumanDecisionProcessor in 'winner' mode only accepts " - "'winner' decisions. Configure decision_strategy='hybrid' " - "to allow 'reject' decisions." - ) - logger.warning( - CONFLICT_ESCALATION_RESOLVED, - conflict_id=conflict.id, - decided_by=decided_by, - decision_type=getattr(decision, "type", type(decision).__name__), - strategy=ConflictResolutionStrategy.HUMAN.value, - note="winner_select_rejected_non_winner", - ) - raise ValueError(msg) - return self._build_reject_resolution( - conflict, - decision, - decided_by=decided_by, - ) + if isinstance(decision, RejectDecision): + if self._mode == "winner": + msg = ( + "HumanDecisionProcessor in 'winner' mode only accepts " + "'winner' decisions. Configure decision_strategy='hybrid' " + "to allow 'reject' decisions." + ) + logger.warning( + CONFLICT_ESCALATION_RESOLVED, + conflict_id=conflict.id, + decided_by=decided_by, + decision_type=getattr(decision, "type", type(decision).__name__), + strategy=ConflictResolutionStrategy.HUMAN.value, + note="winner_select_rejected_non_winner", + ) + raise ValueError(msg) + return self._build_reject_resolution( + conflict, + decision, + decided_by=decided_by, + ) + + msg = f"Unsupported escalation decision type: {type(decision).__name__}" + raise ValueError(msg)🤖 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/communication/conflict_resolution/escalation/processors.py` around lines 118 - 150, The current branch assumes any non-WinnerDecision is a RejectDecision, which will silently misclassify future EscalationDecision variants; update the logic in HumanDecisionProcessor (the block after the isinstance(decision, WinnerDecision) check) to explicitly check for RejectDecision (e.g., isinstance(decision, RejectDecision)) and call _build_reject_resolution only in that case, and otherwise raise a clear error (TypeError or ValueError) indicating an unsupported EscalationDecision variant; keep the existing logging and the existing winner-mode ValueError behavior unchanged.
🤖 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/protocols-audit.md`:
- Line 9: The audit file header currently states it was "Generated mechanically
from `_audit/protocol_audit.py` (uncommitted)," which prevents reproducible
verification; fix this by either committing the generator script
`_audit/protocol_audit.py` into the repository and updating the header to
reference the committed path, or (if committing is not acceptable) replace the
header with a precise reproducible command and pinned context that was used to
produce the file (e.g., exact git commit/SHA, virtualenv/python version, pip
freeze or requirements hash, and the shell command to run the generator); update
the line in docs/reference/protocols-audit.md and ensure any mention of
“Protocol class” generation references the committed script or the exact pinned
command/context you provide.
In `@src/synthorg/communication/conflict_resolution/human_strategy.py`:
- Around line 133-135: The assignment to self._processor uses truthiness
("processor or HumanDecisionProcessor(...)") which can incorrectly override a
valid but falsy injected processor; change the fallback to an explicit None
check so that a provided processor is preserved (e.g. use "processor if
processor is not None else HumanDecisionProcessor(...)" or set self._processor
to HumanDecisionProcessor(...) only when processor is None), referencing the
constructor parameter "processor" and the default "HumanDecisionProcessor" used
here.
In `@src/synthorg/settings/dispatcher.py`:
- Around line 343-366: The transient “already running” race happens because
_on_task_done schedules _reset_running_under_lock but start() can check _running
before that callback runs; modify start() to acquire the same _lifecycle_lock
and, while holding it, detect if self._task is done and collapse it (set
self._task = None and self._running = False) before evaluating the existing
already-running guard; use the existing symbols _lifecycle_lock, _task,
_running, and _post_done_tasks/_reset_running_under_lock to ensure the cleanup
is serialized with the scheduled follow-up and prevents start() from misreading
a completed task as still running.
---
Outside diff comments:
In `@src/synthorg/communication/conflict_resolution/escalation/processors.py`:
- Around line 118-150: The current branch assumes any non-WinnerDecision is a
RejectDecision, which will silently misclassify future EscalationDecision
variants; update the logic in HumanDecisionProcessor (the block after the
isinstance(decision, WinnerDecision) check) to explicitly check for
RejectDecision (e.g., isinstance(decision, RejectDecision)) and call
_build_reject_resolution only in that case, and otherwise raise a clear error
(TypeError or ValueError) indicating an unsupported EscalationDecision variant;
keep the existing logging and the existing winner-mode ValueError behavior
unchanged.
🪄 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: fce507d1-c0ec-410e-8f86-1033d5af04bb
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.gitignoreCLAUDE.mddocs/reference/protocols-audit.mdscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtsrc/synthorg/api/auth/csrf.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/etag.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/core/normalization.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/registry.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/tools/network_validator.pysrc/synthorg/tools/web/http_request.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/core/test_normalization.pytests/unit/settings/test_dispatcher.pyweb/package.jsonweb/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
💤 Files with no reviewable changes (2)
- web/package.json
- .gitignore
📜 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: Build Backend
- GitHub Check: Lighthouse Dashboard
- 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 (11)
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,js,jsx}: Always usecreateLoggerfrom@/lib/logger; never use bareconsole.warn,console.error, orconsole.debugin application code
Logger variable name must always belog(e.g.,const log = createLogger('module-name'))
Pass dynamic or untrusted values to logger methods as separate arguments (not interpolated into the message string) so they go throughsanitizeArg
Wrap attacker-controlled fields inside structured objects insanitizeForLog()before embedding them in logs
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/types/enums.tsweb/src/api/endpoints/reports.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Callers MUST NOT wrap store mutation calls in try/catch; the store owns the error UX
Any new caller of health/readiness endpoints must handle the 503 path explicitly
Any new WS payload handler that ingests untrusted strings MUST route throughsanitizeWsString()orsanitizeWsEnum()helpers
ImportErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported fromweb/src/api/types/error-codes.gen.ts); discriminate onErrorCode.<NAME>, never on raw integer literals
Never export agetXIcon(value): LucideIconfactory that returns a component reference called inside JSX render bodies; export a<XIcon value={...} {...lucideProps} />wrapper component instead usingcreateElementfor the lookup
Detectfetch()in effects withoutAbortControllercleanup using the@eslint-react/web-api-no-leaked-fetchrule
Catch the{count && <Foo />}bug using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...)
Restrictwindow,document,localStorage, and other globals inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handler,useEffect, oruseSyncExternalStore-backed hookReuse
web/src/components/ui/design tokens only in React web dashboard
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/types/enums.tsweb/src/api/endpoints/reports.ts
web/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Test setup must boot with
onUnhandledRequest: 'error'; tests override per-case viaserver.use(...), nevervi.mock('@/api/endpoints/*')
Files:
web/src/__tests__/pages/new-surfaces.test.tsx
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration precedence: DB > env > YAML > code default via
SettingsService/ConfigResolver; noos.environ.getoutside startup
Files:
src/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/core/normalization.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/api/etag.pysrc/synthorg/tools/network_validator.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/auth/service.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/api/services/org_mutations.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics live in
settings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2. Enforced byscripts/check_no_magic_numbers.pyNo
from __future__ import annotations(3.14 has PEP 649). PEP 758 except:except A, B:no parens unless bindingType hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines
Errors:
<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeErrordirectly. Enforced bycheck_domain_error_hierarchy.pyPydantic v2 frozen +
extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes);@computed_fieldfor derived;NotBlankStrfor identifiersArgs models at every system boundary;
parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.pyUse
model_copy(update=...)orcopy.deepcopy()for immutability; deepcopy at system boundariesUse
asyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError)Clock seam:
clock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartableUntrusted content (SEC-1):
wrap_untrusted()fromengine.prompt_safety;HTMLParseGuardfor HTMLUse
from synthorg.observability import get_logger; variable alwayslogger. Neverimport logging/print()in app codeEvent names from
observability.events.<domain>constants; structured kwargs (logger.info(EVENT, key=value))Error paths log WARNING/ERROR with context before raising; state transitions log INFO via
*_STATUS_TRANSITIONEDafter persistence writeNever
error=str(exc)or interpolate{exc}; useerror_type=type(exc).__name__+error=safe_error_description(exc). Neverexc_info=True. OTel: forbidspan.record_exception(exc); usespan.set_attribute("exception.message", ...)+record_exception=False, set_status_on_exception=False. Enforced...
Files:
src/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/core/normalization.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/api/etag.pysrc/synthorg/tools/network_validator.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/auth/service.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/api/services/org_mutations.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/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/core/normalization.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/api/etag.pysrc/synthorg/tools/network_validator.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/auth/service.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/api/services/org_mutations.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use markers
@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% minWindows: unit tests use
WindowsSelectorEventLoopPolicy(3.14 IOCP teardown race). Subprocess tests override backEvery Mock declares
spec=ConcreteClass; baseline atscripts/mock_spec_baseline.txtUse
FakeClockfromtests._shared.fake_clock; inject viaclock=Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...))NEVER skip/xfail flaky tests; fix fundamentally. Use
asyncio.Event().wait()notsleep(large)
Files:
tests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/settings/test_dispatcher.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.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/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/settings/test_dispatcher.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.py
src/synthorg/providers/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Provider calls go through
BaseCompletionProvider(retry + rate limit); never implement retry in driver subclasses. Retryable:RateLimitError,Provider{Timeout,Connection,Internal}Error
Files:
src/synthorg/providers/drivers/litellm_driver.py
web/src/{api,stores}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Display counts come from
data.length; the wire envelope no longer carriestotalfield for paginated responses
Files:
web/src/api/types/enums.tsweb/src/api/endpoints/reports.ts
web/src/api/endpoints/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state)
Files:
web/src/api/endpoints/reports.ts
{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README + public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers
Files:
docs/reference/protocols-audit.md
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use
d2for architecture / nested containers,mermaidfor flowcharts / sequence / pipelines. Markdown tables for tabular data
Files:
docs/reference/protocols-audit.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Read `docs/design/` page before implementing; deviations need approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Use no region/currency/locale privileged defaults; use metric units; use British English
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Every convention PR must ship its enforcement gate
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Timeout/slow failures = source-code regression; never edit `tests/baselines/unit_timing.json`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: After issue: branch + commit + push (no auto-PR); use `/pre-pr-review` before PR creation
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: WebSocket: per-frame timeout closes silent peers (1008); revalidation saturation closes (4011)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Commit messages: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Squash merge. PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: After every squash merge → `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: D2 theme 200 (Dark Mauve), D2 CLI pinned to v0.7.1 in CI
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Use `uv sync` for dependency management; group 'docs' for documentation toolchain
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:03:53.900Z
Learning: Use `go -C cli` never `cd cli` for CLI operations
📚 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/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/core/normalization.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/api/etag.pytests/unit/core/test_normalization.pysrc/synthorg/tools/network_validator.pysrc/synthorg/engine/hybrid_helpers.pytests/unit/communication/conflict_resolution/test_human_strategy.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/auth/service.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pytests/unit/settings/test_dispatcher.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/providers/drivers/litellm_driver.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/api/services/org_mutations.py
🔇 Additional comments (35)
web/src/api/types/enums.ts (3)
10-102: Strong type/value lockstep pattern applied consistently.Using
as const satisfies readonly string[]plus derived unions from each*_VALUEStuple is clean and prevents drift between runtime values and TypeScript unions.
63-72: Type guard implementation looks solid.
Set-backed membership withvalue is DepartmentNameis a good, efficient narrowing path for raw string inputs.
104-117: Good boundary documentation for hand-maintained unions.The explicit note about schema exposure status keeps the temporary manual types understandable and maintainable.
web/src/api/endpoints/reports.ts (3)
4-11: Nice local enum+guard foundation.The
REPORT_PERIOD_VALUES+isReportPeriodsetup is clear and gives a safe runtime boundary for downstream strict unions.
32-53: Validation-first narrowing inlistReportPeriodsis well implemented.Fetching as
string[], validating against the allowed set, then returning narrowed values is a robust forward-safety pattern.
55-61: Request payload typing is precise.
{ period } satisfies GenerateReportRequestkeeps the payload contract explicit without widening.web/src/__tests__/pages/new-surfaces.test.tsx (1)
2-6: Previous issue resolved: comment now accurately reflects handler setup.The misleading statement claiming handlers were "defined locally" has been removed. The updated comment correctly states that MSW handlers are "configured in the global test setup," which is consistent with the absence of any
server.use()calls or handler definitions in this file.docs/reference/protocols-audit.md (2)
34-45: Unresolved: numeric claims still hard-coded instead of runtime-stats markersThis is still open from prior review: public numeric claims here should be sourced via
<!--RS:NAME-->markers, not hard-coded literals.As per coding guidelines: “Numerics in README + public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers”.
384-384: Unresolved: malformed inline code span remains in follow-up issue titleThe line still uses invalid nested backticks for
_PrivatePrefixed, which re-triggers markdownlint MD038.src/synthorg/communication/conflict_resolution/escalation/processors.py (2)
92-101: Mode validation and exposedmodeproperty are solidThe constructor guard plus read-only property gives a clear and safe discriminator surface.
152-158: Dissent-record builder delegation improves cohesionKeeping
build_dissent_records()as a thin delegate to the shared helper avoids drift between processor modes.src/synthorg/api/auth/service.py (1)
55-95: Docstring update is clear and implementation-aligned.The expanded class docstring accurately documents async boundaries, thread-safety expectations, and AuthService scope without changing runtime behaviour.
src/synthorg/core/normalization.py (1)
47-73:compare_ciis a solid centralisation of CI string equality.Delegating to
normalize_identifier()keeps comparison semantics consistent and avoids duplicated inline normalisation logic.src/synthorg/api/etag.py (1)
216-217: Good rationale comments for the bytes-header exemption.The added notes clearly justify keeping bytes-level header comparisons inline at the ASGI layer.
Also applies to: 361-362
src/synthorg/tools/network_validator.py (1)
22-23: Consistent helper adoption for scheme matching.Using
compare_cihere keeps scheme checks aligned with the shared case-insensitive comparison convention.Also applies to: 411-411
src/synthorg/tools/web/http_request.py (1)
15-16: Host-header normalisation change looks correct.Case-insensitive filtering before re-inserting canonical
Hostis the right behaviour for header deduplication.Also applies to: 284-286
src/synthorg/tools/html_parse_guard.py (1)
17-18:aria-hiddencomparison update is correct and robust.Using
compare_cihere improves hidden-element detection consistency for attacker-controlled attribute casing.Also applies to: 339-340
src/synthorg/engine/routing/scorer.py (1)
15-16: Role-match migration tocompare_ciis clean.This keeps role matching semantics consistent with the shared normalisation utility.
Also applies to: 328-330
src/synthorg/api/controllers/_department_health.py (1)
23-24: Department filtering update is correct.Using
compare_cihere is a good consistency win with unchanged functional intent.Also applies to: 107-107
src/synthorg/api/auth/csrf.py (1)
179-180: Good clarification on bytes-header handling.These notes make the
compare_cistr-only boundary explicit while preserving the correct ASGI bytes-header path.Also applies to: 207-208
src/synthorg/engine/hybrid_helpers.py (1)
13-13: Nice migration to shared case-insensitive parsing.Using
compare_cihere keeps behaviour consistent with the new normalisation contract without changing parser fallbacks.Also applies to: 392-392
src/synthorg/api/controllers/settings.py (1)
37-37: Consistent boolean parsing update.Switching
enable_correlationparsing tocompare_ciis clean and aligns with the shared normalisation approach.Also applies to: 623-623
src/synthorg/api/services/org_mutations.py (1)
25-25: Good deduplication of CI name lookups.Delegating both department and agent resolution to
find_by_name_cikeeps semantics consistent and simplifies maintenance.Also applies to: 188-188, 214-214
src/synthorg/providers/drivers/litellm_driver.py (1)
47-47: Solid standardisation ofretry-afterheader matching.This keeps HTTP case-insensitive semantics while aligning with the shared
compare_cihelper.Also applies to: 724-725
src/synthorg/api/auth/middleware.py (1)
27-27: Bearer scheme parsing change looks correct.Using
compare_cihere is a clean, consistent replacement for inline lowercasing.Also applies to: 194-194
src/synthorg/settings/subscribers/backup_subscriber.py (1)
5-5: Good alignment with shared normalisation.This keeps the toggle logic intact while removing one-off case-insensitive parsing.
Also applies to: 114-114
src/synthorg/integrations/webhooks/verifiers/github_hmac.py (1)
6-6: Header lookup refactor is correct.Using
compare_ciin_lookup_header_case_insensitivepreserves existing behaviour and improves consistency with other webhook verifiers.Also applies to: 26-27
src/synthorg/communication/meeting/participant.py (1)
250-252:"all"sentinel resolution looks correct and cleaner.Using
compare_cihere keeps behaviour consistent with the new normalisation helper while preserving the resolver flow.src/synthorg/integrations/webhooks/verifiers/generic_hmac.py (1)
49-54: Header matching refactor is solid.Switching to
compare_cikeeps the verifier logic intact and aligns this path with shared normalisation behaviour.src/synthorg/hr/training/source_selectors/department_diversity.py (1)
64-70: Good observability + normalisation alignment in selector updates.The structured config-invalid log payload is clearer, and the
compare_cirole split cleanly standardises case-insensitive matching.Also applies to: 123-124
scripts/long_running_loops_kill_switch_baseline.txt (1)
31-31: Baseline line update looks consistent.The entry refresh is scoped and keeps the kill-switch baseline aligned with current source locations.
tests/unit/core/test_normalization.py (1)
76-115: Strong contract coverage forcompare_ci.The parametrised cases plus property-based contract check provide good protection for Unicode and whitespace-normalisation semantics.
scripts/loop_bound_init_baseline.txt (1)
34-34: Loop-bound baseline sync is clean.The updated line reference is precise and keeps the baseline file current.
src/synthorg/api/services/_org_department_mutations.py (1)
189-190: Case-insensitive matching migration is well-applied across department mutations.These updates consistently centralise name comparisons without changing the CAS/write safety flow.
Also applies to: 250-251, 290-293
src/synthorg/api/services/_org_agent_mutations.py (1)
171-172: Nice consolidation of case-insensitive matching in agent mutations.The migrated call sites are consistent and preserve existing mutation/reorder control flow.
Also applies to: 238-239, 278-280, 292-293, 335-336, 347-352
Four valid CodeRabbit findings, all applied. - docs/reference/protocols-audit.md: replace 'uncommitted helper' parenthetical with a reproducible regeneration command so the audit's numeric snapshot is reproducible from committed state alone. - src/synthorg/communication/conflict_resolution/human_strategy.py: use 'is None' instead of 'or' for the processor fallback so a custom DecisionProcessor with a falsy __bool__ is preserved. - src/synthorg/communication/conflict_resolution/escalation/processors.py: detect RejectDecision explicitly and call assert_never on any other variant, so a future EscalationDecision member added to the union does not silently fall through as REJECTED_BY_HUMAN. - src/synthorg/settings/dispatcher.py: in start(), collapse a crashed-and-finished poll task under _lifecycle_lock before the _running guard, so a recovery start() does not race the queued _reset_running_under_lock and trip the 'already running' guard on a dispatcher whose previous task is already done.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/synthorg/communication/conflict_resolution/human_strategy.py (2)
72-72: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueTimeout constant placement.
Per coding guidelines, no hardcoded numeric values should exist outside
settings/definitions/. The_NOTIFICATION_DISPATCH_TIMEOUT_SECONDS = 10.0constant on Line 72 may need to be moved to the settings definitions.However, reviewing the allowlist ("0/1/-1, HTTP codes, hex masks, powers-of-2"), this appears to be an operational timeout rather than an allowed exception. Consider whether this should be configurable via settings.
🤖 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/communication/conflict_resolution/human_strategy.py` at line 72, The constant _NOTIFICATION_DISPATCH_TIMEOUT_SECONDS defined in human_strategy.py should be moved into the project settings definitions so timeouts aren’t hardcoded; add a new definition (e.g., NOTIFICATION_DISPATCH_TIMEOUT_SECONDS) in settings/definitions and replace usages of the _NOTIFICATION_DISPATCH_TIMEOUT_SECONDS symbol in human_strategy.py with a settings lookup (import the settings/config accessor used elsewhere in the codebase), preserving the current default value of 10.0 if unset; update any imports and consumers of human_strategy.py accordingly and adjust tests or docs that reference this value.
146-252: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
resolvemethod exceeds 50-line guideline.The
resolvemethod spans ~106 lines (146-252). Per coding guidelines, functions should be <50 lines. While the method has clear logical phases (register → persist → notify → await → process), the length makes it harder to reason about.Consider extracting phases into private async helpers (e.g.,
_persist_escalation,_await_decision) to improve readability. However, given the sequential nature of the workflow and the extensive inline comments explaining the concurrency considerations, this may be acceptable as-is.🤖 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/communication/conflict_resolution/human_strategy.py` around lines 146 - 252, The resolve method is too long (>50 lines); split its logical phases into small private async helpers to keep behavior identical: extract the store-registration-and-persist block (the call to self._registry.register, await self._store.create, the exception logging and self._registry.cancel) into a helper like _persist_escalation(escalation, conflict) that returns the registered future; extract the notifier scheduling block (asyncio.create_task, self._notify_tasks management, add_done_callback) into _schedule_notification(escalation, conflict) that returns the notify_task; extract the wait/timeout/cancel/late-decision logic (awaiting future or asyncio.wait_for, catching TimeoutError and calling _read_late_decision and _handle_timeout_cleanup, catching CancelledError and calling _handle_cancelled_cleanup) into _await_decision(future, escalation, conflict) which returns either a Decision or signals timeout/cancel resolution; and extract the final resolve/process logging into _finalize_resolution(conflict, escalation, decision) which calls _resolve_decided_by and self._processor.process and logs CONFLICT_ESCALATION_RESOLVED; wire these helpers back into resolve so control flow and exception handling remain unchanged.src/synthorg/communication/conflict_resolution/escalation/processors.py (1)
62-62: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffClock seam pattern not applied for testability.
Per coding guidelines, functions should declare
clock: Clock | None = Noneparameter for deterministic time control in tests.datetime.now(UTC)is called directly in:
- Line 62 (
_build_dissent_records_from_resolution)- Line 205 (
_build_winner_resolution)- Line 231 (
_build_reject_resolution)This makes it difficult to write deterministic tests for timestamp-sensitive logic.
Also applies to: 205-205, 231-231
🤖 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/communication/conflict_resolution/escalation/processors.py` at line 62, The three functions _build_dissent_records_from_resolution, _build_winner_resolution, and _build_reject_resolution call datetime.now(UTC) directly; change each to accept an optional clock: Clock | None = None parameter and use clock.now() (or clock.now(UTC) depending on your Clock API) to produce timestamps when clock is provided, otherwise fall back to datetime.now(UTC); update call sites to thread an existing clock where available (or omit to preserve current behavior) and adjust imports/typing to reference Clock so tests can inject a deterministic clock.src/synthorg/settings/dispatcher.py (1)
141-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up the stale bus subscription before crash recovery re-subscribes.
When this branch collapses a finished poll task, the old
_SUBSCRIBER_IDregistration on#settingsis still live because onlystop()and the spawn-rollback path callunsubscribe(). The subsequentstart()then callssubscribe()again against that stale registration. The stop-path comments already note that duplicate subscribe is not guaranteed to be idempotent on every bus implementation, so recovery after the first unexpected task exit can either fail or double-deliver settings changes.🤖 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/settings/dispatcher.py` around lines 141 - 168, Stale bus subscription left when a finished _task is detected: after the block that checks "if self._task is not None and self._task.done()" call the bus unsubscribe to clean up the old _SUBSCRIBER_ID registration (use await self._bus.unsubscribe(_SETTINGS_CHANNEL, _SUBSCRIBER_ID) or the equivalent async method on _bus) before clearing _task/_running, and wrap that unsubscribe in a try/except that logs any error via the existing logger so failure to unsubscribe doesn't crash start(); this ensures start()/subscribe() won't double-subscribe and that stop() semantics remain correct.
♻️ Duplicate comments (3)
docs/reference/protocols-audit.md (3)
384-384:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix malformed inline code span in the follow-up issue title.
The nested/double-backtick formatting is invalid and triggers markdown linting. Use one valid inline code span around
_PrivatePrefixedinside normal sentence text.Suggested minimal fix
-- ``[CLEANUP] Protocols audit follow-up: REVIEW pass for `_PrivatePrefixed` typing seams`` +- `[CLEANUP] Protocols audit follow-up: REVIEW pass for _PrivatePrefixed typing seams`🤖 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/protocols-audit.md` at line 384, The issue is a malformed inline code span in the follow-up issue title; replace the nested/double-backtick usage in the title string "[CLEANUP] Protocols audit follow-up: REVIEW pass for `_PrivatePrefixed` typing seams" with a single inline code span so it reads e.g. "[CLEANUP] Protocols audit follow-up: REVIEW pass for `_PrivatePrefixed` typing seams" (use one backtick pair around _PrivatePrefixed) to satisfy markdown linting.
38-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hard-coded public numerics with runtime-stats markers.
These values are still hard-coded in a public doc and should be sourced via
<!--RS:NAME-->markers (for totals, category counts, and percentage text) so the numbers stay authoritative and gateable.As per coding guidelines: “Numeric claims in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers; see data/README.md”.🤖 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/protocols-audit.md` around lines 38 - 45, Replace the hard-coded numeric literals (e.g., 237, 193, 0, 31, 13 and the "209 of 237 (88%)" summary) in the table and summary text with runtime-stats markers like <!--RS:TOTAL--> for the grand total, <!--RS:CATEGORY_KEEP-->, <!--RS:CATEGORY_RUNTIME_CHECKABLE-->, <!--RS:CATEGORY_MAKE_RUNTIME_CHECKABLE-->, <!--RS:CATEGORY_REMOVE-->, <!--RS:CATEGORY_REVIEW--> for the category counts and a percentage marker such as <!--RS:PCT_RUNTIME_CHECKABLE--> for the percent text; source these names from data/runtime_stats.yaml and follow the marker conventions in data/README.md so the document reads the authoritative values at build time.
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake audit generation reproducible from committed/pinned inputs.
The doc still points to a gitignored generator path. Please either commit the generator or document an exact pinned regeneration recipe (commit SHA + environment/tool versions + command) so reviewers can reproduce this baseline reliably.
🤖 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/protocols-audit.md` at line 9, The docs file protocols-audit.md references an uncommitted generator protocol_audit.py in the _audit working area; fix by either committing that generator script into the repo (so the table can be regenerated) or by updating protocols-audit.md to include a precise, pinned regeneration recipe: the exact commit SHA to run against, the required environment and tool versions (Python version, any packages and their versions or a lockfile), and the exact command to run to reproduce the table; ensure the recipe text explicitly names protocol_audit.py and the repo root invocation so reviewers can reproduce the audit deterministically.
🤖 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/communication/conflict_resolution/escalation/processors.py`:
- Around line 217-223: The _build_reject_resolution function currently types its
decision parameter as EscalationDecision but is only invoked with a
RejectDecision; change the signature of _build_reject_resolution to accept
decision: RejectDecision (and import or reference RejectDecision if necessary)
so the parameter is narrowed, update any type hints or docstrings to reflect
RejectDecision, and verify callers (e.g., the place that previously guards/casts
to RejectDecision before calling _build_reject_resolution) still pass a
RejectDecision instance so type checking succeeds; keep the return type as
ConflictResolution.
---
Outside diff comments:
In `@src/synthorg/communication/conflict_resolution/escalation/processors.py`:
- Line 62: The three functions _build_dissent_records_from_resolution,
_build_winner_resolution, and _build_reject_resolution call datetime.now(UTC)
directly; change each to accept an optional clock: Clock | None = None parameter
and use clock.now() (or clock.now(UTC) depending on your Clock API) to produce
timestamps when clock is provided, otherwise fall back to datetime.now(UTC);
update call sites to thread an existing clock where available (or omit to
preserve current behavior) and adjust imports/typing to reference Clock so tests
can inject a deterministic clock.
In `@src/synthorg/communication/conflict_resolution/human_strategy.py`:
- Line 72: The constant _NOTIFICATION_DISPATCH_TIMEOUT_SECONDS defined in
human_strategy.py should be moved into the project settings definitions so
timeouts aren’t hardcoded; add a new definition (e.g.,
NOTIFICATION_DISPATCH_TIMEOUT_SECONDS) in settings/definitions and replace
usages of the _NOTIFICATION_DISPATCH_TIMEOUT_SECONDS symbol in human_strategy.py
with a settings lookup (import the settings/config accessor used elsewhere in
the codebase), preserving the current default value of 10.0 if unset; update any
imports and consumers of human_strategy.py accordingly and adjust tests or docs
that reference this value.
- Around line 146-252: The resolve method is too long (>50 lines); split its
logical phases into small private async helpers to keep behavior identical:
extract the store-registration-and-persist block (the call to
self._registry.register, await self._store.create, the exception logging and
self._registry.cancel) into a helper like _persist_escalation(escalation,
conflict) that returns the registered future; extract the notifier scheduling
block (asyncio.create_task, self._notify_tasks management, add_done_callback)
into _schedule_notification(escalation, conflict) that returns the notify_task;
extract the wait/timeout/cancel/late-decision logic (awaiting future or
asyncio.wait_for, catching TimeoutError and calling _read_late_decision and
_handle_timeout_cleanup, catching CancelledError and calling
_handle_cancelled_cleanup) into _await_decision(future, escalation, conflict)
which returns either a Decision or signals timeout/cancel resolution; and
extract the final resolve/process logging into _finalize_resolution(conflict,
escalation, decision) which calls _resolve_decided_by and
self._processor.process and logs CONFLICT_ESCALATION_RESOLVED; wire these
helpers back into resolve so control flow and exception handling remain
unchanged.
In `@src/synthorg/settings/dispatcher.py`:
- Around line 141-168: Stale bus subscription left when a finished _task is
detected: after the block that checks "if self._task is not None and
self._task.done()" call the bus unsubscribe to clean up the old _SUBSCRIBER_ID
registration (use await self._bus.unsubscribe(_SETTINGS_CHANNEL, _SUBSCRIBER_ID)
or the equivalent async method on _bus) before clearing _task/_running, and wrap
that unsubscribe in a try/except that logs any error via the existing logger so
failure to unsubscribe doesn't crash start(); this ensures start()/subscribe()
won't double-subscribe and that stop() semantics remain correct.
---
Duplicate comments:
In `@docs/reference/protocols-audit.md`:
- Line 384: The issue is a malformed inline code span in the follow-up issue
title; replace the nested/double-backtick usage in the title string "[CLEANUP]
Protocols audit follow-up: REVIEW pass for `_PrivatePrefixed` typing seams" with
a single inline code span so it reads e.g. "[CLEANUP] Protocols audit follow-up:
REVIEW pass for `_PrivatePrefixed` typing seams" (use one backtick pair around
_PrivatePrefixed) to satisfy markdown linting.
- Around line 38-45: Replace the hard-coded numeric literals (e.g., 237, 193, 0,
31, 13 and the "209 of 237 (88%)" summary) in the table and summary text with
runtime-stats markers like <!--RS:TOTAL--> for the grand total,
<!--RS:CATEGORY_KEEP-->, <!--RS:CATEGORY_RUNTIME_CHECKABLE-->,
<!--RS:CATEGORY_MAKE_RUNTIME_CHECKABLE-->, <!--RS:CATEGORY_REMOVE-->,
<!--RS:CATEGORY_REVIEW--> for the category counts and a percentage marker such
as <!--RS:PCT_RUNTIME_CHECKABLE--> for the percent text; source these names from
data/runtime_stats.yaml and follow the marker conventions in data/README.md so
the document reads the authoritative values at build time.
- Line 9: The docs file protocols-audit.md references an uncommitted generator
protocol_audit.py in the _audit working area; fix by either committing that
generator script into the repo (so the table can be regenerated) or by updating
protocols-audit.md to include a precise, pinned regeneration recipe: the exact
commit SHA to run against, the required environment and tool versions (Python
version, any packages and their versions or a lockfile), and the exact command
to run to reproduce the table; ensure the recipe text explicitly names
protocol_audit.py and the repo root invocation so reviewers can reproduce the
audit deterministically.
🪄 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: 319a7f77-7dee-406d-88b1-d9430ffdcae4
📒 Files selected for processing (5)
docs/reference/protocols-audit.mdscripts/long_running_loops_kill_switch_baseline.txtsrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/settings/dispatcher.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). (8)
- GitHub Check: CodSpeed Web Pass
- GitHub Check: Build Backend
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration precedence: DB > env > YAML > code default via
SettingsService/ConfigResolver; noos.environ.getoutside startupServices own
_lifecycle_lock; timed-out stops must mark service unrestartable
Files:
src/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
No hardcoded numeric values; numerics must live in
settings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2; enforced byscripts/check_no_magic_numbers.pyComments must explain WHY only; no reviewer citations, issue back-references, or migration framing; enforced by
check_no_review_origin_in_code.pyandcheck_no_migration_framing.pyNo
from __future__ import annotations(Python 3.14+ has PEP 649); use PEP 758 except:except A, B:requires parens when bindingType hints required on public functions; mypy strict mode; use Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Define errors as
<Domain><Condition>Errorinheriting fromDomainError; never inheritException/RuntimeErrordirectly; enforced bycheck_domain_error_hierarchy.pyUse Pydantic v2 frozen models with
extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes); use@computed_fieldfor derived properties; useNotBlankStrfor identifiersUse args models at every system boundary; use
parse_typed()for every external dict ingestion; enforced bycheck_boundary_typed.pyImmutability: use
model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundariesFor async code: use
asyncio.TaskGroupfor fan-out/fan-in; helpers must catchExceptionand re-raiseMemoryError/RecursionErrorClock seam pattern: declare
clock: Clock | None = Noneparameter; injectFakeClockin tests for deterministic time controlUntrusted content handling (SEC-1): use
wrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML contentUse
from synthorg.observability import get_logger; variable must be namedlogger; neverimport loggingorprint()in app codeUse event names from
observability.events.<domain>constants; use structured logging with kwargs (logger.info(EVENT, key=value))Error logging: log WARNING/ERROR with context before raising; state tra...
Files:
src/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.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/communication/conflict_resolution/human_strategy.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Numeric claims in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers; see data/README.md
Files:
docs/reference/protocols-audit.md
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use D2 for architecture/nested container diagrams (theme 200, Dark Mauve, CLI pinned v0.7.1); use Mermaid for flowcharts/sequence/pipelines; use Markdown tables for tabular data
Files:
docs/reference/protocols-audit.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Read design specification in `docs/design/` before implementing; deviations require approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: No region/currency/locale privileged; use metric units; use British English
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Every convention PR must ship its enforcement gate; detail in convention-gates.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Test regression detection: timeout/slow failures indicate source-code regression; never edit `tests/baselines/unit_timing.json`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: After issue implementation: branch + commit + push (no auto-PR); use `/pre-pr-review`; after PR: use `/aurelio-review-pr` for external feedback; fix all valid issues; no deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Use `uv sync` for all dependencies; `uv sync --group docs` for documentation toolchain (zensical + D2)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: xdist parallelization: use `-n 8 --dist=loadfile` (auto-applied via pyproject `addopts`; `loadfile` prevents Python 3.14+ Windows ProactorEventLoop leak)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Git commits: use format `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Branch naming: use format `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Pre-commit/pre-push hooks configured in `.pre-commit-config.yaml`; hookify rules in `.claude/hookify.*.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Squash merge PRs; PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: After squash merge: run `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Web: see `web/CLAUDE.md` for detailed guidelines; CLI: see `cli/CLAUDE.md` (use `go -C cli`, never `cd cli`); Shell: see `~/.claude/rules/common/bash.md` for `cd` / `git -C` / Bash file-write rules
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: D2 CLI pinned to v0.7.1 in CI
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T11:32:28.607Z
Learning: Loading reference pages on demand from docs/reference/ (claude-reference.md, conventions.md, convention-gates.md, regional-defaults.md, persistence-boundary.md, configuration-precedence.md, errors.md, sec-prompt-safety.md, lifecycle-sync.md, mcp-handler-contract.md, typed-boundaries.md, retry-patterns.md, scaffolding.md, audit-category-gate-coverage.md, dead-api-endpoints.md, pluggable-subsystems.md, protocols-audit.md, telemetry.md)
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
src/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
🔇 Additional comments (5)
src/synthorg/communication/conflict_resolution/human_strategy.py (3)
133-137: Past review feedback addressed: processor fallback now uses explicitNonecheck.The previous review correctly identified that truthiness (
processor or ...) could override a valid processor with a falsy__bool__. This is now fixed with the explicitprocessor is not Nonecheck.
160-161: PEP 758 except syntax correctly applied.The unparenthesized
except MemoryError, RecursionError:clauses throughout this file follow PEP 758 syntax for Python 3.14+. This is the correct project convention for system-error propagation. Based on learnings: "This repository targets Python 3.14+ and follows PEP 758."Also applies to: 287-288, 315-316, 330-331, 360-361, 378-379, 414-415, 434-435, 471-472
445-445: ⚡ Quick winRemove this comment—no type constraint issue exists.
The plain string
"human"satisfies bothNotBlankStrconstraints (length 5 ≥ 1; non-whitespace). Python type hints are not enforced at runtime, and theprocessmethod does not perform explicit validation ofdecided_by. Type checking tools will flag the type mismatch (strvsNotBlankStr), but there is no runtime risk here.> Likely an incorrect or invalid review comment.src/synthorg/communication/conflict_resolution/escalation/processors.py (2)
69-167: LGTM!The
HumanDecisionProcessorclass consolidates the previousWinnerSelectProcessorandHybridDecisionProcessorinto a single mode-discriminated implementation. The design is clean:
- Mode validation at construction prevents invalid states
assert_neverensures exhaustiveness for futureEscalationDecisionvariants- The
ValueErrorfor reject decisions in winner mode enables proper 422 mapping at the REST layer- Structured logging with appropriate context is preserved
21-22:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMove
NotBlankStrimport outsideTYPE_CHECKINGblock.
NotBlankStris imported underTYPE_CHECKING(lines 21-22) but referenced at runtime in function signatures at lines 109, 174, and 222. Withoutfrom __future__ import annotations, these type hints are evaluated eagerly, causing aNameErrorwhen the module is imported.Fix
-if TYPE_CHECKING: - from synthorg.core.types import NotBlankStr +from synthorg.core.types import NotBlankStrRemove
TYPE_CHECKINGfrom the line 4 import if no longer used elsewhere:-from typing import TYPE_CHECKING, Literal, assert_never +from typing import Literal, assert_never> Likely an incorrect or invalid review comment.
Six valid findings from CodeRabbit + Gemini, two skipped as invalid. Fixed: - src/synthorg/core/normalization.py: docstring 'Three' updated to 'Four' ASGI byte-header call sites (gemini) - src/synthorg/hr/registry.py: get_by_names now uses normalize_identifier so batch and single lookups share casefold + whitespace-strip semantics with get_by_name (coderabbit) - docs/reference/protocols-audit.md: escape malformed inline code span on line 384 (MD038) (coderabbit) - web/src/api/types/enums.ts: 'as const satisfies readonly string[]' on all 16 *_VALUES tuples to prevent silent type widening (coderabbit) - web/src/api/endpoints/reports.ts: validate /reports/periods payload against REPORT_PERIOD_VALUES before narrowing (coderabbit) - web/src/__tests__/pages/new-surfaces.test.tsx: drop misleading 'defined locally' comment; handlers come from global setup (coderabbit) Skipped (factually wrong): - gemini: 'NameError due to missing future annotations' on processors.py: Python 3.14+ has PEP 649 deferred eval; the project explicitly forbids 'from __future__ import annotations'. - coderabbit: 'use RS markers for numerics' on protocols-audit.md: file is intentionally not in scripts/check_doc_numeric_macros.py _SCOPED_FILES; audit numbers are a date-pinned snapshot, not live runtime stats.
Four valid CodeRabbit findings, all applied. - docs/reference/protocols-audit.md: replace 'uncommitted helper' parenthetical with a reproducible regeneration command so the audit's numeric snapshot is reproducible from committed state alone. - src/synthorg/communication/conflict_resolution/human_strategy.py: use 'is None' instead of 'or' for the processor fallback so a custom DecisionProcessor with a falsy __bool__ is preserved. - src/synthorg/communication/conflict_resolution/escalation/processors.py: detect RejectDecision explicitly and call assert_never on any other variant, so a future EscalationDecision member added to the union does not silently fall through as REJECTED_BY_HUMAN. - src/synthorg/settings/dispatcher.py: in start(), collapse a crashed-and-finished poll task under _lifecycle_lock before the _running guard, so a recovery start() does not race the queued _reset_running_under_lock and trip the 'already running' guard on a dispatcher whose previous task is already done.
Aureliolo
force-pushed
the
chore/issue-1850-cleanup
branch
from
aad9454 to
987d045
Compare
Four valid CodeRabbit findings fixed; three skipped as factually
disclaimed by CR's own configuration or by the project's gate scope.
Fixed:
- src/synthorg/communication/conflict_resolution/escalation/processors.py:
narrow _build_reject_resolution decision parameter from
EscalationDecision to RejectDecision so the type checker enforces
the contract that the prior isinstance(RejectDecision) branch
established.
- src/synthorg/settings/dispatcher.py: in start(), unsubscribe the
crashed-task bus registration before clearing _task / _running so
the subsequent subscribe() call cannot double-register on bus
implementations whose subscribe is not idempotent (NATS).
- docs/reference/protocols-audit.md: switch line 384 from a double-
backtick-wrapped form (which markdownlint MD038 still flagged) to
a plain single-backtick code span; commit the audit generator at
scripts/protocol_audit.py and update the doc header to point at
the committed path so the snapshot is reproducible from the repo
alone.
- scripts/protocol_audit.py: new committed audit generator (was
living in the gitignored _audit/ working area).
Skipped (factually disclaimed):
- coderabbit: human_strategy.py:72 timeout-to-settings move.
CR-classified Nitpick / Trivial / Low value. The constant is
tracked in scripts/no_magic_numbers_baseline.txt:177 as a
project-sanctioned pre-existing exception. Similar operational
timeouts in this same PR (_STOP_DRAIN_TIMEOUT_SECONDS = 10.0
at dispatcher.py:43, _BOOTSTRAP_MAX_CONSECUTIVE_ERRORS = 30
at dispatcher.py:40) are also baselined and not flagged by CR.
- coderabbit: human_strategy.py:146-252 resolve()-length extract.
CR's own configuration directive forbids exactly this kind of
suggestion: 'Do not suggest extracting single-use helper
functions called exactly once -- this reduces readability
without improving maintainability.' Every proposed helper would
be called exactly once.
- coderabbit: processors.py Clock-seam threading.
CR-classified Poor tradeoff. The Clock seam is applied at
service boundaries that own time-dependent flow control
(timeouts, expiry, scheduling), not at private model-construction
helpers that produce informational resolved_at /
timestamp fields. Tests for _build_*_resolution do not
verify the timestamp value.
- coderabbit: protocols-audit.md:38-45 RS-marker substitution
(duplicate from round 1). The file is intentionally not in
scripts/check_doc_numeric_macros.py _SCOPED_FILES; the audit
numbers are a date-pinned snapshot recorded by the title
('Protocols Audit (2026-05-10)'), not live runtime stats that
would otherwise rot.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
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/api/endpoints/reports.ts (1)
55-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
generateReportresponse before returning typedReportResponse.Line 62 returns unvalidated wire data as
ReportResponse. If backend drifts (e.g., newperiod), invalid values can bypass types and break downstream exhaustive logic at runtime.Proposed fix
export async function generateReport( period: ReportPeriod, ): Promise<ReportResponse> { const response = await apiClient.post<ApiResponse<ReportResponse>>( '/reports/generate', { period } satisfies GenerateReportRequest, ) - return unwrap(response) + const report = unwrap(response) + if (!isReportPeriod(report.period)) { + throw new Error( + `Unknown report period in /reports/generate response (allowed: ${REPORT_PERIOD_VALUES.join(', ')})`, + ) + } + return report }🤖 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/api/endpoints/reports.ts` around lines 55 - 63, The function generateReport currently returns unvalidated wire data as a ReportResponse; before returning, validate the API payload from apiClient.post (the ApiResponse<ReportResponse> result) against a runtime schema/decoder for ReportResponse (or an ApiResponse wrapper) and only call unwrap/return after successful validation. Update generateReport to run the response through the validator (or replace/augment unwrap) and throw or handle a descriptive error on validation failure so downstream exhaustive logic using ReportResponse cannot receive unexpected shapes.src/synthorg/api/services/_org_agent_mutations.py (1)
335-347:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReorder path mixes
compare_ciand.lower()semantics, which can break Unicode-safe matching.
dept_agentsis selected withcompare_ci, butagent_by_lower/lookup still uses.lower(). For casefold-only equivalences (e.g.StraßevsSTRASSE), this can produce a runtime lookup miss.💡 Proposed fix
-from synthorg.core.normalization import compare_ci +from synthorg.core.normalization import compare_ci, normalize_identifier @@ - agent_by_lower = {a.name.lower(): a for a in dept_agents} - reordered_dept = tuple(agent_by_lower[n.lower()] for n in data.agent_names) + agent_by_norm = { + normalize_identifier(a.name): a for a in dept_agents + } + reordered_dept = tuple( + agent_by_norm[normalize_identifier(n)] for n in data.agent_names + )🤖 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/services/_org_agent_mutations.py` around lines 335 - 347, The reorder logic mixes compare_ci (Unicode-aware) with plain .lower() causing lookup misses for casefold-only equivalents; replace the .lower() usage by building the lookup with a.name.casefold() (e.g., agent_by_fold = {a.name.casefold(): a for a in dept_agents}) and index it using n.casefold() when creating reordered_dept and any subsequent lookups, keeping compare_ci for selection and ensuring all name comparisons use casefold for Unicode-safe matching (update references to agent_by_lower and reordered_dept accordingly).tests/unit/communication/conflict_resolution/test_human_strategy.py (1)
162-183: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRename the tests to the new
winnermode terminology.These names still mention
winner_select, even though the class under test is nowHumanDecisionProcessor(mode="winner"). Keeping the old names makes failures and grep results point at a removed abstraction.♻️ Suggested update
- async def test_winner_select_rejects_reject_decision(self) -> None: + async def test_winner_mode_rejects_reject_decision(self) -> None: ... - async def test_winner_select_rejects_unknown_agent(self) -> None: + async def test_winner_mode_rejects_unknown_agent(self) -> None:🤖 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/communication/conflict_resolution/test_human_strategy.py` around lines 162 - 183, Rename the two test functions to remove the outdated "winner_select" wording and match the current mode naming used by HumanDecisionProcessor(mode="winner"); specifically, update the test function names test_winner_select_rejects_reject_decision and test_winner_select_rejects_unknown_agent to something like test_winner_rejects_reject_decision and test_winner_rejects_unknown_agent (or another concise "winner" prefixed form) while keeping the bodies using HumanDecisionProcessor, process, RejectDecision and WinnerDecision unchanged so test discovery and grep reflect the current abstraction.
🤖 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/protocol_audit.py`:
- Line 44: The regex assigned to variable "pattern" misses class declarations
that include type-parameter brackets (e.g., VersionRepository[T:
BaseModel](Protocol)); update the pattern to accept an optional bracketed
generic section between the class name and the parentheses. Replace the current
pattern r"^class (\w+)\((?:[\w\s,\.]*\b)Protocol\b" with one that allows an
optional "\[...\]" segment before the "(" (for example by adding (?:\[[^\]]*\])?
after (\w+)), so the compiled "pattern" will match declarations like class
Name[T](Protocol): as well as non-generic protocols.
- Around line 79-92: The _count function currently swallows grep failures and
returns 0; instead define or import a ProtocolAuditError subclass of DomainError
and modify _count to raise ProtocolAuditError when subprocess.run raises
FileNotFoundError (missing grep) or when result.returncode > 1 (grep error),
leaving normal zero/positive counts only when grep succeeds (returncode 0 or 1);
reference _count, ProtocolAuditError (subclass of DomainError), and
subprocess.run to locate and implement the change.
In `@src/synthorg/communication/conflict_resolution/escalation/processors.py`:
- Around line 141-149: The structured log note values reference removed classes;
update them to neutral, post-refactor labels. Replace
"winner_select_rejected_non_winner" with "escalation_rejected_non_winner" and
"hybrid_processor_rejected" with "escalation_hybrid_rejected" in the
CONFLICT_ESCALATION_RESOLVED logger calls (the block that uses conflict.id,
decided_by, decision/type, ConflictResolutionStrategy.HUMAN.value) and the
similar logger usage later around the other occurrence (lines ~233-240).
In `@tests/unit/core/test_normalization.py`:
- Around line 108-114: The property test
test_matches_normalize_identifier_contract currently has only 3 deterministic
`@example` cases; add at least 7 more fixed `@example`(...) decorators (for a total
of 10) to meet the project's Hypothesis baseline. Edit the
test_matches_normalize_identifier_contract function and append additional
`@example` entries covering varied edge-cases (e.g., empty string,
whitespace-only, mixed case, unicode with accents, combining marks, ligatures,
numbers/punctuation, and long strings) so the test still asserts compare_ci(a,
b) == (normalize_identifier(a) == normalize_identifier(b)) using the same
function names compare_ci and normalize_identifier.
---
Outside diff comments:
In `@src/synthorg/api/services/_org_agent_mutations.py`:
- Around line 335-347: The reorder logic mixes compare_ci (Unicode-aware) with
plain .lower() causing lookup misses for casefold-only equivalents; replace the
.lower() usage by building the lookup with a.name.casefold() (e.g.,
agent_by_fold = {a.name.casefold(): a for a in dept_agents}) and index it using
n.casefold() when creating reordered_dept and any subsequent lookups, keeping
compare_ci for selection and ensuring all name comparisons use casefold for
Unicode-safe matching (update references to agent_by_lower and reordered_dept
accordingly).
In `@tests/unit/communication/conflict_resolution/test_human_strategy.py`:
- Around line 162-183: Rename the two test functions to remove the outdated
"winner_select" wording and match the current mode naming used by
HumanDecisionProcessor(mode="winner"); specifically, update the test function
names test_winner_select_rejects_reject_decision and
test_winner_select_rejects_unknown_agent to something like
test_winner_rejects_reject_decision and test_winner_rejects_unknown_agent (or
another concise "winner" prefixed form) while keeping the bodies using
HumanDecisionProcessor, process, RejectDecision and WinnerDecision unchanged so
test discovery and grep reflect the current abstraction.
In `@web/src/api/endpoints/reports.ts`:
- Around line 55-63: The function generateReport currently returns unvalidated
wire data as a ReportResponse; before returning, validate the API payload from
apiClient.post (the ApiResponse<ReportResponse> result) against a runtime
schema/decoder for ReportResponse (or an ApiResponse wrapper) and only call
unwrap/return after successful validation. Update generateReport to run the
response through the validator (or replace/augment unwrap) and throw or handle a
descriptive error on validation failure so downstream exhaustive logic using
ReportResponse cannot receive unexpected shapes.
🪄 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: 53c8f624-b14e-4cfa-b6f9-368b2241c491
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
.gitignoreCLAUDE.mddocs/reference/protocols-audit.mdscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtscripts/protocol_audit.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/etag.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/core/normalization.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/registry.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/tools/network_validator.pysrc/synthorg/tools/web/http_request.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/core/test_normalization.pytests/unit/settings/test_dispatcher.pyweb/package.jsonweb/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
💤 Files with no reviewable changes (2)
- .gitignore
- web/package.json
📜 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: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: Lighthouse Site
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{py,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Regional Defaults (MANDATORY): no region/currency/locale privileged; metric units; British English
Files:
src/synthorg/tools/network_validator.pysrc/synthorg/api/etag.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pyCLAUDE.mdsrc/synthorg/communication/meeting/participant.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/providers/drivers/litellm_driver.pytests/unit/settings/test_dispatcher.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/api/controllers/_department_health.pytests/unit/core/test_normalization.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pysrc/synthorg/api/auth/csrf.pytests/unit/communication/conflict_resolution/test_human_strategy.pyscripts/protocol_audit.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/hr/registry.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pydocs/reference/protocols-audit.mdsrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/core/normalization.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration Precedence (MANDATORY): DB > env > YAML > code default via
SettingsService/ConfigResolver; noos.environ.getoutside startupNo Hardcoded Values (MANDATORY): numerics live in
settings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2. Enforced byscripts/check_no_magic_numbers.pyComments WHY only; no reviewer citations / issue back-refs / migration framing. Enforced by
check_no_review_origin_in_code.py+check_no_migration_framing.pyNo
from __future__ import annotations(3.14 has PEP 649). PEP 758 except:except A, B:no parens unless bindingType hints on public functions; mypy strict. Google-style docstrings. Line length 88; functions <50 lines; files <800 lines
Errors:
<Domain><Condition>ErrorfromDomainError; never inheritException/RuntimeError/etc directly. Enforced bycheck_domain_error_hierarchy.pyPydantic v2 frozen +
extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes);@computed_fieldfor derived;NotBlankStrfor identifiersArgs models at every system boundary;
parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.pyImmutability:
model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundariesAsync:
asyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError)Clock seam:
clock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartableUntrusted content (SEC-1):
wrap_untrusted()fromengine.prompt_safety;HTMLParseGuardfor HTMLRepository CRUD:
save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuplesLogging:
from synthorg.observability import get_logger; variable alwayslogger. Neverimport logging/print()in app codeEvent names from
observability.events.<domain>constants; structured kwargs (`logger.info(EVE...
Files:
src/synthorg/tools/network_validator.pysrc/synthorg/api/etag.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/providers/drivers/litellm_driver.pytests/unit/settings/test_dispatcher.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/api/controllers/_department_health.pytests/unit/core/test_normalization.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pysrc/synthorg/api/auth/csrf.pytests/unit/communication/conflict_resolution/test_human_strategy.pyscripts/protocol_audit.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/hr/registry.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/core/normalization.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/tools/network_validator.pysrc/synthorg/api/etag.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/hr/registry.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/core/normalization.py
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,js,jsx}: Always usecreateLoggerfrom@/lib/logger; never use bareconsole.warn,console.error, orconsole.debugin application code
Logger variable name must always belog(e.g.,const log = createLogger('module-name'))
Pass dynamic or untrusted values to logger methods as separate arguments (not interpolated into the message string) so they go throughsanitizeArg
Wrap attacker-controlled fields inside structured objects insanitizeForLog()before embedding them in logs
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Callers MUST NOT wrap store mutation calls in try/catch; the store owns the error UX
Any new caller of health/readiness endpoints must handle the 503 path explicitly
Any new WS payload handler that ingests untrusted strings MUST route throughsanitizeWsString()orsanitizeWsEnum()helpers
ImportErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported fromweb/src/api/types/error-codes.gen.ts); discriminate onErrorCode.<NAME>, never on raw integer literals
Never export agetXIcon(value): LucideIconfactory that returns a component reference called inside JSX render bodies; export a<XIcon value={...} {...lucideProps} />wrapper component instead usingcreateElementfor the lookup
Detectfetch()in effects withoutAbortControllercleanup using the@eslint-react/web-api-no-leaked-fetchrule
Catch the{count && <Foo />}bug using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...)
Restrictwindow,document,localStorage, and other globals inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handler,useEffect, oruseSyncExternalStore-backed hook
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Test setup must boot with
onUnhandledRequest: 'error'; tests override per-case viaserver.use(...), nevervi.mock('@/api/endpoints/*')
Files:
web/src/__tests__/pages/new-surfaces.test.tsx
{web,cli}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Web Dashboard: reuse
web/src/components/ui/; design tokens only. CLI: seecli/CLAUDE.md(usego -C cli, nevercd cli)
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Web: see
web/CLAUDE.md. React 19 framework
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Read
docs/design/page before implementing; deviations need approval. See DESIGN_SPEC.mdDoc Numeric Claims (MANDATORY): numerics in README + public docs sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markersMarkdown tables for tabular data
Files:
CLAUDE.mddocs/reference/protocols-audit.md
web/src/{api,stores}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Display counts come from
data.length; the wire envelope no longer carriestotalfield for paginated responses
Files:
web/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/api/endpoints/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state)
Files:
web/src/api/endpoints/reports.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers:
@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% minxdist
-n 8 --dist=loadfileauto-applied via pyprojectaddopts(loadfileprevents 3.14+Windows ProactorEventLoop leak)Windows: unit tests use
WindowsSelectorEventLoopPolicy(3.14 IOCP teardown race). Subprocess tests override backMock-spec: every Mock declares
spec=ConcreteClass; baseline atscripts/mock_spec_baseline.txtFakeClock from
tests._shared.fake_clock; inject viaclock=Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...))Flaky: NEVER skip/xfail; fix fundamentally. Use
asyncio.Event().wait()notsleep(large)
Files:
tests/unit/settings/test_dispatcher.pytests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.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_dispatcher.pytests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: Convention Rollout (MANDATORY): every convention PR ships its enforcement gate
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: Post-Implementation + Pre-PR Review (MANDATORY): after issue: branch + commit + push (no auto-PR); use `/pre-pr-review`; after PR: `/aurelio-review-pr` for external feedback. Fix EVERYTHING valid; no deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: Git commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: Squash merge. PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: GitHub queries: `gh issue list` via Bash, NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:03:26.181Z
Learning: After every squash merge → `/post-merge-cleanup`
📚 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/tools/network_validator.pysrc/synthorg/api/etag.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/api/auth/service.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/providers/drivers/litellm_driver.pytests/unit/settings/test_dispatcher.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/api/controllers/_department_health.pytests/unit/core/test_normalization.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/settings/subscribers/backup_subscriber.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pysrc/synthorg/api/auth/csrf.pytests/unit/communication/conflict_resolution/test_human_strategy.pyscripts/protocol_audit.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/hr/registry.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/core/normalization.py
🔇 Additional comments (42)
web/src/__tests__/pages/new-surfaces.test.tsx (1)
2-6: Comment update is accurate and resolves prior ambiguity.The revised header now correctly states handlers come from global test setup and avoids implying local handler definitions in this file.
web/src/api/endpoints/reports.ts (1)
4-53: Good runtime narrowing strategy for period lists.The
REPORT_PERIOD_VALUES+isReportPeriodvalidation in Line 4-11 and Line 42-53 is a solid safeguard against untrusted enum drift.web/src/api/types/enums.ts (1)
10-102: Enum tuple/type lockstep is clean and consistent.The
as const satisfies readonly string[]+ derived union pattern is applied uniformly and improves maintainability.src/synthorg/core/normalization.py (1)
47-73: Good centralisation of case-insensitive equality.
compare_ciis clear, correctly scoped tostr, and reusesnormalize_identifierconsistently.src/synthorg/api/etag.py (1)
216-217: Clarifying comments are useful and accurate.The added notes correctly document why bytes-level header matching stays inline at the ASGI boundary.
Also applies to: 361-362
src/synthorg/tools/web/http_request.py (1)
284-286: Host-header normalisation change looks correct.Using
compare_cihere keeps case-insensitive dedup consistent with the shared helper.src/synthorg/providers/drivers/litellm_driver.py (1)
724-725: Retry-After header matching update is solid.The switch to
compare_cikeeps HTTP header matching consistent and robust.src/synthorg/tools/network_validator.py (1)
411-411: Scheme comparison refactor is good.Using
compare_cihere aligns URL scheme checks with the new normalisation standard.src/synthorg/tools/html_parse_guard.py (1)
339-340:aria-hiddencomparison update is correct.Nice consistency improvement by reusing
compare_cifor this string comparison path.src/synthorg/api/auth/csrf.py (1)
179-181: Comment clarifications are appropriate.The rationale for keeping bytes-based ASGI header comparisons inline is clear and well-placed.
Also applies to: 207-209
src/synthorg/api/controllers/_department_health.py (1)
107-107: Department matching refactor looks good.Using
compare_cihere cleanly standardises the case-insensitive comparison.src/synthorg/api/controllers/settings.py (1)
37-37: Case-insensitive setting parsing is correctly centralised.This migration to
compare_ciforenable_correlationis clean and keeps parsing consistent with shared normalization rules.Also applies to: 623-623
src/synthorg/api/auth/middleware.py (1)
27-27: Bearer scheme matching update looks correct.
compare_cikeeps scheme matching case-insensitive while preserving existing token extraction semantics.Also applies to: 194-194
src/synthorg/engine/hybrid_helpers.py (1)
13-13: Replan string parsing change is solid.Using
compare_cihere is a good consistency upgrade for"true"handling in the JSON-string fallback path.Also applies to: 392-392
src/synthorg/engine/routing/scorer.py (1)
15-15: Role-match normalization is implemented correctly.This keeps
_score_rolebehaviour intact while centralizing case-insensitive comparison logic.Also applies to: 328-329
src/synthorg/settings/subscribers/backup_subscriber.py (1)
5-5: Backup enablement parsing update is correct.The new
compare_ciusage preserves behaviour and aligns this subscriber with shared normalization conventions.Also applies to: 114-114
src/synthorg/integrations/webhooks/verifiers/generic_hmac.py (1)
6-6: Header lookup normalization change is good.This keeps signature verification logic intact while making header-key matching consistently case-insensitive.
Also applies to: 53-53
scripts/long_running_loops_kill_switch_baseline.txt (1)
31-31: Baseline entry update looks consistent.The line-number refresh for
SettingsChangeDispatcher:_poll_loopis aligned with expected refactor drift.src/synthorg/communication/meeting/participant.py (1)
10-10: Participant special-token matching is correctly refactored.Using
compare_cifor"all"keeps resolution behaviour consistent with the new shared normalization approach.Also applies to: 250-250
src/synthorg/integrations/webhooks/verifiers/github_hmac.py (1)
6-6: Good centralization of header-name matching.Using
compare_cihere keeps case-insensitive matching consistent with the new shared normalization path without changing verifier semantics.Also applies to: 26-27
scripts/loop_bound_init_baseline.txt (1)
34-34: Baseline offset update looks consistent.This appears to be a straightforward line-number sync after source shifts.
src/synthorg/hr/training/source_selectors/department_diversity.py (1)
64-70: Nice improvement in consistency and observability.The structured
HR_TRAINING_SELECTOR_CONFIG_INVALIDlog andcompare_ci-based role partitioning both align well with the normalization/telemetry cleanup goals.Also applies to: 123-124
src/synthorg/api/services/_org_department_mutations.py (1)
189-190: Case-insensitive matching updates look solid.These replacements remove ad-hoc normalization and keep department/agent attachment checks behaviorally aligned.
Also applies to: 250-251, 290-290
src/synthorg/api/auth/service.py (1)
55-94: Docstring upgrade is clear and useful.This gives strong operational context for async boundaries and ownership without changing execution semantics.
src/synthorg/hr/training/source_selectors/role_top_performers.py (1)
55-61: Good consistency improvements here.The structured config-invalid event and
compare_cicandidate filtering are both clean upgrades.Also applies to: 90-90
src/synthorg/hr/registry.py (4)
18-22: LGTM!Clean import of the shared normalization helpers (
compare_ci,find_by_name_ci,normalize_identifier) aligns with the PR's objective to centralise case-insensitive comparison logic.
208-209: LGTM!Delegating to
find_by_name_ciconsolidates the case-insensitive lookup logic and ensures consistent Unicode-safe casefold + whitespace-stripping behaviour.
249-260: LGTM!The batch lookup now uses
normalize_identifierfor both key building and resolution, ensuring consistent semantics withget_by_name(which routes throughfind_by_name_ciwith the same normalisation). The past review concern about mismatched matching semantics has been addressed.
285-290: LGTM!Using
compare_cifor department filtering is consistent with the PR's migration pattern and ensures Unicode-safe case-insensitive comparison.src/synthorg/api/services/org_mutations.py (3)
25-25: LGTM!Import of
find_by_name_cisupports the migration from inline lowercasing to the shared case-insensitive helper.
182-188: LGTM!
_find_departmentnow cleanly delegates tofind_by_name_ci, eliminating the inline loop and ensuring consistent Unicode-safe normalisation.
208-214: LGTM!
_find_agentmirrors the_find_departmentrefactor, maintaining consistency across both lookup helpers.src/synthorg/settings/dispatcher.py (5)
14-14: LGTM!Import of
compare_cienables Unicode-safe case-insensitive parsing of therestart_requiredmetadata field.
106-111: LGTM!Storing strong references to fire-and-forget tasks in
_post_done_tasksprevents premature GC (RUF006). The comment clearly explains the rationale.
134-165: LGTM!The recovery logic in
start()correctly handles a crashed-and-finished poll task by:
- Detecting
self._task.done()under the lifecycle lock- Unsubscribing the stale bus registration (preventing double-subscribe on non-idempotent buses)
- Clearing
_runningand_taskso the subsequent start can proceedThis addresses the past review concern about the transient "already running" state.
339-398: LGTM!The refactored
_on_task_doneand new_reset_running_under_lockhelper correctly:
- Log immediately (synchronous callback) for observability
- Schedule the locked state reset via
create_task(cannotawaitfrom sync callback)- Use task identity check (
self._task is task) to avoid racing concurrentstart()calls- Register/discard from
_post_done_tasksto maintain strong refs only while needed
518-523: LGTM!Using
compare_ci(str(restart_raw), "false")for therestart_requiredcheck ensures Unicode-safe case-insensitive comparison while maintaining the fail-safe default (anything other than case-insensitive "false" is treated as restart-required).tests/unit/settings/test_dispatcher.py (2)
529-535: LGTM!The test correctly awaits
_post_done_tasksbefore asserting_running is False, ensuring the assertion observes the post-lock state rather than the in-flight intermediate. The comment clearly explains the necessity.
641-643: LGTM!Consistent application of the
_post_done_taskswait pattern for the consecutive-errors termination test.scripts/no_magic_numbers_baseline.txt (1)
1-833: LGTM!Baseline coordinates updated to reflect line shifts from the normalization helper migrations. The suppressions remain at the correct locations in the refactored source files.
docs/reference/protocols-audit.md (1)
36-45: Summary numerics should still be marker-sourced.The public-doc numeric claims in this summary remain hard-coded and should be rendered from runtime stats markers.
As per coding guidelines: “Doc Numeric Claims (MANDATORY): numerics in README + public docs sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers”.CLAUDE.md (1)
46-46: Reference index update looks good.Adding
docs/reference/protocols-audit.mdto the on-demand references is consistent with this PR’s documentation scope.
| def _count(pattern: str, root: Path) -> int: | ||
| """Count matches via system grep -rE; return 0 on no matches.""" | ||
| try: | ||
| result = subprocess.run( | ||
| ["grep", "-rE", "--include=*.py", pattern, str(root)], | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| except FileNotFoundError: | ||
| return 0 | ||
| if result.returncode > 1: | ||
| return 0 | ||
| return sum(1 for line in result.stdout.splitlines() if line.strip()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current error-handling behaviour in _count.
nl -ba scripts/protocol_audit.py | sed -n '79,92p'Repository: Aureliolo/synthorg
Length of output: 103
🏁 Script executed:
cat -n scripts/protocol_audit.py | sed -n '75,95p'Repository: Aureliolo/synthorg
Length of output: 762
🏁 Script executed:
head -100 scripts/protocol_audit.pyRepository: Aureliolo/synthorg
Length of output: 3187
🏁 Script executed:
grep -n "_count" scripts/protocol_audit.pyRepository: Aureliolo/synthorg
Length of output: 517
🏁 Script executed:
find . -type f -name "*.py" -exec grep -l "class DomainError" {} \;Repository: Aureliolo/synthorg
Length of output: 157
🏁 Script executed:
grep -r "DomainError" --include="*.py" | head -20Repository: Aureliolo/synthorg
Length of output: 1998
🏁 Script executed:
grep -h "raise\|Error" scripts/protocol_audit.pyRepository: Aureliolo/synthorg
Length of output: 127
Fail fast on grep errors instead of returning zero counts; use proper error hierarchy.
Returning 0 on grep execution failures silently misclassifies protocols as unused and taints the audit table. Raise on command errors and missing binary so bad audits fail loudly. However, the proposed fix must use an appropriate DomainError subclass (e.g., ProtocolAuditError) per the mandatory error hierarchy constraint—never RuntimeError.
🔧 Corrected approach
Define a ProtocolAuditError class inheriting from DomainError in the same file or import an existing audit-related error class. Then update _count() to raise it instead of returning 0.
🤖 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 `@scripts/protocol_audit.py` around lines 79 - 92, The _count function
currently swallows grep failures and returns 0; instead define or import a
ProtocolAuditError subclass of DomainError and modify _count to raise
ProtocolAuditError when subprocess.run raises FileNotFoundError (missing grep)
or when result.returncode > 1 (grep error), leaving normal zero/positive counts
only when grep succeeds (returncode 0 or 1); reference _count,
ProtocolAuditError (subclass of DomainError), and subprocess.run to locate and
implement the change.
Seven valid CodeRabbit findings, all applied. - scripts/protocol_audit.py: regex now accepts type-parameterised protocol declarations (class Foo[T: Bar](Protocol):); _count raises RuntimeError on missing grep or non-zero grep exit instead of silently returning 0 (which would taint the audit table). - src/synthorg/communication/conflict_resolution/escalation/processors.py: rename structured-log notes from the legacy class names (winner_select_rejected_non_winner, hybrid_processor_rejected) to post-refactor mode-based labels (winner_mode_rejected_reject_decision, hybrid_mode_rejected_decision). - src/synthorg/api/services/_org_agent_mutations.py: switch reorder_dept lookup from .lower() to normalize_identifier so the selection (compare_ci) and the bucketing share Unicode-safe casefold semantics; the mixed pattern produced lookup misses for casefold-only equivalents (Strasse vs STRASSE) the same way the hr/registry.py site did before round 1. - web/src/api/endpoints/reports.ts: validate generateReport response.period through isReportPeriod before returning, mirroring listReportPeriods round-1 hardening. - tests/unit/core/test_normalization.py: bump deterministic Hypothesis examples to ten, covering empty / ASCII / German sharp-s / Greek sigma / whitespace / Latin combining accents / digits / single-char inequality / long strings. - tests/unit/communication/conflict_resolution/test_human_strategy.py: rename test_winner_select_* methods to test_winner_mode_* so test names track HumanDecisionProcessor(mode='winner') rather than the removed WinnerSelectProcessor.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/protocol_audit.py`:
- Around line 81-129: _count currently counts matching lines from grep output
which undercounts when multiple matches occur on the same line; update the grep
invocation inside _count to include the -o flag so grep emits one match per line
(preserving --include and -rE) and thus the final sum of
result.stdout.splitlines() in _count correctly reflects occurrences used by
callers like _typeuse_count and _testuse_count; keep error handling
(FileNotFoundError and non-zero returncode >1) unchanged.
🪄 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: d06fb77e-09af-49e1-bf9d-46eb11ebf9ed
📒 Files selected for processing (6)
scripts/protocol_audit.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/core/test_normalization.pyweb/src/api/endpoints/reports.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). (7)
- GitHub Check: Build Backend
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Comments explain WHY only; no reviewer citations, issue back-refs, or migration framing
No
from __future__ import annotations(Python 3.14 has PEP 649). PEP 758 except:except A, B:no parens unless bindingType hints on public functions; mypy strict mode. Use Google-style docstrings
Line length 88; functions <50 lines; files <800 lines
Error classes:
<Domain><Condition>Errornaming fromDomainError; never inheritException/RuntimeErrordirectlyPydantic v2 frozen +
extra="forbid"on API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes); use@computed_fieldfor derived;NotBlankStrfor identifiersUse args models at every system boundary; call
parse_typed()for every external dict ingestionImmutability: use
model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundariesAsync: use
asyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/RecursionError)Clock seam: declare
clock: Clock | None = None; tests injectFakeClock. Lifecycle: services own_lifecycle_lock; timed-out stops mark unrestartableUntrusted content (SEC-1): use
wrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML parsingRepository CRUD methods:
save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples
Files:
scripts/protocol_audit.pysrc/synthorg/api/services/_org_agent_mutations.pytests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration precedence: DB > env > YAML > code default via
SettingsService/ConfigResolver; noos.environ.getoutside startupNumerics live in
settings/definitions/; allowlist 0/1/-1, HTTP codes, hex masks, powers-of-2Use
from synthorg.observability import get_logger; variable always namedlogger. Neverimport loggingor useprint()in app codeEvent names from
observability.events.<domain>constants; use structured kwargs (logger.info(EVENT, key=value))Error paths log WARNING/ERROR with context before raising; state transitions log INFO via
*_STATUS_TRANSITIONEDAFTER persistence writeSecret-log redaction (SEC-1): never log
error=str(exc)or interpolate exceptions; useerror_type=type(exc).__name__+error=safe_error_description(exc). Neverexc_info=True. OTel: forbidspan.record_exception(exc); usespan.set_attribute("exception.message", ...)+record_exception=False, set_status_on_exception=FalseTelemetry: opt-in, off by default. Every event property must be in
_ALLOWED_PROPERTIESVendor-agnostic naming: NEVER use real vendor names in project code/tests. Use
example-provider,test-provider,example-{large,medium,small}-001. Allowed in.claude/, third-party imports,providers/presets.py,web/public/provider-logos/
Files:
src/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/communication/conflict_resolution/escalation/processors.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/api/services/_org_agent_mutations.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,js,jsx}: Always usecreateLoggerfrom@/lib/logger; never use bareconsole.warn,console.error, orconsole.debugin application code
Logger variable name must always belog(e.g.,const log = createLogger('module-name'))
Pass dynamic or untrusted values to logger methods as separate arguments (not interpolated into the message string) so they go throughsanitizeArg
Wrap attacker-controlled fields inside structured objects insanitizeForLog()before embedding them in logs
Files:
web/src/api/endpoints/reports.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Callers MUST NOT wrap store mutation calls in try/catch; the store owns the error UX
Any new caller of health/readiness endpoints must handle the 503 path explicitly
Any new WS payload handler that ingests untrusted strings MUST route throughsanitizeWsString()orsanitizeWsEnum()helpers
ImportErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported fromweb/src/api/types/error-codes.gen.ts); discriminate onErrorCode.<NAME>, never on raw integer literals
Never export agetXIcon(value): LucideIconfactory that returns a component reference called inside JSX render bodies; export a<XIcon value={...} {...lucideProps} />wrapper component instead usingcreateElementfor the lookup
Detectfetch()in effects withoutAbortControllercleanup using the@eslint-react/web-api-no-leaked-fetchrule
Catch the{count && <Foo />}bug using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...)
Restrictwindow,document,localStorage, and other globals inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handler,useEffect, oruseSyncExternalStore-backed hookReuse
web/src/components/ui/components and use design tokens only in React web dashboard
Files:
web/src/api/endpoints/reports.ts
web/src/{api,stores}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Display counts come from
data.length; the wire envelope no longer carriestotalfield for paginated responses
Files:
web/src/api/endpoints/reports.ts
web/src/api/endpoints/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state)
Files:
web/src/api/endpoints/reports.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers:
@pytest.mark.{unit,integration,e2e,slow}. Asyncauto. Timeout 30s global. Coverage 80% minWindows test policy: unit tests use
WindowsSelectorEventLoopPolicy(3.14 IOCP teardown race); subprocess tests override backMock-spec: every Mock must declare
spec=ConcreteClass; baseline atscripts/mock_spec_baseline.txtFakeClock from
tests._shared.fake_clock; inject viaclock=parameterHypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...)).Flaky tests: NEVER skip/xfail; fix fundamentally. Use
asyncio.Event().wait()notsleep(large)
Files:
tests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.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/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Read design spec from `docs/design/` page before implementing; deviations require approval
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: No region/currency/locale privileged; use metric units and British English
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Every convention PR must ship its enforcement gate
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Timeout/slow test failures indicate source-code regression; never edit `tests/baselines/unit_timing.json`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: After issue implementation: branch + commit + push (no auto-PR); use `/pre-pr-review` before PR creation, not `gh pr create`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: After PR merge: use `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: xdist: `-n 8 --dist=loadfile` auto-applied via pyproject `addopts` (`loadfile` prevents 3.14+ Windows ProactorEventLoop leak)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Git commit message format: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Branch naming: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Use `.pre-commit-config.yaml` for pre-commit/pre-push hooks; hookify rules in `.claude/hookify.*.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Squash merge. PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Use `gh issue list` via Bash for GitHub queries; NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Web: see `web/CLAUDE.md`. CLI: see `cli/CLAUDE.md` (use `go -C cli`, never `cd cli`). Shell: see `~/.claude/rules/common/bash.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Use Go in CLI with `go -C cli` command (never `cd cli`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Python 3.14+; BUSL-1.1 → Apache 2.0 after Change Date
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:29:19.021Z
Learning: Project layout: `src/synthorg/` (src layout), `tests/`, `web/` (React 19), `cli/` (Go binary)
📚 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/protocol_audit.pysrc/synthorg/api/services/_org_agent_mutations.pytests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
🔇 Additional comments (17)
web/src/api/endpoints/reports.ts (4)
4-11: Solid enum/value alignment with runtime narrowing.
REPORT_PERIOD_VALUES+ derivedReportPeriod+isReportPeriodkeeps compile-time and runtime constraints in sync cleanly.
13-26: Clear hand-written API contracts after generator removal.The local
ReportResponseandGenerateReportRequestinterfaces are explicit and readable, and they correctly anchorperiodtoReportPeriod.
32-53:listReportPeriodsfail-fast validation is implemented well.Validating wire
stringvalues againstREPORT_PERIOD_VALUESbefore returningReportPeriod[]is the right defensive boundary.
55-74: Good defensive re-validation ongenerateReport.Re-checking
report.periodbefore returning typed data prevents silent backend drift from leaking into downstream exhaustive switches.tests/unit/core/test_normalization.py (1)
10-122:compare_citest coverage looks complete and robust.Nice addition: the suite now validates explicit Unicode/whitespace cases, symmetry, and the normalization contract with deterministic Hypothesis examples.
src/synthorg/api/services/_org_agent_mutations.py (1)
171-349: Normalization helper migration is consistent and improves matching correctness.Using
compare_ci/normalize_identifieracross update/delete/reorder paths is a solid cleanup and avoids ad-hoc case handling drift.src/synthorg/communication/conflict_resolution/escalation/processors.py (6)
1-27: LGTM!Imports are well-organized. The
TYPE_CHECKINGguard forNotBlankStris appropriate with Python 3.14's PEP 649 deferred annotation evaluation.
29-36: LGTM!The
DecisionModeliteral type and_NO_WINNER_OUTCOMESfrozenset are well-defined and appropriately scoped.
39-66: LGTM!The dissent record builder correctly handles both winner and no-winner outcomes with appropriate position filtering. Immutable tuple return type is appropriate.
69-102: LGTM!The class structure is clean with
__slots__for efficiency, proper mode validation in__init__, and a read-onlymodeproperty. The docstring clearly documents the mode discriminator behaviour.
104-159: LGTM!The dispatch logic is well-structured with explicit type checks,
assert_neverfor exhaustiveness, and appropriate logging before raising. The log note values have been updated to the mode-based naming as requested in prior review.
161-241: LGTM!The static helper methods are well-implemented. The
_build_winner_resolutionproperly validates thatwinning_agent_idmatches a conflict participant before building the resolution. The_build_reject_resolutionparameter type has been narrowed toRejectDecisionas previously suggested, and log note values use the updated mode-based naming.tests/unit/communication/conflict_resolution/test_human_strategy.py (4)
1-52: LGTM!Import correctly updated to
HumanDecisionProcessor. The_tracking_registryhelper uses event-driven synchronization viaasyncio.Queuerather than polling, which is a robust pattern for async test coordination.
54-86: LGTM!The default behaviour tests appropriately exercise the resolver with
timeout_seconds=0and verify escalation outcomes, dissent record construction, and strategy metadata.
89-161: LGTM!The full-loop tests properly exercise both
mode="winner"andmode="hybrid"configurations with event-driven synchronization. The test structure clearly demonstrates the expected behaviour for each mode.
162-268: LGTM!The error path tests properly validate that winner mode rejects both
RejectDecisionand unknown agent IDs with appropriatepytest.raisesassertions. The timeout and sweeper tests comprehensively cover edge cases. The late-decision test correctly usesmode="winner"to verify that a persistedWinnerDecisionis honoured after timeout.scripts/protocol_audit.py (1)
81-109: Use a domain error type instead ofRuntimeErrorin_count().Line 102 and Line 108 still raise
RuntimeError; this breaks the project’s Python error hierarchy contract for domain code paths.
As per coding guidelines "**/*.py: Error classes:<Domain><Condition>Errornaming fromDomainError; never inheritException/RuntimeErrordirectly".
Six valid findings from CodeRabbit + Gemini, two skipped as invalid. Fixed: - src/synthorg/core/normalization.py: docstring 'Three' updated to 'Four' ASGI byte-header call sites (gemini) - src/synthorg/hr/registry.py: get_by_names now uses normalize_identifier so batch and single lookups share casefold + whitespace-strip semantics with get_by_name (coderabbit) - docs/reference/protocols-audit.md: escape malformed inline code span on line 384 (MD038) (coderabbit) - web/src/api/types/enums.ts: 'as const satisfies readonly string[]' on all 16 *_VALUES tuples to prevent silent type widening (coderabbit) - web/src/api/endpoints/reports.ts: validate /reports/periods payload against REPORT_PERIOD_VALUES before narrowing (coderabbit) - web/src/__tests__/pages/new-surfaces.test.tsx: drop misleading 'defined locally' comment; handlers come from global setup (coderabbit) Skipped (factually wrong): - gemini: 'NameError due to missing future annotations' on processors.py: Python 3.14+ has PEP 649 deferred eval; the project explicitly forbids 'from __future__ import annotations'. - coderabbit: 'use RS markers for numerics' on protocols-audit.md: file is intentionally not in scripts/check_doc_numeric_macros.py _SCOPED_FILES; audit numbers are a date-pinned snapshot, not live runtime stats.
Four valid CodeRabbit findings, all applied. - docs/reference/protocols-audit.md: replace 'uncommitted helper' parenthetical with a reproducible regeneration command so the audit's numeric snapshot is reproducible from committed state alone. - src/synthorg/communication/conflict_resolution/human_strategy.py: use 'is None' instead of 'or' for the processor fallback so a custom DecisionProcessor with a falsy __bool__ is preserved. - src/synthorg/communication/conflict_resolution/escalation/processors.py: detect RejectDecision explicitly and call assert_never on any other variant, so a future EscalationDecision member added to the union does not silently fall through as REJECTED_BY_HUMAN. - src/synthorg/settings/dispatcher.py: in start(), collapse a crashed-and-finished poll task under _lifecycle_lock before the _running guard, so a recovery start() does not race the queued _reset_running_under_lock and trip the 'already running' guard on a dispatcher whose previous task is already done.
Four valid CodeRabbit findings fixed; three skipped as factually
disclaimed by CR's own configuration or by the project's gate scope.
Fixed:
- src/synthorg/communication/conflict_resolution/escalation/processors.py:
narrow _build_reject_resolution decision parameter from
EscalationDecision to RejectDecision so the type checker enforces
the contract that the prior isinstance(RejectDecision) branch
established.
- src/synthorg/settings/dispatcher.py: in start(), unsubscribe the
crashed-task bus registration before clearing _task / _running so
the subsequent subscribe() call cannot double-register on bus
implementations whose subscribe is not idempotent (NATS).
- docs/reference/protocols-audit.md: switch line 384 from a double-
backtick-wrapped form (which markdownlint MD038 still flagged) to
a plain single-backtick code span; commit the audit generator at
scripts/protocol_audit.py and update the doc header to point at
the committed path so the snapshot is reproducible from the repo
alone.
- scripts/protocol_audit.py: new committed audit generator (was
living in the gitignored _audit/ working area).
Skipped (factually disclaimed):
- coderabbit: human_strategy.py:72 timeout-to-settings move.
CR-classified Nitpick / Trivial / Low value. The constant is
tracked in scripts/no_magic_numbers_baseline.txt:177 as a
project-sanctioned pre-existing exception. Similar operational
timeouts in this same PR (_STOP_DRAIN_TIMEOUT_SECONDS = 10.0
at dispatcher.py:43, _BOOTSTRAP_MAX_CONSECUTIVE_ERRORS = 30
at dispatcher.py:40) are also baselined and not flagged by CR.
- coderabbit: human_strategy.py:146-252 resolve()-length extract.
CR's own configuration directive forbids exactly this kind of
suggestion: 'Do not suggest extracting single-use helper
functions called exactly once -- this reduces readability
without improving maintainability.' Every proposed helper would
be called exactly once.
- coderabbit: processors.py Clock-seam threading.
CR-classified Poor tradeoff. The Clock seam is applied at
service boundaries that own time-dependent flow control
(timeouts, expiry, scheduling), not at private model-construction
helpers that produce informational resolved_at /
timestamp fields. Tests for _build_*_resolution do not
verify the timestamp value.
- coderabbit: protocols-audit.md:38-45 RS-marker substitution
(duplicate from round 1). The file is intentionally not in
scripts/check_doc_numeric_macros.py _SCOPED_FILES; the audit
numbers are a date-pinned snapshot recorded by the title
('Protocols Audit (2026-05-10)'), not live runtime stats that
would otherwise rot.
Seven valid CodeRabbit findings, all applied. - scripts/protocol_audit.py: regex now accepts type-parameterised protocol declarations (class Foo[T: Bar](Protocol):); _count raises RuntimeError on missing grep or non-zero grep exit instead of silently returning 0 (which would taint the audit table). - src/synthorg/communication/conflict_resolution/escalation/processors.py: rename structured-log notes from the legacy class names (winner_select_rejected_non_winner, hybrid_processor_rejected) to post-refactor mode-based labels (winner_mode_rejected_reject_decision, hybrid_mode_rejected_decision). - src/synthorg/api/services/_org_agent_mutations.py: switch reorder_dept lookup from .lower() to normalize_identifier so the selection (compare_ci) and the bucketing share Unicode-safe casefold semantics; the mixed pattern produced lookup misses for casefold-only equivalents (Strasse vs STRASSE) the same way the hr/registry.py site did before round 1. - web/src/api/endpoints/reports.ts: validate generateReport response.period through isReportPeriod before returning, mirroring listReportPeriods round-1 hardening. - tests/unit/core/test_normalization.py: bump deterministic Hypothesis examples to ten, covering empty / ASCII / German sharp-s / Greek sigma / whitespace / Latin combining accents / digits / single-char inequality / long strings. - tests/unit/communication/conflict_resolution/test_human_strategy.py: rename test_winner_select_* methods to test_winner_mode_* so test names track HumanDecisionProcessor(mode='winner') rather than the removed WinnerSelectProcessor.
One valid CodeRabbit finding applied. - scripts/protocol_audit.py: pass -o to grep so _count emits one line per match instead of one line per matched file-line. Without -o, source lines that contain multiple references to the same protocol (e.g. typed-arrow annotations) get counted once, silently undercounting typeuse / testuse and tainting the REMOVE classification.
Aureliolo
force-pushed
the
chore/issue-1850-cleanup
branch
from
008a607 to
3f73e9d
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
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 `@src/synthorg/communication/conflict_resolution/escalation/processors.py`:
- Around line 93-97: The __init__ in processors.py currently raises ValueError
when mode is invalid but doesn't log it; before raising in the DecisionMode
__init__ (the method that sets self._mode), emit a WARNING-level log containing
the attempted mode and the formatted message (e.g., via the module logger or
logging.getLogger(__name__)) so the invalid value and context are recorded, then
raise the ValueError as before.
- Around line 69-103: Add thin deprecated compatibility wrappers named
WinnerSelectProcessor and HybridDecisionProcessor that subclass or forward to
HumanDecisionProcessor (preserving the same constructor signature) and emit a
DeprecationWarning when instantiated; ensure these shim classes exist in the
same module where HumanDecisionProcessor is defined and re-export both names
(alongside HumanDecisionProcessor) from the package-level escalation __init__.py
so imports from ...escalation.processors and the package root continue to work
during the deprecation cycle.
In `@src/synthorg/settings/dispatcher.py`:
- Around line 147-165: In the start() recovery block where you catch Exception
around await self._bus.unsubscribe(_SETTINGS_CHANNEL, _SUBSCRIBER_ID) (the block
that logs SETTINGS_DISPATCHER_START_REJECTED and then sets self._running=False
and self._task=None), update the exception handler to re-raise MemoryError and
RecursionError before logging/handling other exceptions; i.e., after catching
Exception, if isinstance(exc, (MemoryError, RecursionError)): raise, otherwise
proceed with the logger.warning and recovery cleanup so the helper preserves
system-error propagation.
🪄 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: df51cbd8-9a1c-46ff-b167-7e854aa79e1e
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
.gitignoreCLAUDE.mddocs/reference/protocols-audit.mdscripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txtscripts/protocol_audit.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/api/etag.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/communication/meeting/participant.pysrc/synthorg/core/normalization.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/hr/registry.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/tools/network_validator.pysrc/synthorg/tools/web/http_request.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/core/test_normalization.pytests/unit/settings/test_dispatcher.pyweb/package.jsonweb/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
💤 Files with no reviewable changes (2)
- .gitignore
- web/package.json
📜 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: CodSpeed Web Pass
- GitHub Check: Build Backend
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Site
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration precedence must follow: DB > env > YAML > code default via
SettingsService/ConfigResolver; never useos.environ.getoutside startup perdocs/reference/configuration-precedence.mdNo hardcoded numeric values except: 0, 1, -1, HTTP codes, hex masks, powers-of-2; numerics must live in
settings/definitions/; enforced byscripts/check_no_magic_numbers.pyInclude comments explaining WHY only; no reviewer citations, issue back-refs, or migration framing; enforced by
check_no_review_origin_in_code.py+check_no_migration_framing.pyDo not use
from __future__ import annotations(Python 3.14 has PEP 649); use PEP 758 except:except A, B:requires parens unless bindingType hints required on public functions; strict mypy enforcement; use Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Error classes must follow
<Domain><Condition>Errorpattern, inherit fromDomainError, never directly fromException/RuntimeError/etc; enforced bycheck_domain_error_hierarchy.pyAPI DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes) must use Pydantic v2 frozen mode with
extra="forbid"; use@computed_fieldfor derived values; useNotBlankStrfor identifiersArgs models required at every system boundary; use
parse_typed()for every external dict ingestion; enforced bycheck_boundary_typed.pyEnforce immutability using
model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundariesUse
asyncio.TaskGroupfor async fan-out/fan-in; helpers must catchExceptionwith re-raise forMemoryError/RecursionErrorUse Clock seam pattern:
clock: Clock | None = None; tests injectFakeClock; services own_lifecycle_lock; timed-out stops mark unrestartableUntrusted content (SEC-1): use
wrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML; never directly use untrusted input in promptsLogging: import from
synthorg.observability.get_logger; v...
Files:
src/synthorg/communication/meeting/participant.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/tools/network_validator.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/core/normalization.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/etag.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/escalation/processors.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/communication/meeting/participant.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/tools/network_validator.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/core/normalization.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/api/auth/service.pysrc/synthorg/api/etag.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/providers/drivers/litellm_driver.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/engine/routing/scorer.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
{tests/baselines/unit_timing.json,scripts/*_baseline.{txt,json},scripts/_*_baseline.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Timeout/slow test failures indicate source-code regression; never edit
tests/baselines/unit_timing.jsonorscripts/*_baseline.{txt,json}/scripts/_*_baseline.pydirectly; useALLOW_BASELINE_GROWTH=1with explicit approval
Files:
scripts/long_running_loops_kill_switch_baseline.txtscripts/loop_bound_init_baseline.txtscripts/no_magic_numbers_baseline.txt
web/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,js,jsx}: Always usecreateLoggerfrom@/lib/logger; never use bareconsole.warn,console.error, orconsole.debugin application code
Logger variable name must always belog(e.g.,const log = createLogger('module-name'))
Pass dynamic or untrusted values to logger methods as separate arguments (not interpolated into the message string) so they go throughsanitizeArg
Wrap attacker-controlled fields inside structured objects insanitizeForLog()before embedding them in logs
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx}: Callers MUST NOT wrap store mutation calls in try/catch; the store owns the error UX
Any new caller of health/readiness endpoints must handle the 503 path explicitly
Any new WS payload handler that ingests untrusted strings MUST route throughsanitizeWsString()orsanitizeWsEnum()helpers
ImportErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported fromweb/src/api/types/error-codes.gen.ts); discriminate onErrorCode.<NAME>, never on raw integer literals
Never export agetXIcon(value): LucideIconfactory that returns a component reference called inside JSX render bodies; export a<XIcon value={...} {...lucideProps} />wrapper component instead usingcreateElementfor the lookup
Detectfetch()in effects withoutAbortControllercleanup using the@eslint-react/web-api-no-leaked-fetchrule
Catch the{count && <Foo />}bug using@eslint-react/no-leaked-conditional-rendering; forReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...)
Restrictwindow,document,localStorage, and other globals inside render using@eslint-react/globals; hoist offenders intouseCallbackevent handler,useEffect, oruseSyncExternalStore-backed hook
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Test setup must boot with
onUnhandledRequest: 'error'; tests override per-case viaserver.use(...), nevervi.mock('@/api/endpoints/*')
Files:
web/src/__tests__/pages/new-surfaces.test.tsx
web/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reuse components from
web/src/components/ui/following design tokens; seeweb/CLAUDE.mdfor details
Files:
web/src/__tests__/pages/new-surfaces.test.tsxweb/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must use markers:
@pytest.mark.{unit,integration,e2e,slow}; async tests useautofixture; global timeout 30s; coverage minimum 80%Windows: unit tests use
WindowsSelectorEventLoopPolicy; subprocess tests override back to defaultMock-spec: every Mock must declare
spec=ConcreteClass; baseline tracked atscripts/mock_spec_baseline.txtUse
FakeClockfromtests._shared.fake_clock; inject viaclock=parameterHypothesis: run 10 deterministic CI examples; test failures are real bugs requiring fix +
@example(...)annotationFlaky tests: NEVER skip/xfail; fix fundamentally; use
asyncio.Event().wait()instead ofsleep(large)for waiting
Files:
tests/unit/settings/test_dispatcher.pytests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.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_dispatcher.pytests/unit/core/test_normalization.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.py
{README.md,docs/**/*.md}
📄 CodeRabbit inference engine (CLAUDE.md)
Numeric claims in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers perdata/README.md
Files:
docs/reference/protocols-audit.md
docs/**/*.{d2,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
d2for architecture/nested container diagrams with theme 200 (Dark Mauve),mermaidfor flowcharts/sequence/pipelines; markdown tables for tabular data; D2 CLI pinned to v0.7.1 in CI
Files:
docs/reference/protocols-audit.md
web/src/{api,stores}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Display counts come from
data.length; the wire envelope no longer carriestotalfield for paginated responses
Files:
web/src/api/endpoints/reports.tsweb/src/api/types/enums.ts
web/src/api/endpoints/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state)
Files:
web/src/api/endpoints/reports.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Read `docs/design/` page before implementing; deviations need approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Present every implementation plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: No region/currency/locale should be privileged; use metric units and British English per `docs/reference/regional-defaults.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Only `src/synthorg/persistence/` may import sqlite/psycopg or emit raw SQL; all other code uses abstraction boundary per `docs/reference/persistence-boundary.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Every convention PR must ship its enforcement gate per `docs/reference/convention-gates.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: After implementation: branch + commit + push (no auto-PR); use `/pre-pr-review` command; after PR creation use `/aurelio-review-pr` for external feedback; fix all valid feedback without deferring
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Git commits must follow format: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); enforced by commitizen
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Branches must follow format: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Squash merge all PRs; 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-10T12:50:41.297Z
Learning: After every squash merge, run `/post-merge-cleanup` command
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: CLI is Docker-only (init/start/stop/status); new features go in dashboard + REST API, not CLI
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Web-related work: see `web/CLAUDE.md` for web-specific conventions; use Go CLI only with `go -C cli`, never `cd cli`; shell rules from `~/.claude/rules/common/bash.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T12:50:41.297Z
Learning: Layout: use src layout with `src/synthorg/` structure; separate `tests/`, `web/` (React 19), `cli/` (Go binary); see respective CLAUDE.md files
📚 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/communication/meeting/participant.pysrc/synthorg/api/auth/csrf.pysrc/synthorg/hr/training/source_selectors/department_diversity.pysrc/synthorg/tools/network_validator.pysrc/synthorg/communication/conflict_resolution/human_strategy.pysrc/synthorg/core/normalization.pysrc/synthorg/api/auth/middleware.pysrc/synthorg/api/controllers/settings.pysrc/synthorg/communication/conflict_resolution/escalation/protocol.pysrc/synthorg/api/controllers/_department_health.pysrc/synthorg/hr/training/source_selectors/role_top_performers.pysrc/synthorg/api/auth/service.pytests/unit/settings/test_dispatcher.pytests/unit/core/test_normalization.pysrc/synthorg/api/etag.pysrc/synthorg/integrations/webhooks/verifiers/github_hmac.pysrc/synthorg/communication/conflict_resolution/escalation/factory.pysrc/synthorg/api/services/org_mutations.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/tools/web/http_request.pysrc/synthorg/communication/conflict_resolution/escalation/__init__.pysrc/synthorg/integrations/webhooks/verifiers/generic_hmac.pytests/unit/communication/conflict_resolution/test_human_strategy.pysrc/synthorg/api/services/_org_agent_mutations.pysrc/synthorg/providers/drivers/litellm_driver.pytests/unit/communication/conflict_resolution/escalation/test_factory_registry.pysrc/synthorg/api/services/_org_department_mutations.pysrc/synthorg/engine/routing/scorer.pyscripts/protocol_audit.pysrc/synthorg/settings/subscribers/backup_subscriber.pysrc/synthorg/tools/html_parse_guard.pysrc/synthorg/engine/hybrid_helpers.pysrc/synthorg/hr/registry.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
🔇 Additional comments (41)
web/src/api/types/enums.ts (2)
3-7: Good single-source-of-truth contract in docs.Line 3-Line 7 clearly documents the runtime tuple ↔ derived union coupling and backend parity expectation; this makes future enum edits less error-prone.
10-102: Enum tuple-to-union refactor looks solid.Lines 10-102 consistently use
*_VALUES+(typeof ...)[number], including the newly added enum groups. This is a clean, maintainable replacement for generated schema-coupled aliases.web/src/api/endpoints/reports.ts (2)
4-26: Strong local typing and runtime guard setup.Lines 4-26 establish a clear local contract (
REPORT_PERIOD_VALUES/ReportPeriod) plus a proper runtime validator before narrowing untrusted values.
42-53: Defensive wire-data validation is correctly enforced.Line 42-Line 53 and Line 67-Line 74 validate response payloads before returning typed values, which protects downstream exhaustive handling from unexpected backend enum expansion.
Also applies to: 67-74
web/src/__tests__/pages/new-surfaces.test.tsx (2)
5-6: AI summary contradicts the actual comment.The AI summary states "workflow-executions/webhooks handlers are defined locally in this setup," but the actual comment on lines 5-6 says all handlers are "configured in the global test setup" with no mention of local workflow-executions/webhooks handlers. The code contains no local handler definitions.
The current comment appears correct and addresses the past review feedback appropriately.
34-97: Clean and consistent smoke-test implementation.The nine test cases follow an identical, straightforward pattern: render the page, wait for the heading to appear, and assert it's in the document. This aligns well with the stated goal of catching import breaks, routing errors, or schema mismatches without full UX coverage.
src/synthorg/api/etag.py (1)
216-217: Bytes-header exemption comments are clear and correctly scoped.These comments document the str-vs-bytes boundary without altering runtime behavior in the ASGI hot path.
Also applies to: 361-362
src/synthorg/core/normalization.py (1)
47-73:compare_cihelper implementation looks solid.This centralizes case-insensitive equality on top of
normalize_identifierand keeps call sites consistent.src/synthorg/tools/web/http_request.py (1)
284-286: Host-header normalization refactor is correct.Using
compare_cihere preserves behavior while aligning with the shared case-insensitive comparison path.src/synthorg/tools/html_parse_guard.py (1)
339-340:aria-hiddencheck update is a good normalization win.The new comparison keeps hidden-element stripping behavior consistent with the shared CI helper.
src/synthorg/api/controllers/_department_health.py (1)
107-107: Department name matching refactor is correct.Using
compare_cihere improves consistency with other case-insensitive matching paths.src/synthorg/tools/network_validator.py (1)
411-411: HTTPS scheme comparison change looks good.This keeps SSRF validation behavior intact while standardizing case-insensitive checks.
src/synthorg/api/controllers/settings.py (1)
623-623: Boolean parsing refactor is clean and consistent.
compare_ci(raw_correlation, "true")keeps intent explicit and aligns with shared normalization.src/synthorg/providers/drivers/litellm_driver.py (1)
724-726:retry-afterheader lookup change is correct.The case-insensitive key match is aligned with HTTP semantics and keeps retry extraction robust.
src/synthorg/engine/routing/scorer.py (1)
15-15: Good centralization of role matching logic.Switching to
compare_cikeeps the role-bonus behavior intact while aligning this scorer with the shared normalization path.Also applies to: 328-330
src/synthorg/api/services/org_mutations.py (1)
25-25: Nice deduplication of case-insensitive lookups.Using
find_by_name_ciin both helpers improves consistency and removes duplicated matching code without changing service semantics.Also applies to: 188-188, 214-214
src/synthorg/api/auth/middleware.py (1)
27-27: Bearer-scheme parsing update looks solid.The
compare_ciswitch is a clean consistency improvement for case-insensitive Authorization scheme handling.Also applies to: 194-194
src/synthorg/engine/hybrid_helpers.py (1)
13-13: Good normalization reuse in replan parsing.Delegating the string-bool check to
compare_cimakes the JSON"replan"interpretation path more consistent.Also applies to: 392-392
src/synthorg/api/auth/csrf.py (1)
179-180: Clarifying comments are helpful and well-scoped.The added notes make the bytes-header exception explicit without altering CSRF behavior.
Also applies to: 207-208
src/synthorg/settings/subscribers/backup_subscriber.py (1)
5-5: Enabled-flag parsing refactor looks correct.Using
compare_cihere improves consistency with the project-wide normalization strategy while preserving subscriber behavior.Also applies to: 114-114
src/synthorg/integrations/webhooks/verifiers/generic_hmac.py (1)
6-6: Header lookup normalization is a good cleanup.This keeps generic webhook verifier behavior consistent with the shared case-insensitive comparison helper.
Also applies to: 53-53
src/synthorg/communication/meeting/participant.py (1)
10-10:"all"sentinel matching change is clean.Using
compare_cihere is consistent with the central normalization approach and keeps resolver behavior predictable.Also applies to: 250-250
src/synthorg/integrations/webhooks/verifiers/github_hmac.py (1)
6-6: Clean migration to shared CI comparison helper.The
compare_ciswitch in header lookup keeps the fallback behavior intact and improves consistency with the new normalization path.Also applies to: 26-27
src/synthorg/api/auth/service.py (1)
55-95: Docstring update is clear and aligned with implementation.The added rationale for async boundaries, thread-safety, and out-of-scope behavior is precise and consistent with the current code paths.
src/synthorg/hr/training/source_selectors/department_diversity.py (1)
10-10: Selector changes look solid.Config validation now logs structured context before raising, and role matching correctly uses
compare_cifor centralized normalization semantics.Also applies to: 16-17, 64-71, 111-124
src/synthorg/api/services/_org_department_mutations.py (1)
19-19: CI comparison migration is correctly applied in department mutations.The update/delete/conflict checks now consistently use
compare_ciwith no regression in CAS mutation behavior.Also applies to: 189-190, 250-251, 290-290
src/synthorg/hr/registry.py (1)
18-22: Lookup normalization updates are consistent and correct.
get_by_name,get_by_names, andlist_by_departmentnow share the same normalization strategy and preserve expected lock/ordering semantics.Also applies to: 209-209, 250-260, 289-290
tests/unit/core/test_normalization.py (1)
10-10: Great test coverage forcompare_cicontract.The new cases + symmetry + property contract against
normalize_identifierprovide strong regression protection for normalization semantics.Also applies to: 76-122
src/synthorg/hr/training/source_selectors/role_top_performers.py (1)
10-10: Role selector updates are clean and consistent.Structured config-error logging and
compare_ci-based role matching are both correctly integrated.Also applies to: 15-16, 55-62, 90-90
src/synthorg/api/services/_org_agent_mutations.py (1)
19-19: Agent mutation normalization migration is well executed.The updated conflict checks, replacements, deletions, and departmental reorder paths consistently use shared normalization helpers without altering mutation orchestration.
Also applies to: 171-172, 238-239, 278-280, 292-292, 335-336, 340-343, 349-349
src/synthorg/settings/dispatcher.py (3)
106-111: LGTM!Good use of a strong-reference set to prevent GC from collecting fire-and-forget tasks (RUF006 compliance). The cleanup callback at line 384 properly discards completed tasks.
367-398: LGTM!The refactored done-callback pattern is sound:
- Synchronous callback correctly schedules async work instead of blocking.
- Identity check in
_reset_running_under_lockprevents stale-task races.- Strong ref in
_post_done_tasksplus discard callback keeps the set bounded.
522-522: LGTM!Using
compare_cifor the case-insensitive"false"check aligns with the PR objective to centralise CI comparisons. The fail-safe default (non-"false"→True) is correctly preserved.tests/unit/settings/test_dispatcher.py (2)
529-535: LGTM!Deterministically awaiting
_post_done_tasksbefore asserting eliminates the race between the async reset and the assertion. Aligns correctly with the dispatcher's new callback scheduling.
641-643: LGTM!Consistent application of the same wait pattern as the unexpected-exit test.
scripts/long_running_loops_kill_switch_baseline.txt (1)
31-31: LGTM!Line reference correctly updated to match the new location of
_poll_loopafter the crash-recovery code insertion.scripts/loop_bound_init_baseline.txt (1)
34-34: LGTM!Line offset correctly updated to reflect the shifted
_lifecycle_lockassignment.scripts/no_magic_numbers_baseline.txt (1)
27-27: LGTM!Line-offset migrations are consistent with code changes elsewhere in this PR. The baseline remains accurate for the magic-numbers gate.
Also applies to: 121-121, 376-376, 400-402, 752-756, 780-781, 829-830
docs/reference/protocols-audit.md (1)
36-45: Replace hard-coded summary numerics with runtime-stats markers.Lines 38–45 still hard-code public numeric claims (
237,193,31,13,209 of 237 (88%)) instead of sourcing fromdata/runtime_stats.yamlvia<!--RS:NAME-->markers.As per coding guidelines: “Numeric claims in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers perdata/README.md”.scripts/protocol_audit.py (1)
44-46: Protocol discovery/counting implementation looks correct now.The generic-aware class regex and the
grep -rEooccurrence counting with fail-fast error handling are aligned with the audit script’s stated intent.Also applies to: 81-117
CLAUDE.md (1)
46-46: Reference index update is correct.Adding
docs/reference/protocols-audit.mdto the load-on-demand list improves discoverability and is consistent with the surrounding links.
| class HumanDecisionProcessor: | ||
| """Build a :class:`ConflictResolution` from an operator decision. | ||
|
|
||
| The ``mode`` discriminator selects which decision shapes are | ||
| accepted: | ||
|
|
||
| - ``mode="winner"`` (safest surface) accepts only | ||
| :class:`WinnerDecision`. A :class:`RejectDecision` raises a | ||
| precise ``ValueError`` so the REST layer can surface a | ||
| ``422 Unprocessable Entity`` instead of a 500. | ||
| - ``mode="hybrid"`` additionally accepts | ||
| :class:`RejectDecision`, producing a ``REJECTED_BY_HUMAN`` | ||
| outcome with no winner so the caller can fall back to a | ||
| different strategy (retry, alternative resolution, manual | ||
| intervention). | ||
|
|
||
| Rejects ``RejectDecision`` with a precise ``ValueError`` so the | ||
| REST layer can surface a 422 Unprocessable Entity instead of a 500. | ||
| Args: | ||
| mode: Strategy discriminator. Defaults to ``"winner"``. | ||
| """ | ||
|
|
||
| __slots__ = () | ||
| __slots__ = ("_mode",) | ||
|
|
||
| _mode: DecisionMode | ||
|
|
||
| def __init__(self, mode: DecisionMode = "winner") -> None: | ||
| if mode not in ("winner", "hybrid"): | ||
| msg = f"mode must be 'winner' or 'hybrid', got {mode!r}" | ||
| raise ValueError(msg) | ||
| self._mode = mode | ||
|
|
||
| @property | ||
| def mode(self) -> DecisionMode: | ||
| """The strategy discriminator (``"winner"`` or ``"hybrid"``).""" | ||
| return self._mode | ||
|
|
There was a problem hiding this comment.
Keep deprecated wrappers for the removed processor class names.
This refactor collapses the implementation cleanly, but removing WinnerSelectProcessor and HybridDecisionProcessor outright is still a breaking API change for callers importing those public symbols from ...escalation.processors or the package root. Please keep thin compatibility shims for at least one deprecation cycle and re-export them alongside HumanDecisionProcessor.
♻️ Proposed compatibility shim
class HumanDecisionProcessor:
...
+class WinnerSelectProcessor(HumanDecisionProcessor):
+ """Deprecated compatibility shim; use HumanDecisionProcessor(mode="winner")."""
+
+ def __init__(self) -> None:
+ super().__init__(mode="winner")
+
+
+class HybridDecisionProcessor(HumanDecisionProcessor):
+ """Deprecated compatibility shim; use HumanDecisionProcessor(mode="hybrid")."""
+
+ def __init__(self) -> None:
+ super().__init__(mode="hybrid")You would also want to add both names back to src/synthorg/communication/conflict_resolution/escalation/__init__.py.
🤖 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/communication/conflict_resolution/escalation/processors.py`
around lines 69 - 103, Add thin deprecated compatibility wrappers named
WinnerSelectProcessor and HybridDecisionProcessor that subclass or forward to
HumanDecisionProcessor (preserving the same constructor signature) and emit a
DeprecationWarning when instantiated; ensure these shim classes exist in the
same module where HumanDecisionProcessor is defined and re-export both names
(alongside HumanDecisionProcessor) from the package-level escalation __init__.py
so imports from ...escalation.processors and the package root continue to work
during the deprecation cycle.
…h + lazy lifecycle lock CodeRabbit findings (round 6 review): - src/synthorg/communication/conflict_resolution/escalation/processors.py: log a WARNING with attempted-mode + msg via CONFLICT_VALIDATION_ERROR before raising on invalid mode; aligns with the project rule that error paths log context before raising. - src/synthorg/settings/dispatcher.py: catch MemoryError/RecursionError and re-raise inside the start() recovery unsubscribe block; matches the helper-must-propagate-system-errors rule used at every other except-Exception site in this file. Skipped (factually disclaimed): - coderabbit: re-introduce WinnerSelectProcessor / HybridDecisionProcessor shims for backward compatibility. The project's no-backward-compat policy (pre-alpha) explicitly forbids deprecation shims; the whole point of this PR's processor consolidation was to remove these classes, so re-introducing shims would defeat the refactor. Proper kill switch (per user request): - src/synthorg/settings/dispatcher.py: thread an optional ConfigResolver into the dispatcher and gate the poll loop on settings.dispatcher.enabled (fail-safe to enabled). Replaces the baselined missing-kill-switch entry with a real runtime guard so operators can pause propagation without a restart. - src/synthorg/settings/dispatcher.py: replace the module-level _BOOTSTRAP_MAX_CONSECUTIVE_ERRORS=30 and _STOP_DRAIN_TIMEOUT_SECONDS=10.0 constants with resolver-read helpers (_resolve_max_consecutive_errors, _resolve_stop_drain_timeout) that fall back to inline literals on resolver outage; clears two no_magic_numbers baseline entries. - src/synthorg/settings/definitions/settings_ns.py: register settings.dispatcher_enabled (boolean) and settings.dispatcher_stop_drain_timeout_seconds (float, 1-60s range). - src/synthorg/observability/events/settings.py: new SETTINGS_DISPATCHER_RESOLVE_FAILED event for the kill-switch resolver-failure WARNING (debounced via _resolve_failed_logged). - src/synthorg/api/lifecycle_helpers.py: pass app_state.config_resolver into the dispatcher when available. - tests/unit/settings/test_dispatcher.py: replace the _MAX_CONSECUTIVE_ERRORS module-level monkeypatch with a _FakeConfigResolver stub that returns the deterministic value via the new resolver pathway. Lazy lifecycle lock (per user request): - src/synthorg/settings/dispatcher.py: move asyncio.Lock construction out of __init__ and into start() so the lock binds to the event loop that actually drives the dispatcher. Adds the canonical _task_is_on_current_loop / _drop_stale_loop_state pair from ApprovalTimeoutScheduler, but diverges on the done-task case: treats a finished task on the same loop as 'do not drop' so a concurrent stop() racing a recovery start() does not have its lock yanked out and replaced. The 'finished prior lifecycle' cleanup happens inside the lock via the new _collapse_finished_task_under_lock helper instead. stop() and _reset_running_under_lock both no-op when the lock is None (dispatcher never started or post-drop). Stale-baseline warnings (left for the user's separate cleanup PR or --update-baseline run; gates pass with stale entries present): - scripts/long_running_loops_kill_switch_baseline.txt: src/synthorg/settings/dispatcher.py:_poll_loop:414 (loop now has a real kill switch). - scripts/no_magic_numbers_baseline.txt: dispatcher.py:40:48 and 43:44 (constants moved out of module-level).
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/synthorg/communication/conflict_resolution/escalation/processors.py (1)
63-63: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding a Clock seam for deterministic timestamp testing.
The processor creates timestamps via
datetime.now(UTC)in three places (lines 63, 212, 238). Per coding guidelines, services should acceptclock: Clock | None = Noneto allow tests to inject aFakeClockfor deterministic verification of timestamp-sensitive behavior.♻️ Proposed Clock seam
+from synthorg.core.clock import Clock + class HumanDecisionProcessor: ... - __slots__ = ("_mode",) + __slots__ = ("_mode", "_clock") - def __init__(self, mode: DecisionMode = "winner") -> None: + def __init__( + self, + mode: DecisionMode = "winner", + *, + clock: Clock | None = None, + ) -> None: ... + self._clock = clock or Clock()Then replace
datetime.now(UTC)calls withself._clock.now()(or passclockto the helper function).As per coding guidelines: "Implement Clock seam:
clock: Clock | None = Noneparameter in services; tests must injectFakeClockfor time-dependent logic."Also applies to: 212-212, 238-238
🤖 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/communication/conflict_resolution/escalation/processors.py` at line 63, Add a Clock seam to the processor by accepting a parameter like clock: Clock | None = None in the processor's constructor (store it as self._clock = clock or Clock()) and replace all direct datetime.now(UTC) calls in this module (the three occurrences at lines where timestamps are created) with self._clock.now(); ensure any helper functions that create timestamps accept the clock (or use self._clock) so tests can inject a FakeClock for deterministic timestamps.
🤖 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/settings/dispatcher.py`:
- Line 519: Bootstrap default literals (bootstrap_default = 30 and the 10.0
fallback) should be derived from the canonical settings defaults instead of
hard-coded; replace those magic values by importing and using
synthorg.settings.registry.registered_default_int and registered_default_float
(call them with the appropriate setting key/name used in settings_ns.py) to
obtain the defaults at module load, and if that causes circular-import issues,
add a clear comment next to each literal pointing to the authoritative
definition in settings_ns.py and open an issue to refactor into a non-circular
accessor; refer to the symbols bootstrap_default, the 30 literal, and the 10.0
literal when making the change.
---
Outside diff comments:
In `@src/synthorg/communication/conflict_resolution/escalation/processors.py`:
- Line 63: Add a Clock seam to the processor by accepting a parameter like
clock: Clock | None = None in the processor's constructor (store it as
self._clock = clock or Clock()) and replace all direct datetime.now(UTC) calls
in this module (the three occurrences at lines where timestamps are created)
with self._clock.now(); ensure any helper functions that create timestamps
accept the clock (or use self._clock) so tests can inject a FakeClock for
deterministic timestamps.
🪄 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: 16968468-96b8-44df-8540-d15fbfdae77e
📒 Files selected for processing (6)
src/synthorg/api/lifecycle_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.pysrc/synthorg/observability/events/settings.pysrc/synthorg/settings/definitions/settings_ns.pysrc/synthorg/settings/dispatcher.pytests/unit/settings/test_dispatcher.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). (9)
- GitHub Check: CodSpeed Python Pass
- GitHub Check: Build Backend
- GitHub Check: Build Web Assets (melange)
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Read
docs/design/page before implementing; deviations need approval. See DESIGN_SPEC.md.No region/currency/locale privileged; use metric units and British English.
Every convention PR must ship its enforcement gate. See docs/reference/convention-gates.md.
Configuration precedence: DB > env > YAML > code default via
SettingsService/ConfigResolver; noos.environ.getoutside startup.No hardcoded numeric values in code; numerics live in
settings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2. Enforced byscripts/check_no_magic_numbers.py.Use Google-style docstrings on all public functions.
Line length 88 characters; functions must be under 50 lines; files must be under 800 lines.
Error classes must follow
<Domain><Condition>Errornaming and inherit fromDomainError, never directly fromException/RuntimeError. Enforced bycheck_domain_error_hierarchy.py.Use Pydantic v2 frozen models with
extra="forbid"on all API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes); use@computed_fieldfor derived values; useNotBlankStrfor identifiers.Create args models at every system boundary; use
parse_typed()for every external dict ingestion. Enforced bycheck_boundary_typed.py.Ensure immutability: use
model_copy(update=...)orcopy.deepcopy(); perform deepcopy at system boundaries.Use
asyncio.TaskGroupfor async fan-out/fan-in operations; helpers must catchExceptionbut re-raiseMemoryError/RecursionError.Implement Clock seam:
clock: Clock | None = Noneparameter in services; tests must injectFakeClockfor time-dependent logic.Services must own
_lifecycle_lock; timed-out stops must mark services as unrestartable.For untrusted content (SEC-1): use
wrap_untrusted()fromengine.prompt_safety; useHTMLParseGuardfor HTML parsing.Implement repository CRUD methods:
save(entity),get(id), `...
Files:
src/synthorg/observability/events/settings.pytests/unit/settings/test_dispatcher.pysrc/synthorg/settings/definitions/settings_ns.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.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/observability/events/settings.pysrc/synthorg/settings/definitions/settings_ns.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not edit
tests/baselines/unit_timing.jsonor anyscripts/*_baseline.{txt,json}/scripts/_*_baseline.py; timeout/slow failures indicate source-code regression. Bypass only with explicit user approval viaALLOW_BASELINE_GROWTH=1 git commit.Use pytest markers
@pytest.mark.{unit,integration,e2e,slow}for test classification.Async tests use
automode; global timeout 30 seconds; coverage minimum 80%. xdist-n 8 --dist=loadfileauto-applied via pyproject.Windows unit tests must use
WindowsSelectorEventLoopPolicy; subprocess tests override back to default.Every Mock must declare
spec=ConcreteClass; baseline atscripts/mock_spec_baseline.txt.Use
FakeClockfromtests._shared.fake_clockfor time-dependent tests; inject viaclock=parameter.Hypothesis: 10 deterministic CI examples; failures are real bugs—fix and add
@example(...)to prevent regression.Never skip or xfail flaky tests; fix fundamentally. Use
asyncio.Event().wait()instead ofsleep(large)for async coordination.
Files:
tests/unit/settings/test_dispatcher.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_dispatcher.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T14:47:12.800Z
Learning: Present every plan for accept/deny before coding.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T14:47:12.800Z
Learning: After issue implementation: create branch, commit, push (no auto-PR); use `/pre-pr-review`. After PR creation: use `/aurelio-review-pr` for external feedback. Fix all valid feedback; no deferring.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T14:47:12.800Z
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-10T14:47:12.800Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`).
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T14:47:12.800Z
Learning: Branches must follow `<type>/<slug>` naming from main.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T14:47:12.800Z
Learning: Use squash merge; PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body.
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T14:47:12.800Z
Learning: After every squash merge, run `/post-merge-cleanup`.
📚 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/settings.pytests/unit/settings/test_dispatcher.pysrc/synthorg/settings/definitions/settings_ns.pysrc/synthorg/settings/dispatcher.pysrc/synthorg/api/lifecycle_helpers.pysrc/synthorg/communication/conflict_resolution/escalation/processors.py
🔇 Additional comments (24)
src/synthorg/settings/definitions/settings_ns.py (1)
62-103: LGTM!The two new dispatcher settings are well-defined with appropriate defaults, bounds, and clear documentation explaining the fail-safe behavior. The
dispatcher_enabledkill-switch correctly resolves to enabled on outage, anddispatcher_stop_drain_timeout_secondshas sensible min/max bounds (1.0–60.0s).src/synthorg/observability/events/settings.py (1)
40-46: LGTM!The new
SETTINGS_DISPATCHER_RESOLVE_FAILEDevent follows the established naming convention, and the docstring clearly documents the debouncing behavior and fail-safe semantics.src/synthorg/settings/dispatcher.py (9)
73-117: LGTM!The constructor changes are well-structured: the optional
config_resolverenables runtime kill-switch and tunable resolution, the lazy_lifecycle_lockcorrectly defers lock creation tostart()to bind to the active event loop, and_post_done_tasksproperly holds strong references to prevent GC of fire-and-forget coroutines (RUF006 compliance).
119-157: LGTM!The cross-loop detection logic is sound:
_task_is_on_current_loop()correctly handles all edge cases (no task, done task, introspection failures, mock objects in tests), and_drop_stale_loop_state()cleanly discards primitives bound to a closed or different loop. The PEP 758except RuntimeError, AttributeError:syntax is correct for Python 3.14+.
158-193: LGTM!
_collapse_finished_task_under_lock()correctly addresses the transient "already running" race identified in past reviews. TheMemoryError, RecursionErrorre-raise at line 179-180 satisfies the system-error propagation requirement, and the recovery unsubscribe prevents double-registration on bus implementations that aren't idempotent.
195-295: LGTM!The
start()method properly sequences cross-loop detection before touching lifecycle primitives, lazily initializes the lock, collapses finished tasks under the lock, and maintains transactional subscribe+spawn semantics with proper rollback on failure. The past review concern about the race condition is fully addressed.
297-406: LGTM!The
stop()method correctly handles the lazy lock (no-op when lock is None), resolves drain timeout at runtime, and properly propagates system errors. The guard at line 368 correctly treats a cleared_task(from_reset_running_under_lockracing in) as "task completed" rather than suppressing caller cancellation.
408-476: LGTM!The
_on_task_done()callback correctly schedules the locked state reset via_post_done_tasksrather than attempting synchronous lock acquisition. The strong-ref pattern withadd_done_callback(self._post_done_tasks.discard)prevents both premature GC and unbounded set growth.
477-554: LGTM!The resolver methods (
_resolve_enabled,_resolve_max_consecutive_errors,_resolve_stop_drain_timeout) all correctly re-raiseCancelledErrorandMemoryError, RecursionError, fall back to bootstrap defaults on other failures, and implement appropriate debouncing for the kill-switch resolver logging.
566-618: LGTM!The poll loop correctly gates dispatch on the kill-switch as the first operation, resolves max-consecutive-errors at runtime, and maintains proper error isolation. The sleep on disabled state preserves event-loop yield cadence.
686-689: LGTM!Using
compare_ci(str(restart_raw), "false")for case-insensitive metadata parsing aligns with the PR's centralization ofcompare_ciusage and handles any casing of "false"/"False"/"FALSE".src/synthorg/api/lifecycle_helpers.py (1)
622-629: LGTM!The wiring correctly extracts
config_resolverfromapp_statewhen available and passes it toSettingsChangeDispatcher, enabling the dispatcher's runtime kill-switch and tunable resolution in production. Thehas_config_resolverguard ensures safe access.tests/unit/settings/test_dispatcher.py (3)
53-99: LGTM!The
_FakeConfigResolverstub is well-designed: it provides the three resolver methods needed by the dispatcher, raisesKeyErroron unexpected calls (fail-fast for test bugs), and the docstring clearly explains its purpose. Using a stub rather than a full mock gives precise control over dispatcher tunables.
579-585: LGTM!Awaiting
_post_done_tasksbefore asserting_running is Falsemakes the test deterministic by ensuring the scheduled_reset_running_under_lockcoroutine has completed. This aligns with the dispatcher's new done-callback lifecycle.
651-660: LGTM!Injecting
_FakeConfigResolver(max_consecutive_errors=5)via theconfig_resolverparameter properly exercises the dispatcher's runtime resolution path. Thecast(ConfigResolver, ...)is appropriate since the stub implements the required protocol without inheriting from the type.Also applies to: 687-697
src/synthorg/communication/conflict_resolution/escalation/processors.py (9)
70-109: Prior concern about backward compatibility shims remains unaddressed.A previous review flagged that removing
WinnerSelectProcessorandHybridDecisionProcessoroutright is a breaking API change for any callers importing those symbols. The suggestion was to keep thin compatibility wrappers for at least one deprecation cycle. If these classes were never part of the external API surface, please confirm explicitly; otherwise, consider the deprecation shims.
1-28: LGTM!Imports are well-organized. The
TYPE_CHECKINGguard forNotBlankStris correct for Python 3.14+ with PEP 649 deferred annotation evaluation.
30-37: LGTM!
DecisionModetype alias and_NO_WINNER_OUTCOMESfrozenset are well-defined and idiomatic.
40-67: LGTM!The helper function correctly handles both winner and no-winner outcomes for dissent record generation.
70-109: LGTM!The class docstring clearly documents the mode discriminator behavior. The
__init__correctly validates the mode and logs a warning withattempted_modecontext before raising, following the error-logging guideline.
111-166: LGTM!The process method correctly routes decisions by type and mode, with proper error handling and logging. The
assert_neverguard ensures exhaustive handling ofEscalationDecisionvariants.
168-174: LGTM!Clean delegation to the module-level helper.
176-222: LGTM!Winner validation correctly logs the invalid agent ID context before raising. The INFO log after resolution creation follows the state-transition logging guideline.
224-248: LGTM!The
decisionparameter is correctly narrowed toRejectDecision, and the log note uses the updated naming convention.
CodeRabbit findings: - src/synthorg/settings/dispatcher.py: cross-reference the bootstrap-default literals (30 + 10.0) to their canonical home in settings/definitions/settings_ns.py so future drift between the two surfaces is obvious. The literals are kept inline because importing the registered defaults at module load risks a circular import (registry to models to enums to dispatcher). Skipped (factually disclaimed): - coderabbit: re-introduce a Clock seam on the HumanDecisionProcessor model-construction helpers (DUPLICATE from round 4). Same disposition: the Clock seam is applied at service boundaries with time-dependent flow control, not at private helpers that produce informational resolved_at timestamps; tests for _build_*_resolution do not verify the timestamp value. CI fix (codecov/patch): - tests/unit/settings/test_dispatcher.py: new TestResolverHelpers, TestKillSwitch, and TestLazyLifecycleLock classes covering the round-6 additions: kill switch enabled / disabled / resolver failure paths, max-consecutive-errors and stop-drain-timeout fallback paths, lazy lock construction, no-op stop without start, cross-loop helper edge cases, and stale-loop drop. Adds a _RaisingResolver stub for the resolver-down code paths. - tests/unit/communication/conflict_resolution/test_human_strategy.py: test_invalid_mode_raises_value_error covers the HumanDecisionProcessor invalid-mode branch (logs WARNING + raises ValueError) added in round 6.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
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 `@src/synthorg/settings/dispatcher.py`:
- Around line 174-193: The unsubscribe failure in the start() recovery path must
abort recovery instead of continuing to clear state and resubscribe; in the
try/except around self._bus.unsubscribe(_SETTINGS_CHANNEL, _SUBSCRIBER_ID) (in
start/recovery), change the generic except Exception as exc handler to log the
failure using SETTINGS_DISPATCHER_START_REJECTED as currently done and then
re-raise the exception (or raise a new RecoveryError) so that the subsequent
lines that set self._running = False and self._task = None and the later
subscribe() call are not executed; keep the MemoryError/RecursionError
passthrough behavior and ensure you reference the same symbols
(_bus.unsubscribe, _SETTINGS_CHANNEL, _SUBSCRIBER_ID,
SETTINGS_DISPATCHER_START_REJECTED, self._running, self._task) so the change is
applied in the correct block.
- Around line 201-210: The lifecycle lock stays bound to the old event loop
after a normal stop() because stop() sets self._task to None but does not clear
self._lifecycle_lock; update the shutdown path (the stop() method where
self._task is set to None) to also clear the stale lock by setting
self._lifecycle_lock = None (or otherwise recreating the lock) so subsequent
start() calls create a fresh asyncio.Lock() on the new loop; alternatively, in
the startup path around the check that calls _task_is_on_current_loop() and
_drop_stale_loop_state(), treat the case where self._task is None by also
dropping/clearing self._lifecycle_lock before creating a new lock to avoid the
'<Lock> is bound to a different event loop' RuntimeError.
🪄 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: a9e015b6-ed7c-4b32-a0b4-257efa33d3b3
📒 Files selected for processing (3)
src/synthorg/settings/dispatcher.pytests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/settings/test_dispatcher.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). (7)
- GitHub Check: Build Backend
- GitHub Check: Dashboard Test
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Lighthouse Site
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration precedence: DB > env > YAML > code default via SettingsService/ConfigResolver; no
os.environ.getoutside startupUse
uv sync,ruff,mypy, andpytestwith specified configurations for dependency management, linting, type-checking, and testingDo not use
from __future__ import annotations(3.14 has PEP 649); use PEP 758 except no parens for unbound except clauses unless binding
Files:
tests/unit/communication/conflict_resolution/test_human_strategy.pysrc/synthorg/settings/dispatcher.pytests/unit/settings/test_dispatcher.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers:
@pytest.mark.{unit,integration,e2e,slow}; async auto; 30s global timeout; 80% min coverageMock-spec: every Mock declares
spec=ConcreteClass; baseline atscripts/mock_spec_baseline.txtFakeClock from
tests._shared.fake_clock; inject viaclock=parameterHypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...))Flaky tests: NEVER skip/xfail; fix fundamentally; use
asyncio.Event().wait()notsleep(large)
Files:
tests/unit/communication/conflict_resolution/test_human_strategy.pytests/unit/settings/test_dispatcher.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/communication/conflict_resolution/test_human_strategy.pytests/unit/settings/test_dispatcher.py
src/synthorg/!(persistence)/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only
src/synthorg/persistence/may import sqlite/psycopg or emit raw SQL; all other modules must use repository abstraction
Files:
src/synthorg/settings/dispatcher.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
No hardcoded numeric values except 0/1/-1, HTTP codes, hex masks, and powers-of-2; numerics live in
settings/definitions/Comments must explain WHY only; no reviewer citations, issue back-refs, or migration framing; enforced by check_no_review_origin_in_code.py + check_no_migration_framing.py
Type hints required on public functions; mypy strict mode; Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Error classes must follow
<Domain><Condition>Errornaming and inherit fromDomainErrornever from Exception/RuntimeError/etc directly; enforced by check_domain_error_hierarchy.pyPydantic v2 models for API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes) must be frozen +
extra="forbid"; use@computed_fieldfor derived;NotBlankStrfor identifiersArgs models required at every system boundary; use
parse_typed()for every external dict ingestion; enforced by check_boundary_typed.pyEnsure immutability: use
model_copy(update=...)orcopy.deepcopy(); deepcopy at system boundariesUse
asyncio.TaskGroupfor fan-out/fan-in; helpers catchExceptionand re-raiseMemoryError/RecursionErrorInject
clock: Clock | None = Noneseam; tests injectFakeClockfrom tests._shared.fake_clock; services own_lifecycle_lock; timed-out stops mark unrestartableWrap untrusted content with
wrap_untrusted()from engine.prompt_safety; useHTMLParseGuardfor HTML per SEC-1Use
from synthorg.observability import get_logger; variable always namedlogger; never import logging or use print() in app codeEvent names from
observability.events.<domain>constants; use structured kwargs (logger.info(EVENT, key=value))Error paths log WARNING/ERROR with context before raising; state transitions log INFO via
*_STATUS_TRANSITIONEDAFTER persistence writeSecret-log redaction (SEC-1): never use
error=str(exc)or interpolate{exc}; useerror_type=type(exc).__name__+ `error=safe_error_descript...
Files:
src/synthorg/settings/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/dispatcher.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Read design specification from `docs/design/` before implementing; deviations require approval per DESIGN_SPEC.md
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Present every plan for accept/deny before coding
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: No region/currency/locale should be privileged; use metric units and British English
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Every convention PR must ship its enforcement gate
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Timeout/slow test failures indicate source-code regression; never edit `tests/baselines/unit_timing.json` or any `scripts/*_baseline.{txt,json}` / `scripts/_*_baseline.py`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: After issue implementation: branch + commit + push (no auto-PR); use `/pre-pr-review` gh pr create is hookify-blocked; after PR use `/aurelio-review-pr` for external feedback
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: xdist `-n 8 --dist=loadfile` auto-applied via pyproject addopts (loadfile prevents 3.14+Windows ProactorEventLoop leak)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Commits: `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Branches: `<type>/<slug>` from main
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Pre-commit/pre-push hooks defined in `.pre-commit-config.yaml`; hookify rules in `.claude/hookify.*.md`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: Squash merge; PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: GitHub queries: use `gh issue list` via Bash, NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: After every squash merge, run `/post-merge-cleanup`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: For web development, see `web/CLAUDE.md` for framework-specific guidelines
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: For CLI development, see `cli/CLAUDE.md` and use `go -C cli`, never `cd cli`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-10T15:12:27.388Z
Learning: For shell scripting rules, see `~/.claude/rules/common/bash.md` (canonical for `cd` / `git -C` / Bash file-write rules)
📚 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/communication/conflict_resolution/test_human_strategy.pysrc/synthorg/settings/dispatcher.pytests/unit/settings/test_dispatcher.py
🔇 Additional comments (4)
tests/unit/communication/conflict_resolution/test_human_strategy.py (4)
18-20: Processor import migration is correct.Switching imports to
HumanDecisionProcessoris consistent with the consolidated escalation processor contract.
96-101: Resolver wiring uses mode discriminator consistently.The resolver setup now explicitly tests both
"winner"and"hybrid"processor modes, including timeout/late-decision behaviour with winner mode.Also applies to: 137-142, 215-220
162-183: Winner-mode validation coverage is strong.These tests correctly assert that winner mode rejects
RejectDecisionand unknown winner IDs, covering key guardrails of the new processor API.
185-188: Invalid-mode constructor test is a good defensive addition.This captures the invalid discriminator path and protects the constructor contract from regressions.
Two valid CodeRabbit Major findings, both applied. - src/synthorg/settings/dispatcher.py: abort start()'s recovery path when the unsubscribe of a crashed-task registration fails. Before this fix the helper logged a WARNING and continued past the unsubscribe; the subsequent subscribe() at the bottom of start() would then double-register on bus implementations whose subscribe is not idempotent (NATS), leaving two __settings_dispatcher__ subscriptions live and double-delivering every settings change. The recovery path now logs ERROR, sets _stop_failed=True so the dispatcher is unrestartable, and raises RuntimeError so start() exits before subscribe() runs again. - src/synthorg/settings/dispatcher.py: clear self._lifecycle_lock at the end of stop() so a subsequent start() on a different event loop binds a fresh asyncio.Lock. Without this, stop() set _task to None but left _lifecycle_lock bound to the old loop; start()'s cross-loop guard (_task_is_on_current_loop + _drop_stale_loop_state) only fires when _task is non-None, so the next start() would re-enter the lifecycle-lock async-with on a Lock bound to the closed loop and raise the canonical 'is bound to a different event loop' RuntimeError -- exactly the cross-loop scenario the lazy-init pattern was meant to handle.
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - Password and secret fields now include an eye-toggle for easier visibility control. - Containers running without probes are shown as healthy in the doctor command. - Unloaded and missing PR-review agents are restored and available again. ### What's new - Gate baseline protection is enhanced to block em-dashes during writing. ### Under the hood - Replaced Atlas with yoyo-migrations for persistence management. - Refactored codebase extensively, including context-bound user authentication and registry pattern for enums. - Improved linting by draining magic number usages and tightening mock and constant checks. - Updated CI to retry Docker pushes on network timeout errors. - Updated apko lockfiles for dependency management. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.3](v0.8.2...v0.8.3) (2026-05-12) ### Features * harden gate baseline protection + block em-dashes at write time ([#1860](#1860)) ([b41f151](b41f151)) * **web:** eye-toggle on every password / secret field ([#1873](#1873)) ([9070387](9070387)) ### Bug Fixes * **ci:** retry Docker push on Go net/http deadline + cancellation errors ([#1877](#1877)) ([23a0bfa](23a0bfa)) * **cli:** render running-no-probe containers as healthy in doctor ([#1870](#1870)) ([6263795](6263795)) * restore unloaded and missing PR-review agents ([#1875](#1875)) ([db004fd](db004fd)), closes [#1871](#1871) ### Refactoring * bind authenticated user via ContextVar ([#1858](#1858)) ([57ed0b4](57ed0b4)) * code-structure cleanup (sub-tasks D + F + G + H + I) ([#1859](#1859)) ([362e5c8](362e5c8)) * convert enum dispatch to registry pattern ([#1854](#1854)) ([e90550e](e90550e)) * drain no_magic_numbers baseline to zero via Final hoists ([#1856](#1856) phase 2) ([#1872](#1872)) ([ec8109e](ec8109e)) * drain pagination + loop-init + kill-switch baselines ([#1857](#1857)) ([#1868](#1868)) ([115c3c2](115c3c2)) * **persistence:** replace Atlas with yoyo-migrations ([#1876](#1876)) ([1b7e975](1b7e975)), closes [#1874](#1874) * protocols audit follow-up (REVIEW + fold pass) ([#1869](#1869)) ([af33ddb](af33ddb)) * protocols audit follow-up REMOVE pass ([#1867](#1867)) ([dd1eebc](dd1eebc)) * tighten check_mock_spec gate, add mock_of[T], drain baseline ([#1862](#1862)) ([240a253](240a253)) * tighten check_no_magic_numbers for named module constants ([#1856](#1856)) ([#1866](#1866)) ([90c933b](90c933b)) ### CI/CD * update apko lockfiles ([#1863](#1863)) ([2bd32e6](2bd32e6)) --- 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>
2 participants
Refs #1850.
This PR addresses sub-tasks D + F + G + H + I of #1850. Sub-tasks A + E shipped in #1843; the remaining five were orphaned. The umbrella issue stays open until every sub-task is checked off, so this PR uses
Refs #1850(NOTCloses) to avoid re-triggering the auto-close pitfall that closed #1778 prematurely.Sub-task D — Remove
openapi-typescriptThe 2026-04 audit framing was "migrate to openapi-typescript" but verified state contradicted that intent: the dep was already installed, only 2 files imported the bundle, and 31 hand-written type files coexisted alongside it. Hand-written types win.
openapi-typescript@7.13.0fromweb/package.jsondevDependencies.generate-types/pretype-check/prebuildnpm scripts.web/src/api/types/enums.ts(16 enum types) to hand-written(typeof FOO_VALUES)[number]union literals; runtime arrays become the single source of truth.web/src/api/endpoints/reports.ts(3 types) to hand-written shapes; values cross-checked against the Pydantic source atsrc/synthorg/budget/report_config.py:13andsrc/synthorg/api/controllers/reports.py:35,47.web/package-lock.json(19 transitive deps removed).web/src/api/types/generated.d.tsline from.gitignore.Sub-task F —
compare_cihelper + callsite migrationThe audit assumed
compare_cialready existed; it didn't.compare_ci(a: str, b: str) -> booltosrc/synthorg/core/normalization.py(Unicode-safe viacasefold(), whitespace-stripping, idempotent). Hypothesis property test pinscompare_ci(a, b) == (normalize_identifier(a) == normalize_identifier(b))..lower() ==/.casefold() ==/.lower() !=callsites:compare_ci(...)migrations (single-string equality + filter-by-name predicates).find_by_name_ci(...)migrations collapsing private_find_*helpers inorg_mutations.pyandhr/registry.py:206.csrf.py:179/:205,etag.py:219/:361) stay inline with a one-line exemption comment;compare_ciisstr-only by design.Sub-task G — Collapse Winner+Hybrid processors
The two processor classes had near-identical bodies and differed only in whether they accepted
RejectDecision.WinnerSelectProcessorandHybridDecisionProcessorwith a singleHumanDecisionProcessor(mode: Literal["winner", "hybrid"])atsrc/synthorg/communication/conflict_resolution/escalation/processors.py.modeproperty exposes the discriminator for factory introspection without leaking_mode.modevalidation (defensive; raisesValueErroron invalid values).decided_byparameter typed asNotBlankStrto match theDecisionProcessorProtocol._DECISION_PROCESSOR_FACTORIES) becomes lambda-keyed to construct the rightmode.escalation/__init__.pyre-exports the new class;human_strategy.pydefault switched.processor.mode == "winner" / "hybrid"(stronger than the priorisinstancecheck); error message regex still matches the verbatim 422-translation text.Sub-task H —
AuthServiceclass docstringReplace the 1-line docstring at
src/synthorg/api/auth/service.py:52with a structured docstring covering: purpose, async vs sync rationale (Argon2 ops useasyncio.to_thread; refresh-token persist awaits a repo write; everything else is sync), thread-safety (frozen_config+ statelessargon2/jwt; safe to share across the request-handler pool), and out-of-scope (token revocation, session storage, SYSTEM-token minting).Sub-task I — Protocols audit (audit-only, no removals)
237
Protocolclasses inventoried atdocs/reference/protocols-audit.md. Each entry recorded with module path, line,@runtime_checkablestatus, direct-inheritance count, and test-usage count. Recommendations:testuse ≥ 3)._PrivatePrefixedtyping seams; intent inspection needed).isinstancesites).Cleanup itself is intentionally out of scope; follow-up issues will be filed per area after this PR lands so each can hyperlink to a specific row in the merged doc.
Pre-PR review
13 review agents ran (10 main roster + 3 mini-pass). 12 findings consolidated, 6 implemented in the post-review
refactor: apply pre-PR review feedbackcommit:protocols-audit.mdto the CLAUDE.md "Reference (load on demand)" list.HumanDecisionProcessor.decided_by: str→NotBlankStrin the new processor.issue #1687reference inweb/src/__tests__/pages/new-surfaces.test.tsx.msgwithHR_TRAINING_SELECTOR_CONFIG_INVALIDevent constant in two source selectors.SettingsChangeDispatcher._runningreset through_lifecycle_lockvia a scheduled_reset_running_under_lockcoroutine; tests wait on the new_post_done_tasksset.2 false positives rejected (PEP 758
except A, B:syntax flagged by an agent unfamiliar with Python 3.12+; security "redundant whitespace stripping" notes self-classified as Safe).Issue-resolution-verifier confirmed all 5 sub-tasks RESOLVED at 99-100% confidence.
Test plan
uv run ruff check src/ tests/— clean.uv run mypy src/ tests/—Success: no issues found in 3698 source files.uv run python -m pytest tests/ -m unit— 23,745 passed (6 pre-existing flakes confirmed on clean main).-n8 --dist=loadfileschedulers.npm --prefix web run lint+type-check+test— 3,047 web tests pass.npm --prefix web run build— production build succeeds (864ms).openapi-typescript/WinnerSelectProcessor/HybridDecisionProcessorreferences and only the 4 documented bytes-typed sites carry the inline.lower() ==/.lower() !=pattern.Local cleanup note
After pulling this branch, contributors should delete any stale
web/src/api/types/generated.d.tsfrom their worktree — it is no longer produced and is no longer gitignored.