feat: deep CEO interview to project charter#2045
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a 'deep-intake' conversational flow that enables users to transform vague product ideas into structured, actionable project charters. By leveraging an LLM-driven interview process, the system captures project goals, constraints, and budget envelopes, persisting them as reviewable artifacts. Upon approval, the system automatically handles project provisioning, budget forecasting, and pipeline dispatch, streamlining the transition from concept to execution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Caution Review failedPull request was closed or merged during review WalkthroughIntroduces a complete project-charter subsystem: settings and enums, domain models and errors, LLM interview strategy and prompt, CharterInterviewService and CharterDispatcher, persistence protocol plus SQLite/Postgres repos and migrations, factory wiring, API controller (/meta/charters) and MCP tools/handlers, app startup/state facades and rate limits, observability events, web types/endpoints/store/pages/components and MSW mocks, and extensive unit/conformance/e2e tests. Separately, updates the isolation-gate script to classify xdist node-down with non-zero exit as a crash_advisory. |
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 implements the project charter subsystem, facilitating a structured requirements-elicitation interview process that generates reviewable and approvable project charters. The implementation includes a multi-turn orchestration service, a dispatcher for work-pipeline integration, and a comprehensive API and MCP tool surface supported by dual-backend persistence. Additionally, the test runner is updated to handle Python 3.14 teardown crashes on Windows as advisories. Feedback identifies a logic risk where the system might return stale charter objects if a database fetch fails immediately following a successful state transition; it is recommended to raise an explicit error in these cases to prevent inconsistent state propagation.
| return CharterApprovalResult( | ||
| charter=approved if approved is not None else charter, | ||
| project_id=project_id, | ||
| task_id=result.task_id, | ||
| is_success=result.is_success, | ||
| ) |
There was a problem hiding this comment.
There's a potential issue here where a stale charter object could be returned in the CharterApprovalResult. If self._charter_repo.get(charter_id) returns None after a successful approval, the fallback charter object holds the state from before the approval (e.g., status=DRAFTED). This would send an inconsistent state to the client.
A successful transition_if in _stamp_approved should guarantee the charter exists. If get still returns None, it's a critical data inconsistency. It would be safer to raise an error in this unlikely event rather than returning a stale object.
approved = await self._charter_repo.get(charter_id)
if approved is None:
# This should be impossible if the state transition succeeded,
# and indicates a critical data inconsistency.
raise QueryError(f"Failed to re-fetch charter {charter_id} after approval")
return CharterApprovalResult(
charter=approved,
project_id=project_id,
task_id=result.task_id,
is_success=result.is_success,
)| to_state=CharterStatus.CANCELLED.value, | ||
| ) | ||
| refreshed = await self._charter_repo.get(charter_id) | ||
| return refreshed if refreshed is not None else charter |
There was a problem hiding this comment.
There's a potential issue here with returning a stale object. If self._charter_repo.get(charter_id) returns None after a successful cancellation, the fallback charter object is returned. This object is from before the cancellation and will have an incorrect status (e.g., DRAFTED).
To ensure the caller receives the correct, updated state, it would be safer to raise an error if the re-fetch fails, as this indicates a critical data inconsistency.
if refreshed is None:
# This should be impossible if the state transition succeeded.
raise QueryError(f"Failed to re-fetch charter {charter_id} after cancellation")
return refreshed![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/api/controllers/test_charter.py (1)
22-57: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd unwired degradation tests for patch/cancel endpoints as well.
This suite validates interview/list/get/approve paths, but the same 503 contract should be asserted for edit and cancel routes to keep endpoint degradation coverage complete.
🤖 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/api/controllers/test_charter.py` around lines 22 - 57, Add two unwired-degradation tests to TestCharterControllerUnwired: one asserting PATCH "/api/v1/meta/charters/charter-1" returns _SERVICE_UNAVAILABLE (name it test_patch_returns_503_when_unwired) and one asserting POST "/api/v1/meta/charters/charter-1/cancel" returns _SERVICE_UNAVAILABLE (name it test_cancel_returns_503_when_unwired); use the existing test_client fixture and mirror the style of test_approve_returns_503_when_dispatcher_unwired and test_interview_returns_503_when_unwired so the suite covers edit (PATCH) and cancel routes as well.tests/conformance/persistence/test_charter_repository.py (1)
349-375:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
test_constraint_violation_on_corrupt_raw_writedoes not actually validate a constraint violation.The test never executes a failing write/update path, so it can’t catch regressions in DB-level approval coupling.
Concrete test fix
async def test_constraint_violation_on_corrupt_raw_write( self, backend: PersistenceBackend ) -> None: - """A drafted row carrying approval provenance trips the DB CHECK. - - The model forbids this, so the repo path cannot emit it; this - guards the DB-level approval-coupling constraint by asserting a - clean drafted save (no provenance) succeeds while an approved - save requires full provenance via the model + DB together. - """ + """DB CHECK rejects incomplete provenance for APPROVED transition.""" repo = _repo(backend) - approved = _make_charter( - charter_id="ap1", - status=CharterStatus.APPROVED, - approved_at=_NOW, - approved_by="user-1", - forecast_id=uuid4(), - correlation_id="conv-1", - task_id="task-1", - ) - await repo.save(approved) - fetched = await repo.get(NotBlankStr("ap1")) - assert fetched is not None - assert fetched.status is CharterStatus.APPROVED - # Sanity: a constraint violation type exists for raw corrupt writes. - assert issubclass(ConstraintViolationError, Exception) + await repo.save(_make_charter(charter_id="ap1")) + with pytest.raises(ConstraintViolationError): + await repo.transition_if( + NotBlankStr("ap1"), + CharterStatus.DRAFTED, + CharterStatus.APPROVED, + updated_at=_NOW, + approved_at=_NOW, + approved_by="user-1", + # forecast_id / correlation_id / task_id intentionally omitted + )🤖 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/conformance/persistence/test_charter_repository.py` around lines 349 - 375, The test currently never performs a failing DB write; update test_constraint_violation_on_corrupt_raw_write to perform a raw/bypassing-model insert that violates the approval-coupling CHECK (e.g. insert a row with status=DRAFT but with approved_at/approved_by set) using the lower-level backend execution path (use the provided backend/DB execute/raw SQL helper available in the test harness) and assert that this operation raises ConstraintViolationError; keep the existing setup using repo/_repo and _make_charter, then run the raw INSERT against the charters table (via backend.execute or the test-suite raw SQL helper) and assert raises ConstraintViolationError to validate DB-level constraint enforcement.
🤖 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/run_affected_tests.py`:
- Around line 638-646: The repeated-crash gate is only counting entries with
test ids from _parse_worker_crashes(), so bare "[gwN] node down" worker deaths
get downgraded to advisory; update _parse_worker_crashes() (and any consumer
like the repeated-crash check function) to also record and key plain "node down"
events (e.g., by worker id or crash signature) so they contribute to the
repeated-crash detection, and then ensure the repeated-crash guard treats
repeated worker-down entries the same as entries tied to test ids (i.e., fail
the run rather than mark advisory).
In `@src/synthorg/api/app.py`:
- Around line 1691-1784: The _wire_charter_engine startup hook is meant to be
best-effort but can raise unexpected exceptions during settings/repo/strategy
construction and abort startup; wrap the body of _wire_charter_engine in a broad
try/except that catches Exception, logs the exception with contextual
information (include CHARTER_SUBSTRATE_UNAVAILABLE or similar and the exception
details), and returns without re-raising so startup continues; ensure you still
preserve idempotency (checks using app_state.has_charter_service remain before
constructing) and that exceptions around build_charter_repository,
build_conversational_repositories, build_charter_interview_strategy,
app_state.set_charter_service, and app_state.set_charter_dispatcher are all
guarded.
In `@src/synthorg/meta/charter/dispatch.py`:
- Around line 268-273: The log emits CHARTER_STATUS_TRANSITIONED even though no
state change was persisted; change this to a non-transition telemetry event
(e.g., CHARTER_DUPLICATE_PROJECT_RETRY or CHARTER_PROJECT_ALREADY_EXISTS) and
update the logger.info call (the one that currently passes charter_id=charter.id
and project_id=project_id) to use that new/appropriate constant so transition
events remain reserved for post-persistence state changes and this branch
accurately records a duplicate-project retry.
- Around line 198-211: The dispatch currently calls
self._work_pipeline.run(work_item) before performing the CAS winner check
transition_if(... DRAFTED -> APPROVED ...), which can cause duplicate side
effects in races; change the flow so you first attempt the atomic
transition_if(...) and only when it returns success do you call await
self._work_pipeline.run(work_item), and then call await
self._stamp_approved(...). Update the surrounding error handling and logging so
that pipeline errors are caught and logged as before but do not trigger if the
transition_if failed (abort/return early when transition lost), and apply the
same change to the analogous block that uses _work_pipeline.run and
_stamp_approved in the other occurrence.
- Around line 361-370: The _close_conversation method can raise from
self._conversation_repo.transition_if and currently bubbles up; wrap the call to
conversation_repo.transition_if in a try/except that catches Exception, logs the
failure (including conversation_id and now) via the module/class logger (e.g.
self._logger.exception or self._logger.error with details) and then swallows the
exception so the method remains best-effort and idempotent; keep the
ConversationStatus.ACTIVE -> CLOSED transition and updated_at usage intact but
do not re-raise on failure.
In `@src/synthorg/meta/charter/service.py`:
- Around line 114-130: The per-process asyncio locks created in _lock_for do not
prevent concurrent run_turn executions across multiple workers; replace the
in-memory lock strategy with a cross-process locking mechanism keyed by
conversation_id (e.g., a Redis-based distributed lock or a database advisory/row
lock) so sequencing and next_sequence generation are serialized cluster-wide;
update _lock_for (and any callers such as run_turn and the code handling
next_sequence) to acquire and release the distributed lock (using the
conversation_id as the key) and fall back to a safe DB transaction (SELECT ...
FOR UPDATE or an advisory lock) when Redis is unavailable to ensure only one
worker computes/inserts sequence values at a time.
In `@src/synthorg/meta/mcp/handlers/charter.py`:
- Around line 206-212: The cancel flow currently calls
svc.cancel_charter(charter_id, cancelled_by=_actor_id(actor),
enforce_ownership=False) without local admin checks; update the handler in
charter.py to invoke require_admin_guardrails() (the local admin guard) before
calling svc.cancel_charter with enforce_ownership=False so that only callers who
pass the admin guard can bypass ownership; locate the call site using
cancel_charter and _actor_id(actor) and ensure the require_admin_guardrails()
call appears immediately prior to the service call (or else change
enforce_ownership to True and let the service-layer guardrails handle admin
bypass).
In `@src/synthorg/persistence/sqlite/schema.sql`:
- Line 1786: project_charters allows dangling references; add proper foreign key
constraints so conversation_id and project_id reference their parent tables to
prevent orphan charters. Edit the CREATE TABLE for project_charters to add
FOREIGN KEY(conversation_id) REFERENCES conversations(id) and FOREIGN
KEY(project_id) REFERENCES projects(id) (choose ON DELETE CASCADE if you want
charters removed when a parent is deleted, or ON DELETE RESTRICT to prevent
parent deletion while charters exist), and ensure the existing CHECK/NOT NULL
definitions for conversation_id/project_id remain; also add matching indexes if
not present.
In `@tests/e2e/test_charter_to_run_e2e.py`:
- Around line 454-456: Ensure the test asserts there is exactly one forecast
before selecting it: add an assertion checking the cardinality of
forecast_repo.items (e.g., assert len(forecast_repo.items) == 1) prior to
retrieving the single forecast with next(iter(...)); then proceed to assert
forecast.decision is ForecastDecision.APPROVED and forecast.ceiling_amount ==
pytest.approx(_AMOUNT) to avoid silently masking duplicate writes when using
forecast_repo, items, and forecast variables.
In `@tests/unit/meta/charter/test_dispatch.py`:
- Around line 329-333: Tighten the assertion to check the structured charter id
as well as the event: modify the any(...) check over log_records to require
record.get("event") == CHARTER_DISPATCH_FAILED and record.get("charter_id") ==
charter.id (or the test's expected charter_id variable) so the test verifies
both the event name and the charter_id are present in the logged record.
In `@web/src/api/endpoints/charter.ts`:
- Around line 11-16: The CharterFilters interface currently exposes offset-based
paging (offset?: number) which violates the opaque cursor-based pagination
contract; replace offset with a cursor?: string (or afterCursor/nextCursor as
your project uses) and ensure callers/store code using CharterFilters and the
list endpoint consume/produce the opaque cursor (PaginationMeta) instead of
arithmetic offsets—update the CharterFilters type, any call-sites that pass
offset to listCharters/list endpoint, and the endpoint/handler that returns
PaginationMeta (nextCursor/hasMore) so the client and store use the cursor
string and limit for paging.
In `@web/src/mocks/handlers/charter.ts`:
- Line 34: The mock fixture currently hardcodes the currency field to 'USD';
replace that literal with the project-wide regional default currency
constant/function instead of embedding a locale-specific value. Import and use
the exported identifier (e.g., DEFAULT_CURRENCY or getDefaultCurrency) from the
shared regional/config fixture and assign it to the currency property in this
charter mock (currency), adding a safe fallback only if the import could be
undefined. Update the import and the currency assignment in
web/src/mocks/handlers/charter.ts so the fixture uses the centralized regional
default rather than the hardcoded 'USD'.
- Line 90: The mock handler always returns a hard-coded project_id
('charter-charter-default'); change it to derive the ID from the request params
so responses match the requested charter ID—use the handler's request param
(req.params.id) when building the approve response's project_id (e.g., replace
the static 'project_id' value in the charter mock response with the value from
req.params.id or a format that incorporates it) so tests and callers see a
consistent ID; update the code in web/src/mocks/handlers/charter.ts where
project_id is set.
In `@web/src/pages/charter/CharterDraftCard.tsx`:
- Around line 44-49: CharterDraftCard currently initializes local state (brief,
amount) only once and can become stale when the parent supplies a new charter;
add a useEffect in CharterDraftCard that watches the incoming charter identity
(e.g., charter.id and charter.version or the whole charter object) and calls
setBrief(charter.brief) and setAmount(String(charter.envelope.amount)) when
those change so the local brief/amount are resynchronised; keep existing
parsedAmount/amountValid/dirty logic but ensure the effect keys target the
charter identity (id/version) to avoid clobbering in-progress edits for the same
charter.
In `@web/src/pages/CharterInterviewPage.tsx`:
- Line 64: The "New interview" Button in CharterInterviewPage.tsx currently
disables only on the local sending flag which allows resetInterview to race with
ongoing charter mutations; update the Button's disabled gating to include the
mutation state (combine sending and mutating) so the button is disabled while a
charter mutation is in flight (reference the Button element, the resetInterview
handler, and the sending and mutating variables).
In `@web/src/stores/charter.ts`:
- Around line 71-114: The runTurn handler allows overlapping calls which lets a
failed earlier turn overwrite a later successful turn when it restores
previousMessages; fix by tagging the optimistic user message with a unique turn
id (create optimisticMessageId before set) and store the currentTurnId in state
(or keep optimisticMessageId in a local closure and also set it in state), then
on error only restore previousMessages if the stored
currentTurnId/lastOptimisticId still matches the id created for this runTurn
invocation; update references in runTurn that set sending/messages and the catch
block restore logic to check that id match before calling set({ sending:false,
messages: previousMessages }) so concurrent turns do not clobber newer
transcript state.
---
Outside diff comments:
In `@tests/conformance/persistence/test_charter_repository.py`:
- Around line 349-375: The test currently never performs a failing DB write;
update test_constraint_violation_on_corrupt_raw_write to perform a
raw/bypassing-model insert that violates the approval-coupling CHECK (e.g.
insert a row with status=DRAFT but with approved_at/approved_by set) using the
lower-level backend execution path (use the provided backend/DB execute/raw SQL
helper available in the test harness) and assert that this operation raises
ConstraintViolationError; keep the existing setup using repo/_repo and
_make_charter, then run the raw INSERT against the charters table (via
backend.execute or the test-suite raw SQL helper) and assert raises
ConstraintViolationError to validate DB-level constraint enforcement.
In `@tests/unit/api/controllers/test_charter.py`:
- Around line 22-57: Add two unwired-degradation tests to
TestCharterControllerUnwired: one asserting PATCH
"/api/v1/meta/charters/charter-1" returns _SERVICE_UNAVAILABLE (name it
test_patch_returns_503_when_unwired) and one asserting POST
"/api/v1/meta/charters/charter-1/cancel" returns _SERVICE_UNAVAILABLE (name it
test_cancel_returns_503_when_unwired); use the existing test_client fixture and
mirror the style of test_approve_returns_503_when_dispatcher_unwired and
test_interview_returns_503_when_unwired so the suite covers edit (PATCH) and
cancel routes as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67514ae6-1b02-4c08-97e4-8df1fa04147c
📒 Files selected for processing (75)
scripts/_ghost_wiring_manifest.txtscripts/run_affected_tests.pysrc/synthorg/api/app.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/api/rate_limits/policies.pysrc/synthorg/api/state.pysrc/synthorg/api/state_services_facades.pysrc/synthorg/api/state_services_facades_mcp3.pysrc/synthorg/core/enums.pysrc/synthorg/core/error_taxonomy.pysrc/synthorg/meta/charter/__init__.pysrc/synthorg/meta/charter/config.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/factory.pysrc/synthorg/meta/charter/models.pysrc/synthorg/meta/charter/prompts.pysrc/synthorg/meta/charter/service.pysrc/synthorg/meta/charter/strategy.pysrc/synthorg/meta/config.pysrc/synthorg/meta/errors.pysrc/synthorg/meta/mcp/domains/__init__.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/observability/events/charter.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/persistence/charter_factory.pysrc/synthorg/persistence/charter_protocol.pysrc/synthorg/persistence/postgres/charter_repo.pysrc/synthorg/persistence/postgres/revisions/20260522000002_project_charters.sqlsrc/synthorg/persistence/postgres/schema.sqlsrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/persistence/sqlite/revisions/20260522000002_project_charters.sqlsrc/synthorg/persistence/sqlite/schema.sqlsrc/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/definitions/charter.pysrc/synthorg/settings/enums.pytests/conformance/persistence/test_charter_repository.pytests/e2e/test_charter_to_run_e2e.pytests/unit/api/conftest.pytests/unit/api/controllers/test_charter.pytests/unit/conftest.pytests/unit/meta/charter/__init__.pytests/unit/meta/charter/test_dispatch.pytests/unit/meta/charter/test_factory.pytests/unit/meta/charter/test_models.pytests/unit/meta/charter/test_service.pytests/unit/meta/charter/test_strategy.pytests/unit/meta/mcp/handlers/test_charter.pytests/unit/meta/mcp/test_all_handlers_wired.pytests/unit/observability/test_events.pytests/unit/scripts/test_run_affected_tests.pyweb/src/__tests__/pages/CharterInterviewPage.test.tsxweb/src/__tests__/stores/charter.test.tsweb/src/api/endpoints/charter.tsweb/src/api/endpoints/index.tsweb/src/api/types/dtos.gen.tsweb/src/api/types/enum-values.gen.tsweb/src/api/types/error-codes.gen.tsweb/src/api/types/openapi.gen.tsweb/src/components/layout/Sidebar.tsxweb/src/mocks/handlers/charter.tsweb/src/mocks/handlers/index.tsweb/src/pages/CharterInterviewPage.tsxweb/src/pages/charter/CharterDraftCard.tsxweb/src/pages/charter/InterviewChat.tsxweb/src/pages/settings/utils.tsweb/src/router/index.tsxweb/src/router/route-titles.tsweb/src/router/routes.tsweb/src/stores/charter.tsweb/src/utils/constants.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). (13)
- GitHub Check: Deploy Preview
- GitHub Check: Build Backend
- GitHub Check: Dashboard Test
- GitHub Check: Test Conformance (SQLite)
- GitHub Check: Test Unit
- GitHub Check: Test Integration
- GitHub Check: Test E2E
- GitHub Check: CodSpeed Web benchmarks
- GitHub Check: CodSpeed Python benchmarks
- GitHub Check: Lighthouse Dashboard
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (19)
web/src/**/*.{js,jsx,ts,tsx,mts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{js,jsx,ts,tsx,mts}: Always usecreateLoggerfrom@/lib/logger; never bareconsole.warn/console.error/console.debugin application code. Variable name must always belog. Onlylogger.tsitself may use bare console methods. Uselog.debug()(DEV-only, stripped in production),log.warn(),log.error().
Pass dynamic/untrusted values as separate args to logger calls (not interpolated into the message string) so they go throughsanitizeArg
Attacker-controlled fields inside structured objects must be wrapped insanitizeForLog()before embedding in log calls
Error-code constants (MANDATORY): importErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported from the generatedweb/src/api/types/error-codes.gen.ts). Discriminate onErrorCode.<NAME>, never on raw integer literals.
Use@eslint-react/web-api-no-leaked-fetchto detectfetch()in effects withoutAbortControllercleanup
Files:
web/src/api/endpoints/index.tsweb/src/router/routes.tsweb/src/components/layout/Sidebar.tsxweb/src/router/index.tsxweb/src/pages/settings/utils.tsweb/src/router/route-titles.tsweb/src/__tests__/pages/CharterInterviewPage.test.tsxweb/src/utils/constants.tsweb/src/pages/charter/InterviewChat.tsxweb/src/pages/CharterInterviewPage.tsxweb/src/mocks/handlers/charter.tsweb/src/api/types/enum-values.gen.tsweb/src/stores/charter.tsweb/src/pages/charter/CharterDraftCard.tsxweb/src/api/types/error-codes.gen.tsweb/src/__tests__/stores/charter.test.tsweb/src/api/endpoints/charter.tsweb/src/api/types/dtos.gen.tsweb/src/mocks/handlers/index.tsweb/src/api/types/openapi.gen.ts
web/src/{api/endpoints,stores}/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Cursor pagination (MANDATORY): list endpoints must use opaque cursor-based paging via
PaginationMeta. Stores must keepnextCursor+hasMorein state (not offset arithmetic) and early-return when!hasMore || !nextCursor. Display counts must come fromdata.length.
Files:
web/src/api/endpoints/index.tsweb/src/stores/charter.tsweb/src/api/endpoints/charter.ts
web/src/api/endpoints/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Health / readiness endpoints (MANDATORY):
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state). Any new caller must handle the 503 path explicitly.
Files:
web/src/api/endpoints/index.tsweb/src/api/endpoints/charter.ts
web/src/**/*.{ts,tsx,mts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,mts}: Use@typescript-eslint/no-floating-promisesto forbid unawaited promises so async work cannot survive the test that scheduled it and trip the active-handle gate
Use@typescript-eslint/no-misused-promises(withchecksVoidReturn: { attributes: false }) to forbid passing async functions where the callsite ignores the returned promise. React 19asyncevent handlers stay allowed via theattributes: falseexemption.
Files:
web/src/api/endpoints/index.tsweb/src/router/routes.tsweb/src/components/layout/Sidebar.tsxweb/src/router/index.tsxweb/src/pages/settings/utils.tsweb/src/router/route-titles.tsweb/src/__tests__/pages/CharterInterviewPage.test.tsxweb/src/utils/constants.tsweb/src/pages/charter/InterviewChat.tsxweb/src/pages/CharterInterviewPage.tsxweb/src/mocks/handlers/charter.tsweb/src/api/types/enum-values.gen.tsweb/src/stores/charter.tsweb/src/pages/charter/CharterDraftCard.tsxweb/src/api/types/error-codes.gen.tsweb/src/__tests__/stores/charter.test.tsweb/src/api/endpoints/charter.tsweb/src/api/types/dtos.gen.tsweb/src/mocks/handlers/index.tsweb/src/api/types/openapi.gen.ts
**/*.{py,ts,tsx,jsx,md}
📄 CodeRabbit inference engine (CLAUDE.md)
No region/currency/locale privileged; use metric units; British English per docs/reference/regional-defaults.md
Files:
web/src/api/endpoints/index.tssrc/synthorg/meta/charter/factory.pyweb/src/router/routes.tssrc/synthorg/observability/events/charter.pyweb/src/components/layout/Sidebar.tsxweb/src/router/index.tsxweb/src/pages/settings/utils.tssrc/synthorg/core/enums.pyweb/src/router/route-titles.tssrc/synthorg/settings/definitions/__init__.pyweb/src/__tests__/pages/CharterInterviewPage.test.tsxsrc/synthorg/meta/mcp/domains/__init__.pyweb/src/utils/constants.tssrc/synthorg/meta/charter/prompts.pyweb/src/pages/charter/InterviewChat.tsxtests/unit/meta/charter/test_factory.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/rate_limits/policies.pyweb/src/pages/CharterInterviewPage.tsxtests/unit/conftest.pysrc/synthorg/meta/charter/__init__.pysrc/synthorg/api/state.pyweb/src/mocks/handlers/charter.tstests/unit/api/conftest.pysrc/synthorg/persistence/charter_factory.pytests/unit/observability/test_events.pyweb/src/api/types/enum-values.gen.tssrc/synthorg/meta/charter/config.pysrc/synthorg/api/controllers/__init__.pyweb/src/stores/charter.tstests/unit/api/controllers/test_charter.pysrc/synthorg/api/controllers/charter.pyweb/src/pages/charter/CharterDraftCard.tsxtests/unit/meta/charter/test_strategy.pytests/unit/meta/mcp/test_all_handlers_wired.pyweb/src/api/types/error-codes.gen.tssrc/synthorg/observability/prometheus_labels.pysrc/synthorg/api/state_services_facades_mcp3.pyweb/src/__tests__/stores/charter.test.tssrc/synthorg/settings/enums.pyweb/src/api/endpoints/charter.tssrc/synthorg/meta/errors.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/api/state_services_facades.pysrc/synthorg/persistence/charter_protocol.pytests/unit/meta/mcp/handlers/test_charter.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/settings/definitions/charter.pysrc/synthorg/meta/charter/strategy.pytests/e2e/test_charter_to_run_e2e.pyweb/src/api/types/dtos.gen.tssrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/meta/mcp/handlers/charter.pytests/unit/meta/charter/test_service.pysrc/synthorg/core/error_taxonomy.pysrc/synthorg/api/app.pytests/unit/scripts/test_run_affected_tests.pytests/unit/meta/charter/test_models.pysrc/synthorg/meta/charter/dispatch.pytests/unit/meta/charter/test_dispatch.pyweb/src/mocks/handlers/index.tsscripts/run_affected_tests.pysrc/synthorg/meta/charter/service.pysrc/synthorg/persistence/postgres/charter_repo.pysrc/synthorg/meta/config.pytests/conformance/persistence/test_charter_repository.pyweb/src/api/types/openapi.gen.tssrc/synthorg/meta/charter/models.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Onlysrc/synthorg/persistence/may import sqlite/psycopg or emit raw SQL; new repository protocols inherit from generic categories inpersistence/_generics.py; bespoke methods permitted only under ADR-0001 D7
Configuration Precedence: DB > env > code default viaSettingsService/ConfigResolver(Cat-1) or env > code default (Cat-2,read_only_post_init); Cat-3 bootstrap secrets pure env; YAML is ingestion format only, not precedence tier; noos.environ.getoutside startup
No hardcoded numeric values; numerics live insettings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants (NAME: int|float|Final); enforced byscripts/check_no_magic_numbers.py
Comments document WHY only; no reviewer citations, issue back-refs, or migration framing; enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py
Nofrom __future__ import annotations(Python 3.14 has PEP 649); use PEP 758 except:except A, B:no parens unless binding
Type hints on public functions; mypy strict; Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Errors follow<Domain><Condition>Errorpattern fromDomainError; never inheritException/RuntimeErrordirectly; enforced bycheck_domain_error_hierarchy.py
Pydantic v2 frozen +extra="forbid"on every frozen model project-wide; gatecheck_frozen_model_extra_forbid.py;@computed_fieldauto-exempt; per-line# lint-allow: frozen-extra-forbid -- <reason>forextra="allow"/"ignore"boundaries; use@computed_fieldfor derived; useNotBlankStrfor identifiers
Args models at every system boundary;parse_typed()for every external dict ingestion; enforced bycheck_boundary_typed.py
Immutability: usemodel_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries
Async: useasyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/`RecursionError...
Files:
src/synthorg/meta/charter/factory.pysrc/synthorg/observability/events/charter.pysrc/synthorg/core/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/meta/mcp/domains/__init__.pysrc/synthorg/meta/charter/prompts.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/rate_limits/policies.pysrc/synthorg/meta/charter/__init__.pysrc/synthorg/api/state.pysrc/synthorg/persistence/charter_factory.pysrc/synthorg/meta/charter/config.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/api/state_services_facades_mcp3.pysrc/synthorg/settings/enums.pysrc/synthorg/meta/errors.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/api/state_services_facades.pysrc/synthorg/persistence/charter_protocol.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/settings/definitions/charter.pysrc/synthorg/meta/charter/strategy.pysrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/core/error_taxonomy.pysrc/synthorg/api/app.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.pysrc/synthorg/persistence/postgres/charter_repo.pysrc/synthorg/meta/config.pysrc/synthorg/meta/charter/models.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/meta/charter/factory.pysrc/synthorg/observability/events/charter.pysrc/synthorg/core/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/meta/mcp/domains/__init__.pysrc/synthorg/meta/charter/prompts.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/rate_limits/policies.pysrc/synthorg/meta/charter/__init__.pysrc/synthorg/api/state.pysrc/synthorg/persistence/charter_factory.pysrc/synthorg/meta/charter/config.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/api/state_services_facades_mcp3.pysrc/synthorg/settings/enums.pysrc/synthorg/meta/errors.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/api/state_services_facades.pysrc/synthorg/persistence/charter_protocol.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/settings/definitions/charter.pysrc/synthorg/meta/charter/strategy.pysrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/core/error_taxonomy.pysrc/synthorg/api/app.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.pysrc/synthorg/persistence/postgres/charter_repo.pysrc/synthorg/meta/config.pysrc/synthorg/meta/charter/models.py
web/src/{components,utils}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
NEVER write
getXIcon(value): LucideIconfactories called inside JSX bodies. Export a<XIcon value={...} />wrapper that does the lookup viacreateElementinside the wrapper body. Wrapper components live in their own file, not alongside utility exports.
Files:
web/src/components/layout/Sidebar.tsxweb/src/utils/constants.ts
web/src/{components,hooks}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
NEVER read
window.innerWidth/window.innerHeightdirectly in a render body oruseMemo; useuseViewportSize()from@/hooks/useViewportSizeinstead
Files:
web/src/components/layout/Sidebar.tsx
web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{jsx,tsx}: Use@eslint-react/no-leaked-conditional-renderingto catch the{count && <Foo />}bug where0renders verbatim. ForReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...).
Use@eslint-react/globalsto restrictwindow/document/localStorage/ etc. inside render. Hoist offenders into auseCallbackevent handler, auseEffect, or auseSyncExternalStore-backed hook.Reuse
web/src/components/ui/design tokens in Web Dashboard Design System; detail inweb/CLAUDE.md
Files:
web/src/components/layout/Sidebar.tsxweb/src/router/index.tsxweb/src/__tests__/pages/CharterInterviewPage.test.tsxweb/src/pages/charter/InterviewChat.tsxweb/src/pages/CharterInterviewPage.tsxweb/src/pages/charter/CharterDraftCard.tsx
web/src/{stores,**/*.test.{ts,tsx}}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Active-handle gate (MANDATORY): every unit test runs under
web/test-infra/active-handle-tracker.ts, which fails any test that leaks an event-loop-holding resource. A new store that schedules timers / attaches listeners MUST expose a teardown hook and register it in the globalafterEach; otherwise the gate fails the first test that triggers the schedule.
Files:
web/src/__tests__/pages/CharterInterviewPage.test.tsxweb/src/__tests__/stores/charter.test.ts
src/synthorg/meta/mcp/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
MCP: Define
ToolHandler+args_model; callrequire_admin_guardrails()on admin tools; route through service layers per mcp-handler-contract.md
Files:
src/synthorg/meta/mcp/domains/__init__.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/meta/mcp/handlers/charter.py
web/src/utils/constants.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
WS wire protocol (MANDATORY): the client-server contract lives in
web/src/utils/constants.ts(WS_PROTOCOL_VERSION,WS_MAX_MESSAGE_SIZE,WS_HEARTBEAT_INTERVAL_MS,WS_PONG_TIMEOUT_MS,LOG_SANITIZE_MAX_LENGTH) and MUST stay in lockstep withsrc/synthorg/api/ws_models.py/src/synthorg/api/controllers/ws.py. Bump the protocol version on both sides together for breaking payload changes.
Files:
web/src/utils/constants.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Timeout/slow failures = source-code regression; never edittests/baselines/unit_timing.jsonor anyscripts/*_baseline.{txt,json}/scripts/_*_baseline.py; both families PreToolUse-blocked; per-invocation bypass requires explicit approval (ALLOW_BASELINE_GROWTH=1 git commit)
Test markers:@pytest.mark.{unit,integration,e2e,slow}; async auto; timeout 30s global; coverage 80% min
xdist-n 8 --dist=loadfileauto-applied via pyprojectaddopts; Windows unit tests useWindowsSelectorEventLoopPolicy; subprocess tests override back
Test doubles:FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags; bareMagicMockat typed boundary blocked byscripts/check_mock_spec.py
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)); never skip/xfail flaky tests; fix fundamentally
Files:
tests/unit/meta/charter/test_factory.pytests/unit/conftest.pytests/unit/api/conftest.pytests/unit/observability/test_events.pytests/unit/api/controllers/test_charter.pytests/unit/meta/charter/test_strategy.pytests/unit/meta/mcp/test_all_handlers_wired.pytests/unit/meta/mcp/handlers/test_charter.pytests/e2e/test_charter_to_run_e2e.pytests/unit/meta/charter/test_service.pytests/unit/scripts/test_run_affected_tests.pytests/unit/meta/charter/test_models.pytests/unit/meta/charter/test_dispatch.pytests/conformance/persistence/test_charter_repository.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/meta/charter/test_factory.pytests/unit/conftest.pytests/unit/api/conftest.pytests/unit/observability/test_events.pytests/unit/api/controllers/test_charter.pytests/unit/meta/charter/test_strategy.pytests/unit/meta/mcp/test_all_handlers_wired.pytests/unit/meta/mcp/handlers/test_charter.pytests/e2e/test_charter_to_run_e2e.pytests/unit/meta/charter/test_service.pytests/unit/scripts/test_run_affected_tests.pytests/unit/meta/charter/test_models.pytests/unit/meta/charter/test_dispatch.pytests/conformance/persistence/test_charter_repository.py
web/src/mocks/handlers/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/mocks/handlers/**/*.ts: MSW handlers (MANDATORY):web/src/mocks/handlers/must mirrorweb/src/api/endpoints/*.ts1:1 with a default happy-path handler for every exported endpoint. UseonUnhandledRequest: 'error'in test setup; tests override per-case viaserver.use(...), nevervi.mock('@/api/endpoints/*').
Use typed envelope helpers (successFor,paginatedFor,voidSuccess) to keep MSW handlers in lockstep with endpoint return types
Files:
web/src/mocks/handlers/charter.tsweb/src/mocks/handlers/index.ts
src/synthorg/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/persistence/**/*.py: Repository CRUD:save(entity),get(id),delete(id) -> bool,list_items(...),query(...)returning tuples
Datetime in persistence: useparse_iso_utc/format_iso_utcfrompersistence._shared(reject naive); usenormalize_utcfor already-typed
Files:
src/synthorg/persistence/charter_factory.pysrc/synthorg/persistence/charter_protocol.pysrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/persistence/postgres/charter_repo.py
web/src/api/types/**/*.gen.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Generated DTO types (MANDATORY): NEVER hand-edit
web/src/api/types/*.gen.ts. Regenerate withuv run python scripts/generate_dto_types_ts.py. Import DTOs via the barrel (import type { AgentConfig } from '@/api/types').
Files:
web/src/api/types/enum-values.gen.tsweb/src/api/types/error-codes.gen.tsweb/src/api/types/dtos.gen.tsweb/src/api/types/openapi.gen.ts
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: List reads (fetch*) must seterror: string | nullon the store instead of toasting
Test teardown (MANDATORY): any new store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEach. The globalafterEachinweb/src/test-setup.tsxalready callsuseToastStore.getState().dismissAll(),cancelPendingPersist(), anduseThemeStore.getState().teardown().
Files:
web/src/stores/charter.ts
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres); enforced bycheck_dual_backend_test_parity.py
Files:
tests/conformance/persistence/test_charter_repository.py
🧠 Learnings (5)
📚 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/meta/charter/factory.pysrc/synthorg/observability/events/charter.pysrc/synthorg/core/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/meta/mcp/domains/__init__.pysrc/synthorg/meta/charter/prompts.pytests/unit/meta/charter/test_factory.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/rate_limits/policies.pytests/unit/conftest.pysrc/synthorg/meta/charter/__init__.pysrc/synthorg/api/state.pytests/unit/api/conftest.pysrc/synthorg/persistence/charter_factory.pytests/unit/observability/test_events.pysrc/synthorg/meta/charter/config.pysrc/synthorg/api/controllers/__init__.pytests/unit/api/controllers/test_charter.pysrc/synthorg/api/controllers/charter.pytests/unit/meta/charter/test_strategy.pytests/unit/meta/mcp/test_all_handlers_wired.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/api/state_services_facades_mcp3.pysrc/synthorg/settings/enums.pysrc/synthorg/meta/errors.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/api/state_services_facades.pysrc/synthorg/persistence/charter_protocol.pytests/unit/meta/mcp/handlers/test_charter.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/settings/definitions/charter.pysrc/synthorg/meta/charter/strategy.pytests/e2e/test_charter_to_run_e2e.pysrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/meta/mcp/handlers/charter.pytests/unit/meta/charter/test_service.pysrc/synthorg/core/error_taxonomy.pysrc/synthorg/api/app.pytests/unit/scripts/test_run_affected_tests.pytests/unit/meta/charter/test_models.pysrc/synthorg/meta/charter/dispatch.pytests/unit/meta/charter/test_dispatch.pyscripts/run_affected_tests.pysrc/synthorg/meta/charter/service.pysrc/synthorg/persistence/postgres/charter_repo.pysrc/synthorg/meta/config.pytests/conformance/persistence/test_charter_repository.pysrc/synthorg/meta/charter/models.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
src/synthorg/meta/charter/factory.pysrc/synthorg/observability/events/charter.pysrc/synthorg/core/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/meta/mcp/domains/__init__.pysrc/synthorg/meta/charter/prompts.pytests/unit/meta/charter/test_factory.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/rate_limits/policies.pytests/unit/conftest.pysrc/synthorg/meta/charter/__init__.pysrc/synthorg/api/state.pytests/unit/api/conftest.pysrc/synthorg/persistence/charter_factory.pytests/unit/observability/test_events.pysrc/synthorg/meta/charter/config.pysrc/synthorg/api/controllers/__init__.pytests/unit/api/controllers/test_charter.pysrc/synthorg/api/controllers/charter.pytests/unit/meta/charter/test_strategy.pytests/unit/meta/mcp/test_all_handlers_wired.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/api/state_services_facades_mcp3.pysrc/synthorg/settings/enums.pysrc/synthorg/meta/errors.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/api/state_services_facades.pysrc/synthorg/persistence/charter_protocol.pytests/unit/meta/mcp/handlers/test_charter.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/settings/definitions/charter.pysrc/synthorg/meta/charter/strategy.pytests/e2e/test_charter_to_run_e2e.pysrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/meta/mcp/handlers/charter.pytests/unit/meta/charter/test_service.pysrc/synthorg/core/error_taxonomy.pysrc/synthorg/api/app.pytests/unit/scripts/test_run_affected_tests.pytests/unit/meta/charter/test_models.pysrc/synthorg/meta/charter/dispatch.pytests/unit/meta/charter/test_dispatch.pyscripts/run_affected_tests.pysrc/synthorg/meta/charter/service.pysrc/synthorg/persistence/postgres/charter_repo.pysrc/synthorg/meta/config.pytests/conformance/persistence/test_charter_repository.pysrc/synthorg/meta/charter/models.py
📚 Learning: 2026-05-21T22:55:09.289Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/config.py:29-30
Timestamp: 2026-05-21T22:55:09.289Z
Learning: For this repo’s Pydantic configuration idiom, do not treat numeric literals passed directly as arguments to `pydantic.Field(...)` as “magic numbers” during review. This includes call-site usages like `Field(default=0.2, ge=0.0, le=1.0)` (e.g., in config models such as `ToolAuthoringConfig`, `ToolValidationConfig`, `ToolsmithConfig`). Do not request extracting those `Field(...)` numeric arguments into named constants, since the repo’s `scripts/check_no_magic_numbers.py` intentionally excludes call-site `Field(...)` numerics and relies on `Field(...)` as the canonical way to express these constraints/defaults.
Applied to files:
src/synthorg/meta/charter/factory.pysrc/synthorg/observability/events/charter.pysrc/synthorg/core/enums.pysrc/synthorg/settings/definitions/__init__.pysrc/synthorg/meta/mcp/domains/__init__.pysrc/synthorg/meta/charter/prompts.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/rate_limits/policies.pysrc/synthorg/meta/charter/__init__.pysrc/synthorg/api/state.pysrc/synthorg/persistence/charter_factory.pysrc/synthorg/meta/charter/config.pysrc/synthorg/api/controllers/__init__.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/observability/prometheus_labels.pysrc/synthorg/api/state_services_facades_mcp3.pysrc/synthorg/settings/enums.pysrc/synthorg/meta/errors.pysrc/synthorg/observability/events/persistence.pysrc/synthorg/meta/mcp/handlers/__init__.pysrc/synthorg/api/state_services_facades.pysrc/synthorg/persistence/charter_protocol.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/settings/definitions/charter.pysrc/synthorg/meta/charter/strategy.pysrc/synthorg/persistence/sqlite/charter_repo.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/core/error_taxonomy.pysrc/synthorg/api/app.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.pysrc/synthorg/persistence/postgres/charter_repo.pysrc/synthorg/meta/config.pysrc/synthorg/meta/charter/models.py
📚 Learning: 2026-05-17T11:45:11.839Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1952
File: src/synthorg/settings/definitions/api.py:594-638
Timestamp: 2026-05-17T11:45:11.839Z
Learning: In SynthOrg (Aureliolo/synthorg) pre-alpha, apply the strict no-backward-compat policy: any setting-key rename must be fully completed in the same change/PR with all repo callers updated, and you should not keep legacy aliases or compatibility fallbacks. When reviewing, do not flag a setting-key rename as a breaking upgrade hazard if the rename is repo-wide and fully implemented within the same PR.
Applied to files:
src/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/definitions/charter.py
📚 Learning: 2026-05-17T11:45:11.839Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1952
File: src/synthorg/settings/definitions/api.py:594-638
Timestamp: 2026-05-17T11:45:11.839Z
Learning: In this repository, SynthOrg is pre-alpha and uses a strict no-backward-compat policy for setting-key renames. When reviewing code under src/synthorg/settings, do NOT flag a setting-key rename as an “upgrade-safety” issue if the rename is complete/atomic in the same PR: all callers/usages of the old key are updated simultaneously, and the PR does not keep any legacy aliases, compatibility fallbacks, or migration/rollback paths for the old key.
Applied to files:
src/synthorg/settings/definitions/__init__.pysrc/synthorg/settings/enums.pysrc/synthorg/settings/definitions/charter.py
🪛 LanguageTool
scripts/_ghost_wiring_manifest.txt
[style] ~46-~46: Consider a different adjective to strengthen your wording.
Context: ...iew_enabled + provider switch; runs the deep CEO interview producing a ProjectCharte...
(DEEP_PROFOUND)
🔇 Additional comments (62)
src/synthorg/api/state.py (1)
231-232: LGTM!src/synthorg/api/state_services_facades.py (1)
178-179: LGTM!src/synthorg/api/state_services_facades_mcp3.py (1)
33-36: LGTM!Also applies to: 89-90, 263-307
src/synthorg/api/controllers/__init__.py (1)
26-26: LGTM!Also applies to: 130-130, 228-228
src/synthorg/api/controllers/charter.py (1)
54-277: LGTM!src/synthorg/api/rate_limits/policies.py (1)
129-132: LGTM!scripts/_ghost_wiring_manifest.txt (1)
46-47: LGTM!src/synthorg/meta/mcp/domains/__init__.py (1)
11-11: LGTM!Also applies to: 38-38
src/synthorg/meta/mcp/domains/_charter_args.py (1)
23-79: LGTM!src/synthorg/meta/mcp/domains/charter.py (1)
34-110: LGTM!src/synthorg/meta/mcp/handlers/__init__.py (1)
14-14: LGTM!Also applies to: 45-45
web/src/api/endpoints/index.ts (1)
8-8: LGTM!web/src/api/types/dtos.gen.ts (1)
47-47: LGTM!Also applies to: 68-68, 81-81, 158-158, 168-169, 283-284, 390-390, 438-438
web/src/api/types/enum-values.gen.ts (1)
137-142: LGTM!Also applies to: 658-658
web/src/api/types/error-codes.gen.ts (1)
51-51: LGTM!Also applies to: 71-72, 96-96
web/src/api/types/openapi.gen.ts (1)
2222-2307: LGTM!Also applies to: 4745-4746, 5239-5246, 5467-5474, 5587-5594, 5947-5954, 6671-6683, 6814-6844, 8212-8212, 8982-8997, 10917-10959, 11745-11751, 11910-11910, 18787-18977
web/src/components/layout/Sidebar.tsx (1)
7-7: LGTM!Also applies to: 233-233
web/src/pages/charter/InterviewChat.tsx (1)
1-87: LGTM!web/src/pages/settings/utils.ts (1)
35-35: LGTM!web/src/mocks/handlers/index.ts (1)
71-71: LGTM!Also applies to: 125-125, 176-176, 240-240
web/src/router/index.tsx (1)
20-20: LGTM!Also applies to: 143-146
web/src/router/route-titles.ts (1)
37-37: LGTM!web/src/router/routes.ts (1)
22-22: LGTM!web/src/stores/charter.ts (1)
1-70: LGTM!Also applies to: 117-181
web/src/utils/constants.ts (1)
152-152: LGTM!Also applies to: 180-180
tests/e2e/test_charter_to_run_e2e.py (1)
1-453: LGTM!Also applies to: 457-473
tests/unit/api/conftest.py (1)
91-97: LGTM!Also applies to: 716-719
tests/unit/conftest.py (1)
10-35: LGTM!tests/unit/meta/charter/test_dispatch.py (1)
1-328: LGTM!Also applies to: 334-410
tests/unit/meta/charter/test_factory.py (1)
1-28: LGTM!tests/unit/meta/charter/test_models.py (1)
1-238: LGTM!tests/unit/meta/charter/test_service.py (1)
1-507: LGTM!tests/unit/meta/charter/test_strategy.py (1)
1-94: LGTM!tests/unit/meta/mcp/handlers/test_charter.py (1)
1-270: LGTM!tests/unit/meta/mcp/test_all_handlers_wired.py (1)
214-223: LGTM!tests/unit/observability/test_events.py (1)
223-223: LGTM!tests/unit/scripts/test_run_affected_tests.py (1)
560-582: LGTM!web/src/__tests__/pages/CharterInterviewPage.test.tsx (1)
1-64: LGTM!web/src/__tests__/stores/charter.test.ts (1)
1-146: LGTM!src/synthorg/core/enums.py (1)
880-894: LGTM!src/synthorg/core/error_taxonomy.py (1)
101-101: LGTM!Also applies to: 123-124, 154-154
src/synthorg/observability/events/charter.py (1)
1-52: LGTM!src/synthorg/observability/events/persistence.py (1)
790-800: LGTM!src/synthorg/observability/prometheus_labels.py (1)
282-311: LGTM!src/synthorg/settings/definitions/__init__.py (1)
12-12: LGTM!Also applies to: 41-41
src/synthorg/settings/definitions/charter.py (1)
1-117: LGTM!src/synthorg/settings/enums.py (1)
38-38: LGTM!src/synthorg/meta/config.py (1)
13-13: LGTM!Also applies to: 274-275, 311-314
src/synthorg/persistence/charter_protocol.py (1)
1-146: LGTM!src/synthorg/persistence/charter_factory.py (1)
1-70: LGTM!src/synthorg/persistence/postgres/charter_repo.py (1)
1-533: LGTM!src/synthorg/persistence/postgres/revisions/20260522000002_project_charters.sql (1)
1-90: LGTM!src/synthorg/persistence/postgres/schema.sql (1)
1729-1806: LGTM!src/synthorg/persistence/sqlite/charter_repo.py (1)
1-568: LGTM!src/synthorg/persistence/sqlite/revisions/20260522000002_project_charters.sql (1)
1-103: LGTM!src/synthorg/meta/charter/models.py (1)
33-373: LGTM!src/synthorg/meta/charter/prompts.py (1)
9-94: LGTM!src/synthorg/meta/charter/strategy.py (1)
105-165: LGTM!src/synthorg/meta/charter/config.py (1)
34-76: LGTM!src/synthorg/meta/charter/factory.py (1)
25-53: LGTM!src/synthorg/meta/charter/__init__.py (1)
1-45: LGTM!src/synthorg/meta/errors.py (1)
148-267: LGTM!
| * Worker(s) went ``node down`` with a non-zero returncode (and no | ||
| real failure / repeated crash above) -> crash advisory. The | ||
| controller-side loadscope crash guard in ``tests/conftest.py`` | ||
| suppresses the downstream ``INTERNALERROR>`` the dead-worker | ||
| chain used to emit, and a worker killed mid-teardown can die | ||
| before pytest prints any summary, so neither signal is required | ||
| to recognise the documented Python 3.14 + Windows xdist teardown | ||
| crash. The repeated-named-crash check above still blocks a test | ||
| that crashes the worker on every run. |
There was a problem hiding this comment.
Don't downgrade repeated bare node down crashes to advisory.
This path exits 0 for any non-zero run that only emits [gwN] node down..., but the repeated-crash guard only counts _parse_worker_crashes() entries with test ids. If the same test tears a worker down on both --count 2 iterations and xdist never prints worker ... crashed while running ..., the gate will still pass as advisory. That breaks the stated contract that “every replay” worker crashes must block.
Also applies to: 683-707
🤖 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/run_affected_tests.py` around lines 638 - 646, The repeated-crash
gate is only counting entries with test ids from _parse_worker_crashes(), so
bare "[gwN] node down" worker deaths get downgraded to advisory; update
_parse_worker_crashes() (and any consumer like the repeated-crash check
function) to also record and key plain "node down" events (e.g., by worker id or
crash signature) so they contribute to the repeated-crash detection, and then
ensure the repeated-crash guard treats repeated worker-down entries the same as
entries tied to test ids (i.e., fail the run rather than mark advisory).
| try: | ||
| result = await self._work_pipeline.run(work_item) | ||
| except MemoryError, RecursionError: | ||
| raise | ||
| except Exception as exc: | ||
| logger.error( | ||
| CHARTER_DISPATCH_FAILED, | ||
| charter_id=charter_id, | ||
| error_type=type(exc).__name__, | ||
| error=safe_error_description(exc), | ||
| ) | ||
| raise | ||
|
|
||
| await self._stamp_approved(charter, forecast, result.task_id, approved_by, now) |
There was a problem hiding this comment.
Gate side effects before the decision CAS to prevent duplicate dispatches.
run(work_item) executes before the transition_if(... DRAFTED -> APPROVED ...) winner check. In a multi-worker race, two approvers can both dispatch work and only then one loses CAS, causing duplicate pipeline side effects.
Also applies to: 338-353
🤖 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/meta/charter/dispatch.py` around lines 198 - 211, The dispatch
currently calls self._work_pipeline.run(work_item) before performing the CAS
winner check transition_if(... DRAFTED -> APPROVED ...), which can cause
duplicate side effects in races; change the flow so you first attempt the atomic
transition_if(...) and only when it returns success do you call await
self._work_pipeline.run(work_item), and then call await
self._stamp_approved(...). Update the surrounding error handling and logging so
that pipeline errors are caught and logged as before but do not trigger if the
transition_if failed (abort/return early when transition lost), and apply the
same change to the analogous block that uses _work_pipeline.run and
_stamp_approved in the other occurrence.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/meta/charter/service.py`:
- Around line 531-535: Before raising CharterStateInconsistentError in the
branch where refreshed is None, emit a structured ERROR/WARNING log using the
CHARTER_STATE_INCONSISTENT event (import it from
synthorg.observability.events.charter) and include contextual fields like
charter_id and the refreshed value (and charter if available) so the
inconsistency is recorded; then raise
CharterStateInconsistentError(charter_id=charter_id) as before.
In `@web/src/stores/charter.ts`:
- Around line 86-101: fetchMoreCharters currently snapshots charters before
awaiting charterApi.listCharters and then writes charters: [...charters,
...page.data], which can clobber newer updates; fix by using a state-update
function or re-reading latest state before appending: after the await, obtain
the current charters (via get() or by passing a functional updater to set) and
then set charters to [...latestCharters, ...page.data], while still updating
nextCursor, hasMore and loading; reference fetchMoreCharters,
charterApi.listCharters, get(), set(), and the
charters/nextCursor/hasMore/loading fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0581e77-a130-48a4-a6c4-3708af2afec7
📒 Files selected for processing (23)
src/synthorg/api/app.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.pysrc/synthorg/meta/errors.pysrc/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/observability/events/charter.pytests/conformance/persistence/test_charter_repository.pytests/e2e/test_charter_to_run_e2e.pytests/integration/mcp/test_tool_surface.pytests/unit/api/controllers/test_charter.pytests/unit/meta/charter/test_dispatch.pytests/unit/meta/mcp/handlers/test_charter.pyweb/src/__tests__/stores/charter.test.tsweb/src/api/endpoints/charter.tsweb/src/api/types/dtos.gen.tsweb/src/api/types/openapi.gen.tsweb/src/mocks/handlers/charter.tsweb/src/pages/CharterInterviewPage.tsxweb/src/pages/charter/CharterDraftCard.tsxweb/src/stores/charter.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). (1)
- GitHub Check: Test Unit
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{py,ts,tsx,jsx,md}
📄 CodeRabbit inference engine (CLAUDE.md)
No region/currency/locale privileged; use metric units; British English per docs/reference/regional-defaults.md
Files:
src/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/app.pyweb/src/mocks/handlers/charter.tsweb/src/api/endpoints/charter.tssrc/synthorg/observability/events/charter.pytests/unit/api/controllers/test_charter.pyweb/src/api/types/dtos.gen.tsweb/src/pages/CharterInterviewPage.tsxweb/src/stores/charter.tstests/integration/mcp/test_tool_surface.pyweb/src/__tests__/stores/charter.test.tssrc/synthorg/meta/mcp/domains/charter.pytests/unit/meta/mcp/handlers/test_charter.pytests/e2e/test_charter_to_run_e2e.pytests/conformance/persistence/test_charter_repository.pysrc/synthorg/meta/errors.pytests/unit/meta/charter/test_dispatch.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/meta/charter/dispatch.pyweb/src/api/types/openapi.gen.tsweb/src/pages/charter/CharterDraftCard.tsxsrc/synthorg/meta/charter/service.py
src/synthorg/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/synthorg/**/*.py: Onlysrc/synthorg/persistence/may import sqlite/psycopg or emit raw SQL; new repository protocols inherit from generic categories inpersistence/_generics.py; bespoke methods permitted only under ADR-0001 D7
Configuration Precedence: DB > env > code default viaSettingsService/ConfigResolver(Cat-1) or env > code default (Cat-2,read_only_post_init); Cat-3 bootstrap secrets pure env; YAML is ingestion format only, not precedence tier; noos.environ.getoutside startup
No hardcoded numeric values; numerics live insettings/definitions/; allowlist only 0/1/-1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants (NAME: int|float|Final); enforced byscripts/check_no_magic_numbers.py
Comments document WHY only; no reviewer citations, issue back-refs, or migration framing; enforced bycheck_no_review_origin_in_code.py+check_no_migration_framing.py
Nofrom __future__ import annotations(Python 3.14 has PEP 649); use PEP 758 except:except A, B:no parens unless binding
Type hints on public functions; mypy strict; Google-style docstrings; line length 88; functions <50 lines; files <800 lines
Errors follow<Domain><Condition>Errorpattern fromDomainError; never inheritException/RuntimeErrordirectly; enforced bycheck_domain_error_hierarchy.py
Pydantic v2 frozen +extra="forbid"on every frozen model project-wide; gatecheck_frozen_model_extra_forbid.py;@computed_fieldauto-exempt; per-line# lint-allow: frozen-extra-forbid -- <reason>forextra="allow"/"ignore"boundaries; use@computed_fieldfor derived; useNotBlankStrfor identifiers
Args models at every system boundary;parse_typed()for every external dict ingestion; enforced bycheck_boundary_typed.py
Immutability: usemodel_copy(update=...)orcopy.deepcopy(); deepcopy at system boundaries
Async: useasyncio.TaskGroupfor fan-out/fan-in; helpers catchException(re-raiseMemoryError/`RecursionError...
Files:
src/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/app.pysrc/synthorg/observability/events/charter.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/meta/errors.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.py
src/synthorg/meta/mcp/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
MCP: Define
ToolHandler+args_model; callrequire_admin_guardrails()on admin tools; route through service layers per mcp-handler-contract.md
Files:
src/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/meta/mcp/handlers/charter.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/meta/mcp/domains/_charter_args.pysrc/synthorg/api/app.pysrc/synthorg/observability/events/charter.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/meta/errors.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.py
web/src/**/*.{js,jsx,ts,tsx,mts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{js,jsx,ts,tsx,mts}: Always usecreateLoggerfrom@/lib/logger; never bareconsole.warn/console.error/console.debugin application code. Variable name must always belog. Onlylogger.tsitself may use bare console methods. Uselog.debug()(DEV-only, stripped in production),log.warn(),log.error().
Pass dynamic/untrusted values as separate args to logger calls (not interpolated into the message string) so they go throughsanitizeArg
Attacker-controlled fields inside structured objects must be wrapped insanitizeForLog()before embedding in log calls
Error-code constants (MANDATORY): importErrorCodeandErrorCategoryfrom@/api/types/errors(re-exported from the generatedweb/src/api/types/error-codes.gen.ts). Discriminate onErrorCode.<NAME>, never on raw integer literals.
Use@eslint-react/web-api-no-leaked-fetchto detectfetch()in effects withoutAbortControllercleanup
Files:
web/src/mocks/handlers/charter.tsweb/src/api/endpoints/charter.tsweb/src/api/types/dtos.gen.tsweb/src/pages/CharterInterviewPage.tsxweb/src/stores/charter.tsweb/src/__tests__/stores/charter.test.tsweb/src/api/types/openapi.gen.tsweb/src/pages/charter/CharterDraftCard.tsx
web/src/mocks/handlers/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/mocks/handlers/**/*.ts: MSW handlers (MANDATORY):web/src/mocks/handlers/must mirrorweb/src/api/endpoints/*.ts1:1 with a default happy-path handler for every exported endpoint. UseonUnhandledRequest: 'error'in test setup; tests override per-case viaserver.use(...), nevervi.mock('@/api/endpoints/*').
Use typed envelope helpers (successFor,paginatedFor,voidSuccess) to keep MSW handlers in lockstep with endpoint return types
Files:
web/src/mocks/handlers/charter.ts
web/src/**/*.{ts,tsx,mts}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{ts,tsx,mts}: Use@typescript-eslint/no-floating-promisesto forbid unawaited promises so async work cannot survive the test that scheduled it and trip the active-handle gate
Use@typescript-eslint/no-misused-promises(withchecksVoidReturn: { attributes: false }) to forbid passing async functions where the callsite ignores the returned promise. React 19asyncevent handlers stay allowed via theattributes: falseexemption.
Files:
web/src/mocks/handlers/charter.tsweb/src/api/endpoints/charter.tsweb/src/api/types/dtos.gen.tsweb/src/pages/CharterInterviewPage.tsxweb/src/stores/charter.tsweb/src/__tests__/stores/charter.test.tsweb/src/api/types/openapi.gen.tsweb/src/pages/charter/CharterDraftCard.tsx
web/src/{api/endpoints,stores}/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Cursor pagination (MANDATORY): list endpoints must use opaque cursor-based paging via
PaginationMeta. Stores must keepnextCursor+hasMorein state (not offset arithmetic) and early-return when!hasMore || !nextCursor. Display counts must come fromdata.length.
Files:
web/src/api/endpoints/charter.tsweb/src/stores/charter.ts
web/src/api/endpoints/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Health / readiness endpoints (MANDATORY):
getLiveness()is always 200 while the process is alive;getReadiness()is 200 healthy / 503 unavailable (binary'ok' | 'unavailable'outcome, no tri-state). Any new caller must handle the 503 path explicitly.
Files:
web/src/api/endpoints/charter.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Timeout/slow failures = source-code regression; never edittests/baselines/unit_timing.jsonor anyscripts/*_baseline.{txt,json}/scripts/_*_baseline.py; both families PreToolUse-blocked; per-invocation bypass requires explicit approval (ALLOW_BASELINE_GROWTH=1 git commit)
Test markers:@pytest.mark.{unit,integration,e2e,slow}; async auto; timeout 30s global; coverage 80% min
xdist-n 8 --dist=loadfileauto-applied via pyprojectaddopts; Windows unit tests useWindowsSelectorEventLoopPolicy; subprocess tests override back
Test doubles:FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags; bareMagicMockat typed boundary blocked byscripts/check_mock_spec.py
Hypothesis: 10 deterministic CI examples; failures are real bugs (fix + add@example(...)); never skip/xfail flaky tests; fix fundamentally
Files:
tests/unit/api/controllers/test_charter.pytests/integration/mcp/test_tool_surface.pytests/unit/meta/mcp/handlers/test_charter.pytests/e2e/test_charter_to_run_e2e.pytests/conformance/persistence/test_charter_repository.pytests/unit/meta/charter/test_dispatch.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/api/controllers/test_charter.pytests/integration/mcp/test_tool_surface.pytests/unit/meta/mcp/handlers/test_charter.pytests/e2e/test_charter_to_run_e2e.pytests/conformance/persistence/test_charter_repository.pytests/unit/meta/charter/test_dispatch.py
web/src/api/types/**/*.gen.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
Generated DTO types (MANDATORY): NEVER hand-edit
web/src/api/types/*.gen.ts. Regenerate withuv run python scripts/generate_dto_types_ts.py. Import DTOs via the barrel (import type { AgentConfig } from '@/api/types').
Files:
web/src/api/types/dtos.gen.tsweb/src/api/types/openapi.gen.ts
web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/**/*.{jsx,tsx}: Use@eslint-react/no-leaked-conditional-renderingto catch the{count && <Foo />}bug where0renders verbatim. ForReactNode | undefinedprops use{value != null && value !== false && <jsx>}; for compound truthiness useBoolean(...).
Use@eslint-react/globalsto restrictwindow/document/localStorage/ etc. inside render. Hoist offenders into auseCallbackevent handler, auseEffect, or auseSyncExternalStore-backed hook.Reuse
web/src/components/ui/design tokens in Web Dashboard Design System; detail inweb/CLAUDE.md
Files:
web/src/pages/CharterInterviewPage.tsxweb/src/pages/charter/CharterDraftCard.tsx
web/src/stores/**/*.ts
📄 CodeRabbit inference engine (web/CLAUDE.md)
web/src/stores/**/*.ts: List reads (fetch*) must seterror: string | nullon the store instead of toasting
Test teardown (MANDATORY): any new store that schedules timers or attaches event listeners must expose an equivalent cleanup hook and register it in the globalafterEach. The globalafterEachinweb/src/test-setup.tsxalready callsuseToastStore.getState().dismissAll(),cancelPendingPersist(), anduseThemeStore.getState().teardown().
Files:
web/src/stores/charter.ts
web/src/{stores,**/*.test.{ts,tsx}}
📄 CodeRabbit inference engine (web/CLAUDE.md)
Active-handle gate (MANDATORY): every unit test runs under
web/test-infra/active-handle-tracker.ts, which fails any test that leaks an event-loop-holding resource. A new store that schedules timers / attaches listeners MUST expose a teardown hook and register it in the globalafterEach; otherwise the gate fails the first test that triggers the schedule.
Files:
web/src/__tests__/stores/charter.test.ts
tests/conformance/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Dual-backend conformance:
tests/conformance/persistence/consumesbackendfixture (SQLite + Postgres); enforced bycheck_dual_backend_test_parity.py
Files:
tests/conformance/persistence/test_charter_repository.py
🧠 Learnings (3)
📚 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/meta/mcp/domains/_charter_args.pysrc/synthorg/api/app.pysrc/synthorg/observability/events/charter.pytests/unit/api/controllers/test_charter.pytests/integration/mcp/test_tool_surface.pysrc/synthorg/meta/mcp/domains/charter.pytests/unit/meta/mcp/handlers/test_charter.pytests/e2e/test_charter_to_run_e2e.pytests/conformance/persistence/test_charter_repository.pysrc/synthorg/meta/errors.pytests/unit/meta/charter/test_dispatch.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.py
📚 Learning: 2026-05-21T22:55:20.496Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/models.py:114-114
Timestamp: 2026-05-21T22:55:20.496Z
Learning: In this repo’s “magic number” review standard, the existing gate in `scripts/check_no_magic_numbers.py` intentionally does NOT flag numeric literals used as raw call-site arguments. So, do not flag numeric literals passed as keyword arguments to Pydantic `Field()` (e.g., `Field(ge=0, le=100)` / `Field(ge=1, le=50)`)—this is an established idiom. Only treat numeric literals as “magic numbers” when they occur in the locations the gate checks (module-level assignments and function/method parameter defaults).
Applied to files:
src/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/app.pysrc/synthorg/observability/events/charter.pytests/unit/api/controllers/test_charter.pytests/integration/mcp/test_tool_surface.pysrc/synthorg/meta/mcp/domains/charter.pytests/unit/meta/mcp/handlers/test_charter.pytests/e2e/test_charter_to_run_e2e.pytests/conformance/persistence/test_charter_repository.pysrc/synthorg/meta/errors.pytests/unit/meta/charter/test_dispatch.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.py
📚 Learning: 2026-05-21T22:55:09.289Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 2035
File: src/synthorg/meta/toolsmith/config.py:29-30
Timestamp: 2026-05-21T22:55:09.289Z
Learning: For this repo’s Pydantic configuration idiom, do not treat numeric literals passed directly as arguments to `pydantic.Field(...)` as “magic numbers” during review. This includes call-site usages like `Field(default=0.2, ge=0.0, le=1.0)` (e.g., in config models such as `ToolAuthoringConfig`, `ToolValidationConfig`, `ToolsmithConfig`). Do not request extracting those `Field(...)` numeric arguments into named constants, since the repo’s `scripts/check_no_magic_numbers.py` intentionally excludes call-site `Field(...)` numerics and relies on `Field(...)` as the canonical way to express these constraints/defaults.
Applied to files:
src/synthorg/meta/mcp/domains/_charter_args.pysrc/synthorg/api/app.pysrc/synthorg/observability/events/charter.pysrc/synthorg/meta/mcp/domains/charter.pysrc/synthorg/meta/errors.pysrc/synthorg/api/controllers/charter.pysrc/synthorg/meta/mcp/handlers/charter.pysrc/synthorg/meta/charter/dispatch.pysrc/synthorg/meta/charter/service.py
🔇 Additional comments (25)
src/synthorg/meta/mcp/domains/_charter_args.py (1)
66-67: LGTM!src/synthorg/api/app.py (1)
1706-1799: LGTM!web/src/mocks/handlers/charter.ts (1)
15-16: LGTM!Also applies to: 35-35, 54-65, 100-100
web/src/api/endpoints/charter.ts (1)
1-2: LGTM!Also applies to: 10-10, 15-15, 23-29
src/synthorg/observability/events/charter.py (1)
32-32: LGTM!Also applies to: 51-51
tests/unit/api/controllers/test_charter.py (1)
49-64: LGTM!web/src/api/types/dtos.gen.ts (1)
342-342: LGTM!web/src/pages/CharterInterviewPage.tsx (1)
64-68: LGTM!Also applies to: 81-87
web/src/stores/charter.ts (1)
33-38: LGTM!Also applies to: 49-50, 61-79, 108-156
tests/integration/mcp/test_tool_surface.py (1)
1-1: LGTM!Also applies to: 398-402
web/src/__tests__/stores/charter.test.ts (1)
5-5: LGTM!Also applies to: 20-21, 32-42, 49-49
src/synthorg/meta/mcp/domains/charter.py (1)
87-97: LGTM!tests/unit/meta/mcp/handlers/test_charter.py (1)
197-229: LGTM!tests/e2e/test_charter_to_run_e2e.py (1)
454-457: LGTM!tests/conformance/persistence/test_charter_repository.py (1)
352-374: LGTM!src/synthorg/meta/errors.py (1)
270-292: LGTM!tests/unit/meta/charter/test_dispatch.py (1)
331-337: LGTM!src/synthorg/api/controllers/charter.py (1)
20-31: LGTM!Also applies to: 154-193
src/synthorg/meta/mcp/handlers/charter.py (1)
204-210: LGTM!src/synthorg/meta/charter/dispatch.py (1)
38-49: LGTM!Also applies to: 225-233, 278-285, 376-400
web/src/api/types/openapi.gen.ts (3)
10161-10175: LGTM!
18797-18798: LGTM!
18816-18816: LGTM!web/src/pages/charter/CharterDraftCard.tsx (1)
44-48: LGTM!src/synthorg/meta/charter/service.py (1)
35-35: LGTM!
| const { hasMore, nextCursor, charters, loading } = get() | ||
| // Early-return on no-more-pages and on duplicate-in-flight calls per | ||
| // the cursor-pagination MANDATORY rule in ``web/CLAUDE.md``. | ||
| if (!hasMore || !nextCursor || loading) return | ||
| set({ loading: true }) | ||
| try { | ||
| const page = await charterApi.listCharters({ | ||
| ...filters, | ||
| cursor: nextCursor, | ||
| }) | ||
| set({ | ||
| charters: [...charters, ...page.data], | ||
| nextCursor: page.nextCursor, | ||
| hasMore: page.hasMore, | ||
| loading: false, | ||
| }) |
There was a problem hiding this comment.
Use current state when appending paginated rows to avoid stale overwrite.
fetchMoreCharters snapshots charters before the await and then writes charters: [...charters, ...page.data]. If another action updates the list while this request is in flight, this response can clobber newer state.
Suggested fix
- const { hasMore, nextCursor, charters, loading } = get()
+ const { hasMore, nextCursor, loading } = get()
@@
- set({
- charters: [...charters, ...page.data],
- nextCursor: page.nextCursor,
- hasMore: page.hasMore,
- loading: false,
- })
+ set((s) => ({
+ charters: [...s.charters, ...page.data],
+ nextCursor: page.nextCursor,
+ hasMore: page.hasMore,
+ loading: false,
+ }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/stores/charter.ts` around lines 86 - 101, fetchMoreCharters currently
snapshots charters before awaiting charterApi.listCharters and then writes
charters: [...charters, ...page.data], which can clobber newer updates; fix by
using a state-update function or re-reading latest state before appending: after
the await, obtain the current charters (via get() or by passing a functional
updater to set) and then set charters to [...latestCharters, ...page.data],
while still updating nextCursor, hasMore and loading; reference
fetchMoreCharters, charterApi.listCharters, get(), set(), and the
charters/nextCursor/hasMore/loading fields.
Pin SelectorEventLoop as the process-wide default for unit tests on Windows so sync TestClient portals and asyncio.run sites avoid the Python 3.14 ProactorEventLoop IOCP teardown race, and make the affected-tests classifier treat a bare worker node-down (no real failure, no repeated named crash) as advisory -- the loadscope crash guard suppresses the INTERNALERROR the gate previously required.
…/test gaps) Security: enforce creator-only access on GET / PATCH edit / cancel via service-layer ownership check, shaped as NotFound so the response cannot probe foreign charter existence (CHARTER_OWNERSHIP_DENIED is logged so operators still see the fence event). Add the require_approval_roles guard on the REST approve endpoint to match the MCP admin guardrail; budget is spent there. Document _FORECAST_NAMESPACE intent. Drop unused now param on _resolve_project. Add an enforce_ownership=False kwarg on cancel_charter so the MCP admin cancel path legitimately operates on a charter it did not author. Add the composite (created_at, id) DESC index in both backends so list_items/query avoid a sort. Tests: concurrent run_turn serialisation, concurrent approve CAS race, MCP handler unit suite (interview wraps untrusted via TAG_TASK_DATA, list / get / cancel / approve), brief_hash determinism, DuplicateRecordError idempotent retry, dispatch-failure structured logging, turn-cap at default config, lock-allocation stress, ownership-denied path. Web: composite key + eslint exception on StringList, replace literal & with &, optimistic-rollback on runTurn failure, aria-atomic on interview live region, defensive doc note on Zustand store memory, local-component rationale on ChatBubble + StringList.
CI: - Bump test_tool_surface 219 to 224 for 5 new charter MCP tools. Reviewer (Gemini, 2): - G1 dispatch.py: raise CharterStateInconsistentError on missing charter re-fetch after successful CAS (no stale-fallback). - G2 service.py: same pattern in cancel_charter. Reviewer (CodeRabbit, 13 in-code; 3 skipped with documented disproof): - C2 app.py _wire_charter_engine: wrap body in try/except so unexpected wiring failures stay best-effort. - C4 dispatch.py: emit new CHARTER_PROJECT_ALREADY_EXISTS event for DuplicateRecordError retry; reserve CHARTER_STATUS_TRANSITIONED for actual state changes. - C5 dispatch.py _close_conversation: wrap in try/except so a best-effort close cannot retroactively fail a successful approval. - C7 mcp/handlers/charter.py + domains/charter.py + _charter_args.py: add require_admin_guardrails to cancel handler; promote cancel tool to admin_tool with ADMIN_GUARDRAIL_PROPERTIES so the enforce_ownership=False bypass is locally guarded. - C9 e2e: assert forecast cardinality == 1 before selecting. - C10 test_dispatch: assert structured charter_id on CHARTER_DISPATCH_FAILED log record. - C11 controllers/charter.py + endpoints/charter.ts + stores + mocks + store tests: convert /meta/charters list to opaque cursor pagination via encode_countless_seek_meta; store now carries nextCursor/hasMore + fetchMoreCharters. - C12 mocks/charter.ts: replace hardcoded 'USD' with DEFAULT_CURRENCY from utils/currencies. - C13 mocks/charter.ts: derive approve response project_id from request params.id. - C14 CharterDraftCard.tsx: useEffect to resync local brief/amount state when parent supplies a new charter id/version. - C15 CharterInterviewPage.tsx: disable 'New interview' button while a charter mutation is in flight (sending OR mutating). - C16 stores/charter.ts: early-return runTurn when already sending to prevent overlapping turns from clobbering transcript on rollback. - C-OOD1 test_charter.py controller: add 503 tests for PATCH and POST /cancel routes (matches existing approve/interview coverage). - C-OOD2 test_charter_repository conformance: replace placeholder with a real failing-write test exercising the chk_charter_approval_coupling CHECK via transition_if with intentionally-omitted provenance fields. Skipped with disproof (documented in babysit round-history): - C1 scripts/run_affected_tests.py:638-707: node-down to advisory downgrade is intentional per the docstring + project memory project_isolation_gate_fullsuite_crash.md; repeated-name guard at line 670 is the safety net for genuine repro crashes. - C3 dispatch.py:198-211 (run-before-CAS): DB CHECK chk_charter_approval_coupling mandates task_id NOT NULL when status=approved, so a CAS-first refactor requires a schema change (intermediate state or nullable task_id). Per-charter asyncio.Lock serialises in-process; multi-worker race is C6 territory and pre-alpha out-of-scope. - C6 service.py:114-130 (distributed lock): pre-alpha single-worker deployment; asyncio.Lock is sufficient. Distributed-lock infrastructure (Redis advisory / DB SELECT FOR UPDATE) is a multi-file dependency change not warranted now. - C8 (FK constraints on conversation_id/project_id): reverted schema changes because all existing charter conformance tests + the e2e fixture build charters with parent ids that don't exist in the parent tables; SQLite enables PRAGMA foreign_keys ON, so adding FKs would cascade to ~10 test-fixture rewrites. Application-layer guards (_resolve_conversation + ProjectService in dispatch) already enforce parent presence in production.
Round 3 fix C7 added require_admin_guardrails to the _charter_cancel handler; the existing test_cancel_passes_enforce_ownership_false_admin_path now needs confirm + reason fields in the args, and we add a sibling test_cancel_requires_admin_guardrail that pins the rejection path.
ESLint @eslint-react/set-state-in-effect (max-warnings 0) rejected the useEffect form. The idiomatic React pattern for 'reset local state when prop identity changes' is a key on the parent; that's what the babysit round 3 C14 fix should have used.
main merged #2044 (mission-control + flight-recorder) which added the cockpit MCP domain, cockpit settings namespace, and its own generated types. Rebasing this charter PR on top required: - Keeping both charter and cockpit imports in the MCP domain / handler aggregators and the SettingNamespace enum. - Bumping the pinned tool-count assertions to 231 (219 baseline + 7 cockpit + 5 charter) in test_tool_surface and test_all_handlers_wired. - Regenerating web/src/api/types/{dtos,enum-values,openapi}.gen.ts so both the cockpit and charter DTO additions are reflected (the generator is the single source of truth; the rebase conflict markers inside the .gen.ts files were resolved by accepting cockpit then regenerating to produce the union).
… for fetchMoreCharters Two new CodeRabbit findings on the round-3 fixes: - service.py / dispatch.py: emit a structured ERROR log on the CHARTER_STATE_INCONSISTENT event before raising CharterStateInconsistentError so the row disappearance is recorded even though the exception bubbles past. Stage label disambiguates the two call sites (cancel_charter vs approve_charter). - stores/charter.ts fetchMoreCharters: replace pre-await snapshot of charters with a functional set updater so any mutation that lands during the in-flight cursor fetch is preserved instead of being clobbered by the stale snapshot.
Aureliolo
force-pushed
the
feat/1977-deep-ceo-interview-charter
branch
from
9184fdd to
6f89dd4
Compare
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2045 +/- ##
=========================================
Coverage 85.04% 85.05%
=========================================
Files 2208 2251 +43
Lines 128126 130269 +2143
Branches 10613 10748 +135
=========================================
+ Hits 108962 110796 +1834
- Misses 16488 16755 +267
- Partials 2676 2718 +42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - New brownfield codebase intake mode supports merger and acquisition scenarios. - Added deep CEO interview feature to improve project charter creation. - Introduced mission control and flight recorder operator cockpit for better operational oversight. - Research mode added for enhanced exploratory work. - Runtime services now log safety-spine state at boot for clearer diagnostics. ### What's new - Research mode feature enables deeper data exploration. - CEO interview integration helps shape project charters. - Mission control and flight recorder cockpit introduced for operational tracking. ### Under the hood - Improved codebase modularity with module-size gates and lint tightening. - Added __init__.py to 21 test directories for better test discovery. - Promoted six transitive dependencies to direct dependencies for clarity. - Split codespell ignore list into vocabulary and source renames. - Decomposed oversized web utilities, hooks, and libraries for maintainability. - Enhanced CI with Lychee link checker integration and retry logic for cosign signing. - Sharded unit and integration tests and added Postgres service container in CI. - Updated infrastructure and web dependencies; maintained lock files. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.8](v0.8.7...v0.8.8) (2026-05-24) ### Features * brownfield codebase intake (merger/acquisition entry mode) ([#2042](#2042)) ([e287621](e287621)), closes [#1975](#1975) * deep CEO interview to project charter ([#2045](#2045)) ([904f2fb](904f2fb)) * mission control + flight recorder operator cockpit ([#2044](#2044)) ([1c2660b](1c2660b)) * research mode ([#2041](#2041)) ([f81a5ac](f81a5ac)), closes [#1989](#1989) * surface safety-spine state in runtime-services boot log (closes [#2096](#2096)) ([#2097](#2097)) ([f187b31](f187b31)) ### Refactoring * add __init__.py to 21 leaf test directories (INP001) ([#2081](#2081)) ([2592118](2592118)), closes [#2064](#2064) * codebase modularity (1/4) - module-size gates + lint tightening + tools ([#2078](#2078)) ([556fbd9](556fbd9)), closes [#2047](#2047) [#2040](#2040) * promote 6 transitive deps to direct deps ([#2083](#2083)) ([adedc6a](adedc6a)) * split codespell ignore-words-list into vocab + source renames ([#2085](#2085)) ([917d98a](917d98a)), closes [#2074](#2074) * **web:** PR A foundation, decompose oversized utils/hooks/lib ([#2092](#2092)) ([#2098](#2098)) ([aedbba5](aedbba5)) ### CI/CD * exclude slsa.dev from lychee (transient timeout on canonical badge) ([#2090](#2090)) ([346c51d](346c51d)) * fix paths-filter shallow-clone race and scorecard allowlist ([#2089](#2089)) ([7cd7ce8](7cd7ce8)) * refresh .test_durations.{unit,integration} ([#2087](#2087)) ([ddf2d86](ddf2d86)) * retry cosign sign on transient GHCR/Rekor failures ([#2100](#2100)) ([da9422a](da9422a)) * shard test-unit + test-integration, sysmon coverage, Postgres service container ([#2080](#2080)) ([0768787](0768787)) * wire Lychee link-checker (workflow + installer + pre-push hook) ([#2084](#2084)) ([1c0694a](1c0694a)) ### Maintenance * Lock file maintenance ([#2086](#2086)) ([a78810a](a78810a)) * Update Infrastructure dependencies ([#2055](#2055)) ([041ad8b](041ad8b)) * Update Web dependencies ([#2054](#2054)) ([4d57b9a](4d57b9a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: synthorg-repo-bot[bot] <279117679+synthorg-repo-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Closes #1977.
Summary
Adds the deep-intake conversational front door: a multi-turn requirements-elicitation interview that elicits goals / constraints / success criteria / scope boundaries / budget+time envelope from a vague one-line product idea, persists a reviewable
ProjectCharter, and on approval drives a real project run through the work-pipeline spine.Builds on EPIC B #1968 (conversation substrate, reused) and #1960 (pipeline spine). Part of EPIC #1973 (autonomous product studio).
What ships
Domain
ProjectCharter+BudgetEnvelope+ScopeBoundaries+CharterDraft+InterviewDecision(frozen,extra="forbid", XOR validators on project binding + approval coupling).CharterStatus(StrEnum):DRAFTED(under review) →APPROVED(terminal) /CANCELLED(terminal).CharterNotFoundError,CharterNotEditableError,CharterAlreadyDecidedError,CharterInterviewUnavailableError,CharterInterviewResponseInvalidError,UnknownCharterStrategyError.Subsystem (
src/synthorg/meta/charter/)CharterInterviewStrategyProtocol +LLMCharterInterviewerdefault; pluggable viaCharterConfig.interview_strategydiscriminator ("llm"only;UnknownCharterStrategyErroron misconfiguration, no silent default).CharterInterviewServiceorchestrates per-conversation locks, runs one turn, persists either an assistant question (conversation stays ACTIVE — deviation from the plan's PROPOSED transition) or the drafted charter, edits in place while DRAFTED, cancels with DRAFTED→CANCELLED CAS. Ownership-fenced onget/edit_charter/cancel_charter(creator-only; foreign access shaped as NotFound,CHARTER_OWNERSHIP_DENIEDlogged structurally).CharterDispatcher.approveresolves the project (existing or deterministiccharter-{id}new), persists an already-APPROVEDForecastwithceiling_amount = envelope.amount, builds aWorkItemcarryingforecast_id+hard_ceiling, runswork_pipeline.run(item), then CAS-transitions DRAFTED→APPROVED with the full provenance set (approved_at/approved_by/forecast_id/correlation_id/task_id) and closes the interview conversation.Persistence (dual-backend)
CharterRepositoryProtocol composesStatefulRepository[ProjectCharter, NotBlankStr, CharterStatus]+FilteredQueryRepository[ProjectCharter, CharterFilterSpec](ADR-0001 compliant, no bespoke methods).cost_forecast_repo.py; JSON-encoded tuple columns; flattened envelope; CHECK constraints enforce project-binding XOR + approval coupling at the DB level.20260522000002_project_charters.sql(both backends) + declaredschema.sqlupdates; composite(created_at DESC, id DESC)index covers thelist_items/queryORDER BY.tests/conformance/persistence/test_charter_repository.py) covers CRUD round-trip, in-place edit, filtered query, count, status transitions, CAS-fail-on-mismatch, project-binding XOR, approval coupling.API + MCP
CharterControllerat/meta/charters—POST /interview,GET /,GET /{id},PATCH /{id},POST /{id}/approve,POST /{id}/cancel. Approve is gated byrequire_approval_roles(CEO / Manager / Board Member) so budget-spend authority matches the MCP admin guardrail; the dispatcher does NOT itself ownership-fence approve so an approval-tier actor can legitimately dispatch a junior's charter (authorship is preserved oncreated_by).CHARTER_SUBSTRATE_UNAVAILABLEwarning when the subsystem is not wired.synthorg_charter_interview/list/get/cancel/approve(admin-gated). MCP cancel passesenforce_ownership=Falseso admin operators can cancel stalled charters they did not author.Boot wiring
_wire_charter_enginestartup hook (after persistence + provider), gated onmeta.charter.interview_enabled+ provider + persistence; best-effort with structured warning + 503 surface when unwired. Idempotent; rebuilds viapost_setup_reinit.ENFORCEDentries forCharterInterviewServiceandCharterDispatcher.Settings
SettingNamespace.CHARTER.settings/definitions/charter.pyregisters:interview_enabled,interview_strategy,interview_model,interview_temperature,interview_max_tokens,interview_max_turns,default_currency(withlint-allow: regional-defaultsmirroringbudget.DEFAULT_CURRENCY). All numerics defined here; no inline literals.Observability
observability/events/charter.py:CHARTER_INTERVIEW_TURN,CHARTER_INTERVIEW_QUESTION,CHARTER_INTERVIEW_DRAFTED,CHARTER_INTERVIEW_CAP_REACHED,CHARTER_INTERVIEW_RESPONSE_INVALID,CHARTER_INTERVIEW_FAILED,CHARTER_EDITED,CHARTER_STATUS_TRANSITIONED,CHARTER_OWNERSHIP_DENIED,CHARTER_APPROVED,CHARTER_CANCELLED,CHARTER_DISPATCHED,CHARTER_DISPATCH_FAILED,CHARTER_SUBSTRATE_UNAVAILABLE.VALID_SETTINGS_NAMESPACESincludes"charter"for the Prometheus settings-mutation metric.Web dashboard
CharterInterviewPageat/meta/charterswith a two-pane layout:InterviewChat(left, witharia-live="polite" aria-atomic="true"transcript + composer) andCharterDraftCard(right, inline-editable brief + budget envelope, plus Approve / Cancel actions). Nav entry inSidebar.useCharterStorewith the project's mutation-pattern discipline (try/catch in the store, toast on success/failure, sentinel-null on error, optimistic message rollback onrunTurnfailure). Memory-only storage documented inline.Untrusted-content fence
Human interview messages are wrapped in
<task-data>envelopes viawrap_untrusted(TAG_TASK_DATA, ...)at both the REST controller and the MCP handler before reaching the strategy; the strategy never sees a raw user string.Acceptance
tests/e2e/test_charter_to_run_e2e.py::test_vague_idea_becomes_approved_charter_that_runsdrives the full flow under the simulation harness with a scripted provider and a realwork_pipeline: vague one-liner → interview → drafted charter → approve → new project + APPROVED forecast + spine-created Task (past CREATED). Marked@pytest.mark.e2e— runs in CI, not in the local pre-push gate.Test coverage added in the review round
run_turnserialise on a shared conversation (per-conversationasyncio.Lockis exercised).dispatcher.approveCAS race (only one approval wins; project + pipeline run once).wrap_untrusted(TAG_TASK_DATA, ...)verified at the MCP boundary.compute_brief_hashdeterminism (same brief → same hash; idempotent retries upsert).DuplicateRecordErrorbranch onproject_service.create(idempotent project-creation retry).transition_ifreturnsFalseon state mismatch (dual-backend conformance).CHARTER_DISPATCH_FAILEDstructurally logged before re-raise.get/edit/cancelpaths.Test plan
uv run python -m pytest tests/ -m unit(full unit suite, including all charter modules + observability inventory + MCP tool-count assertion at 224)uv run python -m pytest tests/conformance/persistence/test_charter_repository.py(SQLite + Postgres, run in CI)uv run python -m pytest tests/e2e/test_charter_to_run_e2e.py -m e2e(acceptance, run in CI; not in the local pre-push gate per project convention)npm --prefix web run lint && npm --prefix web run type-check && npm --prefix web run test(3217/3217 passing locally)Notable deviations from the original plan
require_approval_rolesguard for authorisation rather than dispatcher-level ownership; admins legitimately approve juniors' charters. Authorship is preserved oncreated_byfor audit.Operator follow-up on merge
The two-backend schema-drift baselines update on a new persistence table; CI / a maintainer will run
uv run python scripts/check_schema_drift_revisions.py --backend sqliteand--backend postgresand refresh any drift baselines if the gate flags it. The new MCP tool count (224) is pinned intests/unit/meta/mcp/test_all_handlers_wired.py; any subsequent tool-surface change must bump this number explicitly.