feat: add meeting scheduler, event-triggered meetings, and Go CLI lint fixes#407
Merged
feat: add meeting scheduler, event-triggered meetings, and Go CLI lint fixes#407
Conversation
Implement the meeting scheduler service that bridges meeting configuration
and meeting execution. Adds frequency-based periodic scheduling (via
asyncio tasks) and event-triggered meetings (on-demand API).
New modules:
- frequency.py: MeetingFrequency enum (daily/weekly/bi_weekly/per_sprint_day/monthly)
- participant.py: ParticipantResolver protocol + RegistryParticipantResolver
- scheduler.py: MeetingScheduler background service (start/stop/trigger_event)
API changes:
- GET /meetings: list meeting records with status/type filters + pagination
- GET /meetings/{id}: get meeting record by ID (or 404)
- POST /meetings/trigger: trigger event-based meetings
- Added "meetings" WebSocket channel + meeting event types
- AppState wired with meeting_orchestrator + meeting_scheduler
Config change:
- MeetingTypeConfig.frequency: NotBlankStr -> MeetingFrequency enum
(Pydantic coerces strings transparently, YAML configs work unchanged)
Frontend:
- Meeting TypeScript types + API client + Pinia store
- MeetingLogsPage: DataTable with filters, detail sidebar, trigger dialog
Pre-reviewed by 14 agents, 19 findings addressed: - Wrap event publisher in try/except (data-loss prevention) - Add error recovery in _run_periodic loop body - Extract _publish_meeting_event helper (function length) - Split _execute_meeting try/except for clarity - Replace assert with proper TypeError guard - Merge double context.items() iteration - Add exhaustiveness check for frequency mapping - Log before raising SchedulerAlreadyRunningError - Add logger usage in meetings controller (404 + trigger) - Bound TriggerMeetingRequest.context (max keys/lengths) - Remove ambiguous pass-through debug log - Add MeetingPhase type union in frontend types.ts - Re-fetch full record on WS meeting events (not just status) - Keep trigger dialog open on error - Update CLAUDE.md (package structure, stores, events) - Fix assert_awaited_with -> assert_awaited_once_with - Add NoParticipantsResolvedError test - Remove unnecessary async on sync static method tests
Critical fixes: - Wire scheduler start()/stop() into app lifecycle (_safe_startup/_safe_shutdown) - Add 5 missing meeting event constants to CLAUDE.md logging section - Add console.warn in meetings store empty catch block Major fixes: - Replace MEETING_SCHEDULER_ERROR with MEETING_NOT_FOUND for 404 path - Use TaskGroup for parallel meeting execution in trigger_event - Fix TriggerMeetingRequest.context type: Record<string, string> - Replace _tasks list mutation with immutable patterns - Sync selectedMeeting on WS refresh - Validate context list values in participant resolver - Re-raise CancelledError instead of swallowing in _run_periodic - Add Final annotations to all channel constants - Add Meeting Scheduler section to communication design doc - Add meeting/ subpackage entry in CLAUDE.md package structure - Add channel guard to handleWsEvent - Document leader selection convention - Make resolver raise NoParticipantsResolvedError on empty results - Rewrite MeetingLogsPage.test.ts to test store, not duplicate - Add fetchMeetings/triggerMeeting store tests - Add TriggerMeetingRequest validation boundary tests Medium fixes: - Add _publish_meeting_event error + MemoryError re-raise tests - Fix test name typo: callsmock → calls_mock - Fix comment typo: meetingmock_scheduler → meeting_scheduler - Remove unnecessary async from mock_orchestrator fixture - Use comprehensions in _build_default_agenda - Move NoParticipantsResolvedError import to top-level - Fix errors.py module docstring overclaim - Add per_sprint_day to coercion test parametrize - Pass data.context directly instead of `or None` - Extract _create_app_without_meetings helper - Replace inline styles with Tailwind classes in MeetingLogsPage - Add stop_noop_when_not_running test
…solver When a context value is present but is neither str nor list/tuple, explicitly log a warning and return empty instead of silently falling through to registry lookups. Prevents misresolution via pass-through for internal callers using dict[str, Any] context.
- Widen TriggerMeetingRequest.context to dict[str, str | list[str]] so API callers can pass multi-value participant entries; update validator to handle list items; align frontend TS type - Extract _resolve_participants and _run_and_publish from _execute_meeting to stay under 50-line function limit - Replace f-string status formatting in _publish_meeting_event with explicit _STATUS_TO_WS_EVENT mapping (no API layer import) - Guard trigger_event with self._config.enabled check - Assert mock_scheduler.trigger_event called with expected args - Add exact-boundary test for max key/value lengths - Add docstring to test_accepts_enum_value - Rename MeetingLogsPage.test.ts to .store-integration.test.ts
dict[str, str] is not assignable to dict[str, str | list[str]] under mypy strict mode (dict is invariant in value type). Add explicit type annotations on test variables.
- Remove unreachable "in_progress" → "meeting.started" mapping from _STATUS_TO_WS_EVENT (status is always terminal post-run_meeting) - Wrap _build_default_agenda in exception guard so failures don't propagate to TaskGroup and crash the entire trigger_event call - Execute meeting immediately on first periodic loop entry, then sleep (run-now-then-repeat pattern avoids missing today's meeting)
- Fix "e.g." → "e.g.," in CLAUDE.md event names line - Add DEBUG log before literal ID fallback in participant resolver - Add test for oversized context list items in TriggerMeetingRequest - Add meeting scheduler lifecycle test (_safe_startup/_safe_shutdown) - Use pydantic.ValidationError in config frequency tests - Add test_context_tuple_value for tuple context branch - Batch triggerMeeting store updates (single array spread) - Parametrize test_all_channels_contains_expected
Replace the local selected ref with a computed over meetingStore.selectedMeeting so the sidebar reflects real-time WS refresh updates instead of holding a stale object reference.
…tore - Strip whitespace-only context strings in participant resolver and log warning instead of passing blank IDs downstream - Log periodic task errors during scheduler stop() instead of silently discarding them via return_exceptions=True - Add _MAX_CONTEXT_LIST_ITEMS=50 limit to TriggerMeetingRequest validator with test coverage - Fix meeting_client fixture return type: Generator → Iterator - Add loading state to triggerMeeting store action for consistent UI
- Move gofmt from linters to formatters section in .golangci.yml (v2 reclassified it, causing "gofmt is a formatter" config error) - Force bash shell for go test step to prevent PowerShell from splitting -coverprofile=coverage.out into separate arguments
- Format list context values as comma-joined strings in agenda descriptions instead of Python repr (e.g. "a, b" not "['a', 'b']") - Re-raise MemoryError/RecursionError in _run_periodic before the broad except Exception handler - Use arrow wrapper for ErrorBoundary @Retry to prevent immediate invocation on render - Add optional chaining on decisions/action_items/contributions .length in sidebar template - Guard "Trigger Meeting" button with v-if="canWrite" to match write-action pattern used in TaskBoardPage
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds a Meeting Scheduler subsystem (frequency-based and event-triggered), participant resolution, API endpoints and WS publishing, AppState/lifecycle wiring, observability events, frontend types/endpoints/store/UI, docs, and extensive backend/frontend tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Scheduler as MeetingScheduler
participant Resolver as ParticipantResolver
participant Registry as AgentRegistry
participant Orchestrator as MeetingOrchestrator
participant WS as WsPublisher
Client->>Scheduler: trigger_event(event_name, context)
Scheduler->>Scheduler: find matching meeting types
loop per meeting type
Scheduler->>Resolver: resolve(participant_refs, context)
Resolver->>Registry: query (list_active / list_by_department / get_by_name)
Registry-->>Resolver: agent IDs
Resolver-->>Scheduler: deduplicated participant IDs
Scheduler->>Scheduler: build default agenda from context
Scheduler->>Orchestrator: execute(meeting_type, agenda, participants)
Orchestrator-->>Scheduler: MeetingRecord
Scheduler->>WS: publish meeting.started/completed/failed
end
Scheduler-->>Client: tuple[MeetingRecord, ...]
sequenceDiagram
participant System
participant Scheduler as MeetingScheduler
participant Async as AsyncIO
participant Orchestrator as MeetingOrchestrator
System->>Scheduler: start()
Scheduler->>Scheduler: mark running
Scheduler->>Async: spawn periodic tasks per frequency
loop intervals
Async->>Scheduler: _run_periodic(meeting_type)
Scheduler->>Orchestrator: execute(...)
Orchestrator-->>Scheduler: MeetingRecord
Scheduler->>Scheduler: publish WS event
end
System->>Scheduler: stop()
Scheduler->>Async: cancel tasks
Async-->>Scheduler: tasks cancelled
Scheduler->>Scheduler: mark stopped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Contributor
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 comprehensive meeting management system, enabling scheduled and event-driven agent meetings. It provides the core backend services for scheduling, participant resolution, and API exposure, alongside a fully functional frontend interface for monitoring and initiating these meetings. The changes enhance the platform's ability to orchestrate multi-agent interactions and provide visibility into their collaborative processes. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-architected feature for meeting scheduling, including periodic and event-triggered meetings. The changes span the entire stack, from backend services and API endpoints to a new frontend UI with a corresponding Pinia store and WebSocket integration. The code is generally of high quality, with comprehensive tests and updated documentation. My review has identified a critical syntax error in the new scheduler module that needs to be addressed, as well as a couple of opportunities for performance improvements in the new meetings API controller.
Comment on lines
+245
to
+246
| except MemoryError, RecursionError: | ||
| raise |
Contributor
There was a problem hiding this comment.
The syntax except MemoryError, RecursionError: is from Python 2 and will raise a SyntaxError in Python 3. To catch multiple exceptions, they should be enclosed in a tuple. This is a critical bug that will prevent the periodic meeting task from handling these errors correctly.
except (MemoryError, RecursionError):
raise
Comment on lines
+403
to
+404
| except MemoryError, RecursionError: | ||
| raise |
Contributor
There was a problem hiding this comment.
Similar to another part of this file, the syntax except MemoryError, RecursionError: is invalid in Python 3 and will cause a SyntaxError. You should use a tuple to catch multiple exception types. This will prevent the event publisher from correctly handling these specific errors.
Suggested change
| except MemoryError, RecursionError: | |
| raise | |
| except (MemoryError, RecursionError): | |
| raise |
Comment on lines
+111
to
+114
| if status is not None: | ||
| records = tuple(r for r in records if r.status == status) | ||
| if meeting_type is not None: | ||
| records = tuple(r for r in records if r.meeting_type_name == meeting_type) |
Contributor
There was a problem hiding this comment.
The current implementation for filtering records is inefficient as it reconstructs the records tuple for each applied filter. For a large number of records, this can lead to unnecessary memory allocations and processing. A more performant approach would be to chain generator expressions for filtering and only materialize the final list or tuple once.
Suggested change
| if status is not None: | |
| records = tuple(r for r in records if r.status == status) | |
| if meeting_type is not None: | |
| records = tuple(r for r in records if r.meeting_type_name == meeting_type) | |
| records_iter = orchestrator.get_records() | |
| if status is not None: | |
| records_iter = (r for r in records_iter if r.status == status) | |
| if meeting_type is not None: | |
| records_iter = (r for r in records_iter if r.meeting_type_name == meeting_type) | |
| page, meta = paginate(tuple(records_iter), offset=offset, limit=limit) |
Comment on lines
+137
to
+142
| for record in records: | ||
| if record.meeting_id == meeting_id: | ||
| return Response( | ||
| content=ApiResponse[MeetingRecord](data=record), | ||
| status_code=200, | ||
| ) |
Contributor
There was a problem hiding this comment.
This implementation iterates through all meeting records to find one by its ID, which is an O(N) operation. This can become a performance bottleneck if the number of meetings grows large. For better performance, consider using a more direct lookup method. A potential long-term improvement would be to have the MeetingOrchestrator provide a method like get_record_by_id(meeting_id) that uses a dictionary/hash map for O(1) lookups.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #407 +/- ##
==========================================
- Coverage 93.79% 93.65% -0.15%
==========================================
Files 466 469 +3
Lines 21944 22211 +267
Branches 2103 2141 +38
==========================================
+ Hits 20582 20801 +219
- Misses 1058 1097 +39
- Partials 304 313 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the missing “meeting scheduler” layer and wires it end-to-end: backend scheduling/triggering + API/WS exposure + frontend meeting logs UI, along with supporting type/config changes and CI tweaks.
Changes:
- Added
MeetingScheduler(periodic + event-triggered meetings),MeetingFrequency, and participant resolution (ParticipantResolver/RegistryParticipantResolver) with lifecycle wiring. - Exposed meetings via REST (
GET /meetings,GET /meetings/{id},POST /meetings/trigger) and WebSocket events/channel; added observability event constants. - Implemented
MeetingLogsPageUI + Pinia store + API client + TS types; updated related tests and CLI CI lint/workflow config.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/views/MeetingLogsPage.vue | Replaces “Coming Soon” with full meeting logs table, detail sidebar, WS wiring, filtering, and trigger dialog. |
| web/src/styles/theme.ts | Extends status color typing/mapping to include meeting statuses. |
| web/src/stores/meetings.ts | Adds Pinia meetings store (fetch/list/get/trigger + WS refresh handler). |
| web/src/api/types.ts | Adds meeting domain TS types + WS channel/event type extensions. |
| web/src/api/endpoints/meetings.ts | Adds meetings REST client endpoints (list/get/trigger). |
| web/src/tests/views/MeetingLogsPage.test.ts | Removes old “Coming Soon” view test. |
| web/src/tests/views/MeetingLogsPage.store-integration.test.ts | Adds minimal store integration test scaffold. |
| web/src/tests/stores/meetings.test.ts | Adds store behavior tests (WS refresh, fetch, trigger dedup, error handling). |
| tests/unit/communication/test_config.py | Updates meeting config tests to use MeetingFrequency. |
| tests/unit/communication/meeting/test_scheduler.py | Adds unit tests for scheduler behavior (periodic lifecycle, triggers, publishing). |
| tests/unit/communication/meeting/test_participant.py | Adds tests for participant resolver resolution cascade + dedup + context behavior. |
| tests/unit/communication/meeting/test_frequency.py | Adds tests for MeetingFrequency and frequency_to_seconds. |
| tests/unit/communication/meeting/test_config_frequency.py | Adds tests for config coercion/validation with MeetingFrequency. |
| tests/unit/communication/conftest.py | Updates shared fixtures to use MeetingFrequency. |
| tests/unit/api/test_channels.py | Extends WS channel set to include meetings. |
| tests/unit/api/test_app.py | Updates lifecycle tests to cover scheduler start/stop and new startup/shutdown signatures. |
| tests/unit/api/controllers/test_meetings.py | Adds comprehensive controller tests for list/get/trigger + request validation bounds. |
| tests/integration/communication/test_meeting_integration.py | Updates integration test config to use MeetingFrequency. |
| src/ai_company/observability/events/meeting.py | Adds scheduler/API observability event constants. |
| src/ai_company/communication/meeting/scheduler.py | Implements MeetingScheduler with periodic tasks, event trigger execution, publishing. |
| src/ai_company/communication/meeting/participant.py | Introduces participant resolver protocol + registry-based resolver implementation. |
| src/ai_company/communication/meeting/frequency.py | Introduces MeetingFrequency and interval conversion helper. |
| src/ai_company/communication/meeting/errors.py | Adds scheduler-specific error types. |
| src/ai_company/communication/meeting/init.py | Re-exports new meeting scheduler/resolver/frequency symbols. |
| src/ai_company/communication/config.py | Switches meeting type frequency field to `MeetingFrequency |
| src/ai_company/api/ws_models.py | Adds meeting WS event types. |
| src/ai_company/api/state.py | Adds meeting orchestrator/scheduler to AppState with 503 enforcement helpers. |
| src/ai_company/api/controllers/meetings.py | Implements meetings controller + bounded trigger request validation. |
| src/ai_company/api/channels.py | Adds meetings WS channel constant and includes it in ALL_CHANNELS. |
| src/ai_company/api/app.py | Wires meeting scheduler into startup/shutdown and app factory signature. |
| docs/design/communication.md | Documents scheduler responsibilities, modes, and participant resolution rules. |
| cli/.golangci.yml | Updates golangci-lint v2 config (formatters section, removes incompatible linter entries). |
| CLAUDE.md | Updates repo architecture/docs and event-constant guidance to include meeting scheduler events. |
| .github/workflows/cli.yml | Fixes Go workflow shell usage for cross-platform execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+151
to
+155
| if isinstance(result, Exception): | ||
| logger.warning( | ||
| MEETING_SCHEDULER_ERROR, | ||
| note="periodic task error during shutdown", | ||
| exc_info=result, |
| ) | ||
| resolved = [] | ||
| elif isinstance(val, (list, tuple)): | ||
| resolved = [v for v in val if isinstance(v, str) and v.strip()] |
Comment on lines
+45
to
+60
| onMounted(async () => { | ||
| try { | ||
| if (authStore.token && !wsStore.connected) { | ||
| wsStore.connect(authStore.token) | ||
| } | ||
| wsStore.subscribe(['meetings']) | ||
| wsStore.onChannelEvent('meetings', meetingStore.handleWsEvent) | ||
| } catch (err) { | ||
| console.error('WebSocket setup failed:', sanitizeForLog(err)) | ||
| } | ||
| try { | ||
| await meetingStore.fetchMeetings() | ||
| } catch (err) { | ||
| console.error('Initial data fetch failed:', sanitizeForLog(err)) | ||
| } | ||
| }) |
Greptile SummaryThis PR delivers a well-structured meeting scheduler system — periodic asyncio tasks, event-triggered meetings, a REST/WS API, and a Vue frontend — on top of a clean Key findings:
Confidence Score: 2/5
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/ai_company/communication/meeting/scheduler.py
Line: 423-446
Comment:
**`meeting.started` payload missing `meeting_id` — frontend handler is dead code**
`_publish_started_event` publishes a payload of `{"meeting_type": ..., "status": "in_progress"}` — deliberately before the meeting runs, so no `meeting_id` exists yet.
However, in `web/src/stores/meetings.ts` the `handleWsEvent` handler for `meeting.started` does:
```ts
case 'meeting.started': {
const meetingId = payload.meeting_id // always undefined here
if (typeof meetingId === 'string' && meetingId) {
void _refreshMeeting(meetingId) // never reached
}
break
}
```
Because `meeting_id` is absent from the payload, `typeof undefined === 'string'` is `false`, so `_refreshMeeting` is **never called** for `meeting.started` events. The entire case branch is dead code.
Options to fix:
1. Keep the event but emit it after the orchestrator creates an ID (i.e. from within `run_meeting` at the point a `meeting_id` is assigned) so the payload can include it.
2. Since a `meeting_id` cannot exist before the meeting runs, remove the `meeting_id` check from the `meeting.started` branch and instead refresh the full list (`fetchMeetings`) on this event rather than a single record.
3. Drop `meeting.started` until a `meeting_id` can be provided in the payload.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/ai_company/api/controllers/meetings.py
Line: 170-174
Comment:
**HTTP request blocks until all triggered meetings complete**
`trigger_event` calls `orchestrator.run_meeting(...)` for each matching meeting type inside an `asyncio.TaskGroup`, and the `trigger_meeting` endpoint `await`s the full result before sending a response.
Since LLM-driven meetings can take minutes per meeting, and multiple meetings can match the same event, the HTTP connection will be held open for the sum of all execution times. In practice this means:
- HTTP clients (browsers, API callers, load balancers) will time out long before meetings finish.
- The caller receives no response at all, making it impossible to tell if the trigger succeeded.
The typical pattern for long-running triggers is to fire the tasks as background work and return immediately with an accepted status. For example, create the tasks with `asyncio.create_task` (storing them in `_tasks` so `stop()` can cancel them), return 202 Accepted with the meeting IDs that will be populated, and let clients poll `GET /meetings` or subscribe to the `meetings` WS channel for completion events.
```python
# Instead of awaiting the full execution:
records = await scheduler.trigger_event(data.event_name, context=data.context)
return Response(content=..., status_code=200)
# Fire and return 202 immediately, letting WS events deliver the result:
asyncio.create_task(scheduler.trigger_event(data.event_name, context=data.context))
return Response(content=..., status_code=202)
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/ai_company/api/app.py
Line: 484-487
Comment:
**Direct private attribute mutation — expose a setter instead**
The factory directly writes to `meeting_scheduler._event_publisher` (a private slot), requiring `noqa: SLF001` on both lines. `MeetingScheduler` already receives `event_publisher` as a constructor keyword argument (defaulting to `None`) but there is no public setter.
A `set_event_publisher` method (or accepting it directly in `create_app` before constructing the scheduler) would remove the need to bypass encapsulation here:
```python
# In MeetingScheduler:
def set_event_publisher(
self, publisher: Callable[[str, dict[str, Any]], None]
) -> None:
"""Wire the WebSocket event publisher (called once, during app creation)."""
if self._event_publisher is not None:
msg = "Event publisher already set"
raise RuntimeError(msg)
self._event_publisher = publisher
```
Then in `create_app`:
```python
if meeting_scheduler is not None and meeting_scheduler._event_publisher is None:
meeting_scheduler.set_event_publisher(_make_meeting_publisher(channels_plugin))
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/ai_company/api/app.py
Line: 126-140
Comment:
**`_on_meeting_event` missing error handling — inconsistent with `_make_expire_callback`**
`_make_expire_callback` wraps its `channels_plugin.publish()` call in a `try/except` and logs failures as warnings (best-effort semantics built into the callback itself). `_on_meeting_event` has no equivalent guard — exceptions propagate to the scheduler's `_publish_started_event` / `_publish_meeting_event` callers, which do catch them, so the system survives. However:
1. The two publisher factories have inconsistent contracts (one is self-contained best-effort, one relies on callers for safety).
2. A `ValueError` from `WsEventType(event_name)` with an unrecognised string will propagate and be logged as a generic scheduler warning rather than a more specific publish failure.
Consider adding the same defensive wrapper:
```python
def _on_meeting_event(event_name: str, payload: dict[str, Any]) -> None:
try:
event = WsEvent(
event_type=WsEventType(event_name),
channel=CHANNEL_MEETINGS,
timestamp=datetime.now(UTC),
payload=payload,
)
channels_plugin.publish(event.model_dump_json(), channels=[CHANNEL_MEETINGS])
except MemoryError, RecursionError:
raise
except Exception:
logger.warning(
API_APPROVAL_PUBLISH_FAILED,
event_name=event_name,
exc_info=True,
)
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 17d0f35 |
Comment on lines
50
to
+52
| COORDINATION_FAILED = "coordination.failed" | ||
|
|
||
| MEETING_STARTED = "meeting.started" |
There was a problem hiding this comment.
meeting.started event is never published
WsEventType.MEETING_STARTED is defined here and handled in the frontend store (handleWsEvent has a case 'meeting.started' branch), but the scheduler's _STATUS_TO_WS_EVENT dict only maps "completed", "failed", and "budget_exhausted" — and _publish_meeting_event is only called after execution finishes in _run_and_publish. There is no code path that emits meeting.started at any point.
This creates a permanent divergence between the declared protocol and the runtime behaviour. Either:
- Add a publish call in
_run_and_publishbefore awaitingorchestrator.run_meeting()and wire the"in_progress"status to"meeting.started"in_STATUS_TO_WS_EVENT, or - Remove
MEETING_STARTEDfromWsEventTypeandmeeting.startedfrom the TypeScriptWsEventTypeunion and the frontendhandleWsEventswitch until it is implemented.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/api/ws_models.py
Line: 50-52
Comment:
**`meeting.started` event is never published**
`WsEventType.MEETING_STARTED` is defined here and handled in the frontend store (`handleWsEvent` has a `case 'meeting.started'` branch), but the scheduler's `_STATUS_TO_WS_EVENT` dict only maps `"completed"`, `"failed"`, and `"budget_exhausted"` — and `_publish_meeting_event` is only called after execution finishes in `_run_and_publish`. There is no code path that emits `meeting.started` at any point.
This creates a permanent divergence between the declared protocol and the runtime behaviour. Either:
- Add a publish call in `_run_and_publish` before awaiting `orchestrator.run_meeting()` and wire the `"in_progress"` status to `"meeting.started"` in `_STATUS_TO_WS_EVENT`, or
- Remove `MEETING_STARTED` from `WsEventType` and `meeting.started` from the TypeScript `WsEventType` union and the frontend `handleWsEvent` switch until it is implemented.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ai_company/api/app.py`:
- Around line 403-404: The MeetingScheduler is never given an event publisher so
it cannot emit lifecycle events to WebSocket clients; create a publisher from
channels_plugin scoped to the "meetings" channel (e.g. call the channels_plugin
publisher factory/creator API) and pass that publisher into the MeetingScheduler
instance (either via its constructor or the scheduler.set_event_publisher
method) when wiring up MeetingScheduler alongside MeetingOrchestrator; ensure
the same publisher is used in the lifecycle hooks that start/stop the scheduler
so published events go to the "meetings" channel.
In `@src/ai_company/api/controllers/meetings.py`:
- Around line 175-179: Remove the duplicate pre-trigger telemetry by deleting
the logger.info call that emits MEETING_EVENT_TRIGGERED in the meetings
controller (the logger.info block that passes event_name=data.event_name and
source="api"); MeetingScheduler.trigger_event() already emits this event after
matching configs, so remove this logging statement to avoid double-counting and
false positives while leaving other logging and the trigger_event call intact.
In `@src/ai_company/communication/meeting/frequency.py`:
- Around line 34-40: The module-level mapping _FREQUENCY_SECONDS should be made
immutable: construct the dict using the existing keys (MeetingFrequency,
_SECONDS_PER_DAY, _SECONDS_PER_WEEK, _SECONDS_PER_BI_WEEK, _SECONDS_PER_MONTH,
_SECONDS_PER_DAY for PER_SPRINT_DAY), perform a deepcopy of that constructed
dict, then wrap it with mapping proxy semantics (MappingProxyType) so
_FREQUENCY_SECONDS is a read-only mapping at import time and cannot be mutated
at runtime; update any imports to include MappingProxyType and copy.deepcopy as
needed and keep the symbol name _FREQUENCY_SECONDS unchanged so existing lookups
work.
In `@src/ai_company/communication/meeting/participant.py`:
- Around line 138-139: The list/tuple branch currently filters values with
v.strip() but returns the raw items, which preserves surrounding whitespace;
update the branch that sets resolved from val so it returns normalized IDs
(e.g., apply v.strip() and keep only non-empty strings) — locate the code
handling isinstance(val, (list, tuple)) and change the list comprehension that
builds resolved to use v.strip() (and possibly deduplicate if desired) so
downstream matching receives clean IDs like "agent-7" instead of " agent-7 ".
- Around line 110-171: The _resolve_entry method is too long and mixes multiple
resolution paths; split it into smaller helpers: extract context resolution into
a new private method (e.g., _resolve_from_context(entry, ctx)) that implements
the ctx lookup logic (string, list/tuple, unexpected types) and returns
list[str], and extract registry-based resolution into another private method
(e.g., _resolve_from_registry(entry)) that handles the "all" case (calling
self._registry.list_active()), department lookup
(self._registry.list_by_department(entry)), and name lookup
(self._registry.get_by_name(entry)); then have _resolve_entry call these helpers
in order and keep its body under 50 lines while preserving existing logging
calls (MEETING_NO_PARTICIPANTS, MEETING_PARTICIPANTS_RESOLVED) and return
semantics.
In `@src/ai_company/communication/meeting/scheduler.py`:
- Around line 41-47: The WS event mapping and publish flow only emits terminal
events because _publish_meeting_event() is called after run_meeting() and
_STATUS_TO_WS_EVENT only contains terminal keys; add a pre-run publish to emit
the "meeting.started" event by either extending _STATUS_TO_WS_EVENT with a
"started" key mapped to "meeting.started" and invoking
_publish_meeting_event("started", ...) before run_meeting(), or create a
dedicated pre-run hook in the scheduler/orchestrator that calls the WS publisher
with the "meeting.started" event prior to calling run_meeting(); update any
scheduler paths that start meetings (e.g., the function that
schedules/orchestrates run_meeting()) to call this pre-run publisher so
scheduled meetings can emit meeting.started.
- Around line 192-197: The current code in the block that creates tasks with
asyncio.TaskGroup hides failures by filtering out None results from
self._execute_meeting(), so callers (e.g., trigger_event) cannot distinguish “no
matches” from “all matches failed.” Change the logic after the TaskGroup to
explicitly handle task outcomes: collect each tg-created task, check
t.exception() and, if present, either raise that exception (re-raising
immediately) or convert it into an explicit failure record (including the
matching trigger identifier and error) to return alongside successful results;
treat a literal None returned by _execute_meeting() as an explicit failure
record rather than silently dropping it. Ensure you update the method that calls
this code (trigger_event) to expect the new explicit failure records or
propagate exceptions accordingly.
In `@tests/unit/communication/meeting/test_participant.py`:
- Around line 150-160: Move the local import of NoParticipantsResolvedError out
of the test body into the module-level imports for consistency; specifically,
add "from ai_company.communication.meeting.errors import
NoParticipantsResolvedError" alongside the other imports at top of the file and
remove the in-function import inside test_empty_result_raises which calls await
resolver.resolve(()), leaving the test otherwise unchanged.
In `@web/src/__tests__/views/MeetingLogsPage.store-integration.test.ts`:
- Around line 16-22: The test currently only checks that useMeetingStore exposes
methods (fetchMeetings, fetchMeeting, triggerMeeting, handleWsEvent); replace it
with behavior-focused assertions: mock the API calls and websocket events used
by fetchMeetings/fetchMeeting/triggerMeeting/handleWsEvent, invoke each method,
and assert the store's state changes (e.g., meetings list, currentMeeting,
loading/error flags) and that API/WS mocks were called with expected args;
ensure you test that triggerMeeting causes the appropriate refresh (e.g., calls
fetchMeetings or updates state) and that handleWsEvent processes a ws message to
update the store.
In `@web/src/views/MeetingLogsPage.vue`:
- Around line 221-237: The template uses optional chaining on
selected.minutes.decisions and selected.minutes.action_items but the
MeetingMinutes type declares them as required arrays; either update the
MeetingMinutes interface to make decisions and action_items optional
(decisions?: string[]; action_items?: ActionItem[]) so the template can safely
use ?.length, or remove the defensive ?. operators in the template (the v-if
checks) to match the current required-array types; locate the MeetingMinutes
type and the template references to selected.minutes.decisions /
selected.minutes.action_items and apply the change consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8b650e6-a9fd-4524-9b7b-b7536d50cba3
📒 Files selected for processing (34)
.github/workflows/cli.ymlCLAUDE.mdcli/.golangci.ymldocs/design/communication.mdsrc/ai_company/api/app.pysrc/ai_company/api/channels.pysrc/ai_company/api/controllers/meetings.pysrc/ai_company/api/state.pysrc/ai_company/api/ws_models.pysrc/ai_company/communication/config.pysrc/ai_company/communication/meeting/__init__.pysrc/ai_company/communication/meeting/errors.pysrc/ai_company/communication/meeting/frequency.pysrc/ai_company/communication/meeting/participant.pysrc/ai_company/communication/meeting/scheduler.pysrc/ai_company/observability/events/meeting.pytests/integration/communication/test_meeting_integration.pytests/unit/api/controllers/test_meetings.pytests/unit/api/test_app.pytests/unit/api/test_channels.pytests/unit/communication/conftest.pytests/unit/communication/meeting/test_config_frequency.pytests/unit/communication/meeting/test_frequency.pytests/unit/communication/meeting/test_participant.pytests/unit/communication/meeting/test_scheduler.pytests/unit/communication/test_config.pyweb/src/__tests__/stores/meetings.test.tsweb/src/__tests__/views/MeetingLogsPage.store-integration.test.tsweb/src/__tests__/views/MeetingLogsPage.test.tsweb/src/api/endpoints/meetings.tsweb/src/api/types.tsweb/src/stores/meetings.tsweb/src/styles/theme.tsweb/src/views/MeetingLogsPage.vue
💤 Files with no reviewable changes (1)
- web/src/tests/views/MeetingLogsPage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Agent
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{py,ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{py,ts,tsx,js,jsx,vue}: Enforce 88-character line length via ruff
Never use real vendor names (Anthropic, OpenAI, Claude, GPT, etc.) in project-owned code, docstrings, comments, tests, or config examples. Use generic names:example-provider,example-large-001,example-medium-001,example-small-001,large/medium/smallas aliases. Tests must usetest-provider,test-small-001, etc.
Files:
web/src/api/endpoints/meetings.tssrc/ai_company/communication/meeting/errors.pyweb/src/styles/theme.tstests/unit/api/controllers/test_meetings.pysrc/ai_company/api/ws_models.pysrc/ai_company/communication/config.pysrc/ai_company/communication/meeting/scheduler.pytests/unit/communication/conftest.pysrc/ai_company/communication/meeting/participant.pyweb/src/__tests__/views/MeetingLogsPage.store-integration.test.tstests/integration/communication/test_meeting_integration.pyweb/src/__tests__/stores/meetings.test.tsweb/src/api/types.tssrc/ai_company/observability/events/meeting.pyweb/src/stores/meetings.tstests/unit/api/test_channels.pysrc/ai_company/communication/meeting/frequency.pytests/unit/communication/meeting/test_participant.pysrc/ai_company/api/state.pysrc/ai_company/api/channels.pysrc/ai_company/api/app.pytests/unit/api/test_app.pyweb/src/views/MeetingLogsPage.vuesrc/ai_company/api/controllers/meetings.pytests/unit/communication/meeting/test_scheduler.pytests/unit/communication/meeting/test_frequency.pytests/unit/communication/meeting/test_config_frequency.pysrc/ai_company/communication/meeting/__init__.pytests/unit/communication/test_config.py
**/*.{py,ts,tsx,js,jsx,vue,go}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{py,ts,tsx,js,jsx,vue,go}: Functions must be less than 50 lines; files must be less than 800 lines
Handle errors explicitly; never silently swallow exceptions
Validate at system boundaries: user input, external APIs, config files
Files:
web/src/api/endpoints/meetings.tssrc/ai_company/communication/meeting/errors.pyweb/src/styles/theme.tstests/unit/api/controllers/test_meetings.pysrc/ai_company/api/ws_models.pysrc/ai_company/communication/config.pysrc/ai_company/communication/meeting/scheduler.pytests/unit/communication/conftest.pysrc/ai_company/communication/meeting/participant.pyweb/src/__tests__/views/MeetingLogsPage.store-integration.test.tstests/integration/communication/test_meeting_integration.pyweb/src/__tests__/stores/meetings.test.tsweb/src/api/types.tssrc/ai_company/observability/events/meeting.pyweb/src/stores/meetings.tstests/unit/api/test_channels.pysrc/ai_company/communication/meeting/frequency.pytests/unit/communication/meeting/test_participant.pysrc/ai_company/api/state.pysrc/ai_company/api/channels.pysrc/ai_company/api/app.pytests/unit/api/test_app.pyweb/src/views/MeetingLogsPage.vuesrc/ai_company/api/controllers/meetings.pytests/unit/communication/meeting/test_scheduler.pytests/unit/communication/meeting/test_frequency.pytests/unit/communication/meeting/test_config_frequency.pysrc/ai_company/communication/meeting/__init__.pytests/unit/communication/test_config.py
web/**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Web dashboard linting:
npm --prefix web run lintvia ESLint
Files:
web/src/api/endpoints/meetings.tsweb/src/styles/theme.tsweb/src/__tests__/views/MeetingLogsPage.store-integration.test.tsweb/src/__tests__/stores/meetings.test.tsweb/src/api/types.tsweb/src/stores/meetings.tsweb/src/views/MeetingLogsPage.vue
web/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
web/**/*.{ts,tsx,vue}: Web dashboard type checking:npm --prefix web run type-checkvia vue-tsc
Use TypeScript in Vue components and web dashboard for type safety
Files:
web/src/api/endpoints/meetings.tsweb/src/styles/theme.tsweb/src/__tests__/views/MeetingLogsPage.store-integration.test.tsweb/src/__tests__/stores/meetings.test.tsweb/src/api/types.tsweb/src/stores/meetings.tsweb/src/views/MeetingLogsPage.vue
web/**/*.{ts,tsx,js,jsx,vue,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Web dashboard uses PrimeVue components, Tailwind CSS for styling, Pinia for state management, Vue Router for routing, and Axios for API calls
Files:
web/src/api/endpoints/meetings.tsweb/src/styles/theme.tsweb/src/__tests__/views/MeetingLogsPage.store-integration.test.tsweb/src/__tests__/stores/meetings.test.tsweb/src/api/types.tsweb/src/stores/meetings.tsweb/src/views/MeetingLogsPage.vue
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Nofrom __future__ import annotations— Python 3.14 has PEP 649 native lazy annotations
Useexcept A, B:(no parentheses) syntax for exception handling — ruff enforces PEP 758 except syntax on Python 3.14
All public functions require type hints; mypy strict mode enforced
Google-style docstrings required on all public classes and functions; enforced by ruff D rules
Create new objects instead of mutating existing ones; for non-Pydantic internal collections, usecopy.deepcopy()at construction +MappingProxyTypewrapping for read-only enforcement
Use frozen Pydantic models for config/identity; separate mutable-via-copy models for runtime state usingmodel_copy(update=...); never mix static config fields with mutable runtime fields
Use Pydantic v2 withBaseModel,model_validator,computed_field,ConfigDict. Use@computed_fieldfor derived values; useNotBlankStrfor all identifier/name fields instead of manual whitespace validators
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code; avoid barecreate_taskand prefer structured concurrency
Python 3.14+ required; supports PEP 649 native lazy annotations
Files:
src/ai_company/communication/meeting/errors.pytests/unit/api/controllers/test_meetings.pysrc/ai_company/api/ws_models.pysrc/ai_company/communication/config.pysrc/ai_company/communication/meeting/scheduler.pytests/unit/communication/conftest.pysrc/ai_company/communication/meeting/participant.pytests/integration/communication/test_meeting_integration.pysrc/ai_company/observability/events/meeting.pytests/unit/api/test_channels.pysrc/ai_company/communication/meeting/frequency.pytests/unit/communication/meeting/test_participant.pysrc/ai_company/api/state.pysrc/ai_company/api/channels.pysrc/ai_company/api/app.pytests/unit/api/test_app.pysrc/ai_company/api/controllers/meetings.pytests/unit/communication/meeting/test_scheduler.pytests/unit/communication/meeting/test_frequency.pytests/unit/communication/meeting/test_config_frequency.pysrc/ai_company/communication/meeting/__init__.pytests/unit/communication/test_config.py
src/ai_company/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/ai_company/**/*.py: Every module with business logic must importfrom ai_company.observability import get_loggerand definelogger = get_logger(__name__); never useimport logging,logging.getLogger(), orprint()in application code
Always use constants from domain-specific event modules (e.g.,events.provider,events.budget,events.cfo,events.conflict) underai_company.observability.eventsfor event names; import directly asfrom ai_company.observability.events.<domain> import EVENT_CONSTANT
Use structured logging kwargs format:logger.info(EVENT, key=value); never use positional format strings likelogger.info('msg %s', val)
All error paths must log at WARNING or ERROR with context before raising
All state transitions must log at INFO level
DEBUG level for object creation, internal flow, entry/exit of key functions
Pure data models, enums, and re-exports do not need logging
Files:
src/ai_company/communication/meeting/errors.pysrc/ai_company/api/ws_models.pysrc/ai_company/communication/config.pysrc/ai_company/communication/meeting/scheduler.pysrc/ai_company/communication/meeting/participant.pysrc/ai_company/observability/events/meeting.pysrc/ai_company/communication/meeting/frequency.pysrc/ai_company/api/state.pysrc/ai_company/api/channels.pysrc/ai_company/api/app.pysrc/ai_company/api/controllers/meetings.pysrc/ai_company/communication/meeting/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e,@pytest.mark.slow
Prefer@pytest.mark.parametrizefor testing similar cases
Files:
tests/unit/api/controllers/test_meetings.pytests/unit/communication/conftest.pytests/integration/communication/test_meeting_integration.pytests/unit/api/test_channels.pytests/unit/communication/meeting/test_participant.pytests/unit/api/test_app.pytests/unit/communication/meeting/test_scheduler.pytests/unit/communication/meeting/test_frequency.pytests/unit/communication/meeting/test_config_frequency.pytests/unit/communication/test_config.py
web/**/__tests__/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Web dashboard testing:
npm --prefix web run testvia Vitest
Files:
web/src/__tests__/views/MeetingLogsPage.store-integration.test.tsweb/src/__tests__/stores/meetings.test.ts
🧠 Learnings (17)
📚 Learning: 2026-03-14T16:17:59.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.180Z
Learning: CLI workflow (`.github/workflows/cli.yml`): Go lint (golangci-lint + go vet) + test (race + coverage) + cross-compile build (linux/darwin/windows × amd64/arm64) + govulncheck on `cli/**` changes. GoReleaser release on `v*` tags (attaches assets, pins checksums in install scripts, appends table to Release notes).
Applied to files:
.github/workflows/cli.ymlcli/.golangci.yml
📚 Learning: 2026-03-14T16:17:59.180Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.180Z
Learning: Coverage: Codecov integration with coverage upload + test results analytics via codecov-action + test-results-action (best-effort, CI not gated on Codecov availability)
Applied to files:
.github/workflows/cli.yml
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Pre-push hooks enforce: mypy type-check + pytest unit tests + golangci-lint + go vet + go test (CLI, conditional on `cli/**/*.go`)
Applied to files:
.github/workflows/cli.ymlcli/.golangci.yml
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: CLI tests: `cd cli && go test ./...` to run all Go tests
Applied to files:
.github/workflows/cli.yml
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: CLI linting: `cd cli && golangci-lint run` for comprehensive Go linting
Applied to files:
.github/workflows/cli.ymlcli/.golangci.yml
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Full test suite with coverage: `uv run pytest tests/ -n auto --cov=ai_company --cov-fail-under=80`
Applied to files:
.github/workflows/cli.yml
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to cli/**/*.go : Go 1.26+ required for CLI development
Applied to files:
cli/.golangci.yml
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: CLI vetting: `cd cli && go vet ./...` for Go analysis
Applied to files:
cli/.golangci.yml
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to web/**/__tests__/**/*.{ts,tsx,js} : Web dashboard testing: `npm --prefix web run test` via Vitest
Applied to files:
web/src/__tests__/views/MeetingLogsPage.store-integration.test.ts
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to src/ai_company/**/*.py : Always use constants from domain-specific event modules (e.g., `events.provider`, `events.budget`, `events.cfo`, `events.conflict`) under `ai_company.observability.events` for event names; import directly as `from ai_company.observability.events.<domain> import EVENT_CONSTANT`
Applied to files:
src/ai_company/observability/events/meeting.pysrc/ai_company/api/app.pyCLAUDE.md
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to src/ai_company/**/*.py : Every module with business logic must import `from ai_company.observability import get_logger` and define `logger = get_logger(__name__)`; never use `import logging`, `logging.getLogger()`, or `print()` in application code
Applied to files:
src/ai_company/api/state.pysrc/ai_company/api/app.pyCLAUDE.md
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to web/**/*.{ts,tsx,js,jsx,vue,json} : Web dashboard uses PrimeVue components, Tailwind CSS for styling, Pinia for state management, Vue Router for routing, and Axios for API calls
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to src/ai_company/**/*.py : Use structured logging kwargs format: `logger.info(EVENT, key=value)`; never use positional format strings like `logger.info('msg %s', val)`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to src/ai_company/**/*.py : All error paths must log at WARNING or ERROR with context before raising
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to src/ai_company/**/*.py : All state transitions must log at INFO level
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to src/ai_company/**/*.py : Pure data models, enums, and re-exports do not need logging
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to src/ai_company/**/*.py : DEBUG level for object creation, internal flow, entry/exit of key functions
Applied to files:
CLAUDE.md
🧬 Code graph analysis (17)
web/src/api/endpoints/meetings.ts (2)
web/src/api/types.ts (5)
MeetingFilters(570-575)PaginatedResponse(105-107)MeetingRecord(560-568)ApiResponse(94-96)TriggerMeetingRequest(577-580)web/src/api/client.ts (3)
apiClient(12-16)unwrapPaginated(68-84)unwrap(56-62)
web/src/styles/theme.ts (1)
web/src/api/types.ts (4)
TaskStatus(5-14)ApprovalStatus(28-28)AgentStatus(42-42)MeetingStatus(492-498)
tests/unit/api/controllers/test_meetings.py (2)
src/ai_company/api/app.py (1)
create_app(392-539)src/ai_company/api/controllers/meetings.py (1)
TriggerMeetingRequest(29-74)
src/ai_company/communication/config.py (1)
src/ai_company/communication/meeting/frequency.py (1)
MeetingFrequency(16-31)
src/ai_company/communication/meeting/scheduler.py (6)
src/ai_company/communication/meeting/errors.py (2)
NoParticipantsResolvedError(34-35)SchedulerAlreadyRunningError(38-39)src/ai_company/communication/meeting/frequency.py (1)
frequency_to_seconds(48-57)src/ai_company/communication/meeting/orchestrator.py (1)
run_meeting(103-181)src/ai_company/communication/meeting/participant.py (2)
resolve(24-43)resolve(65-108)src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/communication/config.py (1)
MeetingTypeConfig(83-137)
tests/unit/communication/conftest.py (1)
src/ai_company/communication/meeting/frequency.py (1)
MeetingFrequency(16-31)
src/ai_company/communication/meeting/participant.py (2)
src/ai_company/communication/meeting/errors.py (1)
NoParticipantsResolvedError(34-35)src/ai_company/hr/registry.py (3)
list_active(129-138)list_by_department(140-158)get_by_name(113-127)
web/src/__tests__/views/MeetingLogsPage.store-integration.test.ts (1)
web/src/stores/meetings.ts (1)
useMeetingStore(12-117)
tests/integration/communication/test_meeting_integration.py (1)
src/ai_company/communication/meeting/frequency.py (1)
MeetingFrequency(16-31)
web/src/__tests__/stores/meetings.test.ts (2)
web/src/api/types.ts (2)
MeetingRecord(560-568)WsEvent(615-620)web/src/stores/meetings.ts (1)
useMeetingStore(12-117)
web/src/api/types.ts (1)
web/src/styles/theme.ts (1)
Priority(6-6)
src/ai_company/api/state.py (2)
src/ai_company/communication/meeting/orchestrator.py (1)
MeetingOrchestrator(68-457)src/ai_company/communication/meeting/scheduler.py (1)
MeetingScheduler(55-437)
src/ai_company/api/app.py (3)
src/ai_company/communication/meeting/orchestrator.py (1)
MeetingOrchestrator(68-457)src/ai_company/communication/meeting/scheduler.py (3)
MeetingScheduler(55-437)stop(138-160)start(98-136)src/ai_company/api/state.py (4)
persistence(110-112)meeting_scheduler(166-171)AppState(29-218)meeting_orchestrator(158-163)
src/ai_company/api/controllers/meetings.py (5)
src/ai_company/api/dto.py (2)
ApiResponse(39-57)PaginatedResponse(76-96)src/ai_company/api/guards.py (2)
require_read_access(83-107)require_write_access(56-80)src/ai_company/api/pagination.py (1)
paginate(26-58)src/ai_company/api/state.py (2)
meeting_orchestrator(158-163)meeting_scheduler(166-171)src/ai_company/communication/meeting/scheduler.py (1)
trigger_event(162-197)
tests/unit/communication/meeting/test_frequency.py (1)
src/ai_company/communication/meeting/frequency.py (2)
MeetingFrequency(16-31)frequency_to_seconds(48-57)
src/ai_company/communication/meeting/__init__.py (4)
src/ai_company/communication/meeting/errors.py (3)
MeetingSchedulerError(30-31)NoParticipantsResolvedError(34-35)SchedulerAlreadyRunningError(38-39)src/ai_company/communication/meeting/frequency.py (1)
MeetingFrequency(16-31)src/ai_company/communication/meeting/participant.py (2)
ParticipantResolver(21-43)RegistryParticipantResolver(46-171)src/ai_company/communication/meeting/scheduler.py (1)
MeetingScheduler(55-437)
tests/unit/communication/test_config.py (2)
src/ai_company/communication/meeting/frequency.py (1)
MeetingFrequency(16-31)src/ai_company/communication/config.py (1)
MeetingTypeConfig(83-137)
🔇 Additional comments (20)
.github/workflows/cli.yml (1)
93-96: LGTM!Explicitly specifying
shell: bashensures consistent shell behavior across all matrix platforms (ubuntu, macos, windows). This is a good fix since Windows runners default to PowerShell, and using bash provides uniformity for the test command execution.cli/.golangci.yml (1)
22-24: Formatter migration to v2 is correct and properly configured.The
formatters.enable: [gofmt]block follows golangci-lint v2 schema correctly. CI workflow is pinned to golangci-lint v2.11.3, and the.golangci.ymlconfiguration usesversion: "2"withgo: "1.26"as required.tests/unit/communication/meeting/test_frequency.py (1)
11-56: Strong coverage for enum parsing and interval conversion.This test set cleanly validates valid values, invalid value handling, and
allfrequency_to_secondsmappings.tests/integration/communication/test_meeting_integration.py (1)
19-19: Enum migration in integration tests is correctly applied.The updated import and usage keep this test aligned with the new
MeetingFrequencytype contract.Also applies to: 424-425
src/ai_company/api/ws_models.py (1)
52-54: Meeting WebSocket event types are well-scoped and consistent.These additions fit the existing event namespace and lifecycle naming pattern.
web/src/styles/theme.ts (1)
3-6: Status theme updates correctly include meeting states.The
Statustype andstatusColorsmap are updated consistently for
schedule-related UI badges.Also applies to: 60-61
tests/unit/communication/meeting/test_config_frequency.py (1)
10-72: Validation-focused tests are comprehensive and targeted.Good coverage of enum acceptance, coercion, and mutual exclusivity rules.
src/ai_company/communication/meeting/frequency.py (1)
16-32: Enum and conversion API design look solid.Public surface is typed, documented, and easy to consume from config/scheduler code.
Also applies to: 48-57
docs/design/communication.md (1)
374-408: Scheduler design documentation is clear and implementation-aligned.The added section captures scheduling modes and participant resolution behavior
in a way that matches the introduced architecture.tests/unit/communication/test_config.py (1)
21-21: Enum-based frequency migration is consistently applied across config tests.The updated inputs cover construction, validation, immutability, and
serialization paths without introducing gaps.Also applies to: 101-102, 116-117, 131-133, 144-145, 148-148, 151-153, 164-167, 175-178, 184-184, 190-198, 204-205, 209-210, 220-221, 245-246, 254-255, 260-261, 274-275
web/src/api/endpoints/meetings.ts (1)
1-29: LGTM!The API endpoints are well-structured with proper type annotations, URL encoding for path parameters, and consistent use of the unwrap utilities for error handling. The return types correctly match the expected API response structures.
web/src/__tests__/stores/meetings.test.ts (1)
1-208: LGTM!Comprehensive test coverage for the meeting store including initial state verification, WebSocket event handling (completed, started, failed), API fetch operations, deduplication logic, and error handling. The mock setup is clean with proper reset in
beforeEach.web/src/stores/meetings.ts (1)
1-117: LGTM!Well-structured Pinia store with proper separation of concerns. The error handling strategy is appropriate: public actions set the error state for UI feedback while the internal
_refreshMeetingsilently logs failures to prevent WebSocket events from disrupting the user experience. The deduplication logic intriggerMeetingis efficient.tests/unit/communication/meeting/test_participant.py (1)
26-160: LGTM on test coverage!Comprehensive test coverage for the participant resolver including context precedence, deduplication, various input types (list, tuple, literals), and error conditions. The async test patterns and mock assertions are correctly implemented.
web/src/views/MeetingLogsPage.vue (2)
45-60: Consider consolidating error handling in onMounted.The WS setup and initial fetch have separate try/catch blocks, which is good for isolation. However, if the WS setup fails (line 52-54), the page still attempts to fetch meetings, which is the correct resilient behavior. The implementation looks intentional.
127-287: LGTM on the Meeting Logs page implementation!Well-structured Vue component with comprehensive meeting management features:
- Real-time updates via WebSocket integration
- Status filtering with proper error feedback
- Trigger dialog with validation and loading states
- Detailed sidebar with nested minutes display
- Proper accessibility attributes (aria-label on filter)
src/ai_company/communication/meeting/__init__.py (1)
22-99: LGTM!The package
__init__.pycorrectly expands the public API surface with the new meeting scheduler components. Imports are properly organized, and__all__maintains alphabetical order for consistency.tests/unit/api/controllers/test_meetings.py (2)
1-322: LGTM on test coverage!Comprehensive test suite covering:
- CRUD operations (list, get by ID, 404 handling)
- Filtering by status and meeting_type
- Trigger endpoint integration with scheduler mock
- Service unavailability (503) handling
- Extensive
TriggerMeetingRequestvalidation including all boundary conditionsThe test fixtures are well-organized with proper auth seeding and mock configuration.
234-292: Boundary values in validation tests are correct.All constants match production code:
_MAX_CONTEXT_KEYS=20,_MAX_CONTEXT_KEY_LEN=256,_MAX_CONTEXT_VAL_LEN=1024,_MAX_CONTEXT_LIST_ITEMS=50.src/ai_company/api/state.py (1)
13-16: LGTM!The
AppStateextension for meeting services follows the established patterns used by other services:
- Proper
__slots__entries for memory efficiency- Optional constructor parameters with
Nonedefaults- Property accessors that use
_require_servicefor consistent 503 handling- Logging via
get_logger(__name__)per project guidelinesBased on learnings: "Every module with business logic must import
from ai_company.observability import get_loggerand definelogger = get_logger(__name__)" - this is correctly implemented at line 22-26.Also applies to: 52-53, 75-76, 89-90, 157-171
Comment on lines
+192
to
+197
| async with asyncio.TaskGroup() as tg: | ||
| tasks = [ | ||
| tg.create_task(self._execute_meeting(mt, context)) for mt in matching | ||
| ] | ||
|
|
||
| return tuple(r for t in tasks if (r := t.result()) is not None) |
There was a problem hiding this comment.
Don't collapse matching-trigger failures into an empty result.
Line 197 drops every None returned by _execute_meeting(), but this class returns None for participant-resolution, agenda-build, and orchestrator errors. That makes trigger_event() use the same () for both “nothing matched” and “every matching meeting failed”, so POST /meetings/trigger can report a successful empty response while all matching triggers actually errored. Return explicit failure records, or raise once any matched trigger fails.
As per coding guidelines "Handle errors explicitly; never silently swallow exceptions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/communication/meeting/scheduler.py` around lines 192 - 197,
The current code in the block that creates tasks with asyncio.TaskGroup hides
failures by filtering out None results from self._execute_meeting(), so callers
(e.g., trigger_event) cannot distinguish “no matches” from “all matches failed.”
Change the logic after the TaskGroup to explicitly handle task outcomes: collect
each tg-created task, check t.exception() and, if present, either raise that
exception (re-raising immediately) or convert it into an explicit failure record
(including the matching trigger identifier and error) to return alongside
successful results; treat a literal None returned by _execute_meeting() as an
explicit failure record rather than silently dropping it. Ensure you update the
method that calls this code (trigger_event) to expect the new explicit failure
records or propagate exceptions accordingly.
Comment on lines
+16
to
+22
| it('meeting store has expected API methods', () => { | ||
| const store = useMeetingStore() | ||
| expect(typeof store.fetchMeetings).toBe('function') | ||
| expect(typeof store.fetchMeeting).toBe('function') | ||
| expect(typeof store.triggerMeeting).toBe('function') | ||
| expect(typeof store.handleWsEvent).toBe('function') | ||
| }) |
There was a problem hiding this comment.
This integration test is too shallow to catch store regressions.
The test only asserts method presence. It won’t fail if API calls, state updates, or WS-triggered refresh behavior break.
✅ Suggested behavior-focused test
+import * as meetingsApi from '@/api/endpoints/meetings'
...
-it('meeting store has expected API methods', () => {
+it('fetchMeetings calls API and updates state', async () => {
const store = useMeetingStore()
- expect(typeof store.fetchMeetings).toBe('function')
- expect(typeof store.fetchMeeting).toBe('function')
- expect(typeof store.triggerMeeting).toBe('function')
- expect(typeof store.handleWsEvent).toBe('function')
+ await store.fetchMeetings()
+ expect(meetingsApi.listMeetings).toHaveBeenCalledOnce()
+ expect(store.meetings).toEqual([])
+ expect(store.total).toBe(0)
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/__tests__/views/MeetingLogsPage.store-integration.test.ts` around
lines 16 - 22, The test currently only checks that useMeetingStore exposes
methods (fetchMeetings, fetchMeeting, triggerMeeting, handleWsEvent); replace it
with behavior-focused assertions: mock the API calls and websocket events used
by fetchMeetings/fetchMeeting/triggerMeeting/handleWsEvent, invoke each method,
and assert the store's state changes (e.g., meetings list, currentMeeting,
loading/error flags) and that API/WS mocks were called with expected args;
ensure you test that triggerMeeting causes the appropriate refresh (e.g., calls
fetchMeetings or updates state) and that handleWsEvent processes a ws message to
update the store.
- Add `_, _ =` to all unchecked fmt.Fprintln/fmt.Fprintf calls
- Use `defer func() { _ = resp.Body.Close() }()` for HTTP responses
- Add `_ =` to tmpFile.Close/os.Remove in cleanup paths
- Fix unchecked os.Remove in selfupdate test
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cli/cmd/init.go (1)
46-52:⚠️ Potential issue | 🟡 MinorDuplicate comment on lines 46-47.
There's a duplicated comment line. Remove one of them.
Proposed fix
// Warn if re-initializing over existing config (JWT secret will change). - // Warn if re-initializing over existing config (JWT secret will change). // isInteractive() is already checked at function entry, so prompt is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/init.go` around lines 46 - 52, Remove the duplicated comment above the config overwrite check: delete one of the repeated "// Warn if re-initializing over existing config (JWT secret will change)." lines found immediately before the if existing := config.StatePath(state.DataDir); fileExists(existing) { block so only a single comment remains; the rest of the logic (isInteractive() check at function entry, the if block using config.StatePath, fileExists, and fmt.Fprintf to cmd.ErrOrStderr()) should be left unchanged.cli/cmd/doctor.go (1)
27-84: 🧹 Nitpick | 🔵 TrivialFunction
runDoctorexceeds 50-line limit (58 lines).Consider extracting the GitHub issue URL generation (lines 52-79) into a separate helper function to improve readability and comply with the 50-line guideline.
Suggested extraction
+func buildIssueURL(report diagnostics.Report) string { + issueTitle := fmt.Sprintf("[CLI] Bug report — %s/%s, CLI %s", report.OS, report.Arch, report.CLIVersion) + issueBody := fmt.Sprintf( + "## Environment\n\nOS: %s/%s\nCLI: %s (%s)\nDocker: %s\nCompose: %s\nHealth: %s\n\n"+ + "> Full diagnostics saved to: attach the file from `synthorg doctor` output\n\n"+ + "## Steps to Reproduce\n\n1. \n\n## Expected Behavior\n\n\n## Actual Behavior\n\n", + report.OS, report.Arch, report.CLIVersion, report.CLICommit, + report.DockerVersion, report.ComposeVersion, report.HealthStatus, + ) + + encodedBody := url.QueryEscape(issueBody) + if len(encodedBody) > 3500 { + issueBody = fmt.Sprintf( + "## Environment\n\nOS: %s/%s\nCLI: %s\nDocker: %s\nHealth: %s\n\n"+ + "> Attach the full diagnostics file from `synthorg doctor` output\n", + report.OS, report.Arch, report.CLIVersion, + report.DockerVersion, report.HealthStatus, + ) + encodedBody = url.QueryEscape(issueBody) + } + + return fmt.Sprintf( + "%s/issues/new?title=%s&labels=type%%3Abug&body=%s", + version.RepoURL, + url.QueryEscape(issueTitle), + encodedBody, + ) +}Then in
runDoctor:_, _ = fmt.Fprintln(out, text) - // Generate GitHub issue URL (exclude logs — may contain secrets). - issueTitle := fmt.Sprintf("[CLI] Bug report — %s/%s, CLI %s", report.OS, report.Arch, report.CLIVersion) - issueBody := fmt.Sprintf( - "## Environment\n\nOS: %s/%s\nCLI: %s (%s)\nDocker: %s\nCompose: %s\nHealth: %s\n\n"+ - "> Full diagnostics saved to: attach the file from `synthorg doctor` output\n\n"+ - "## Steps to Reproduce\n\n1. \n\n## Expected Behavior\n\n\n## Actual Behavior\n\n", - report.OS, report.Arch, report.CLIVersion, report.CLICommit, - report.DockerVersion, report.ComposeVersion, report.HealthStatus, - ) - - // Truncate body if URL would exceed browser limits (~4000 chars). - encodedBody := url.QueryEscape(issueBody) - if len(encodedBody) > 3500 { - issueBody = fmt.Sprintf( - "## Environment\n\nOS: %s/%s\nCLI: %s\nDocker: %s\nHealth: %s\n\n"+ - "> Attach the full diagnostics file from `synthorg doctor` output\n", - report.OS, report.Arch, report.CLIVersion, - report.DockerVersion, report.HealthStatus, - ) - encodedBody = url.QueryEscape(issueBody) - } - - issueURL := fmt.Sprintf( - "%s/issues/new?title=%s&labels=type%%3Abug&body=%s", - version.RepoURL, - url.QueryEscape(issueTitle), - encodedBody, - ) - - _, _ = fmt.Fprintf(out, "File a bug report:\n %s\n", issueURL) + // Generate GitHub issue URL (exclude logs — may contain secrets). + _, _ = fmt.Fprintf(out, "File a bug report:\n %s\n", buildIssueURL(report))As per coding guidelines: "Functions must be less than 50 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/doctor.go` around lines 27 - 84, runDoctor is over the 50-line limit because the GitHub issue URL construction is inline; extract that logic into a new helper (e.g., buildGitHubIssueURL or createIssueURL) that accepts the diagnostics report (and any needed version.RepoURL constant) and returns the final issue URL string. Move the creation of issueTitle, issueBody, the QueryEscape/truncation logic, and the final fmt.Sprintf into that helper, leaving runDoctor to call buildGitHubIssueURL(report) and print the returned URL; update imports/usages as needed and keep function behavior unchanged.cli/internal/selfupdate/updater.go (1)
224-292: 🛠️ Refactor suggestion | 🟠 MajorRefactor
ReplaceAtinto smaller steps.
ReplaceAtexceeds the 50-line limit and now has repeated cleanup/error branches. Split into helper functions (write temp binary, Windows backup, swap/rollback, cleanup) to satisfy the size rule and reduce branching complexity.As per coding guidelines, “Functions must be less than 50 lines; files must be less than 800 lines.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/selfupdate/updater.go` around lines 224 - 292, ReplaceAt is too large and has repeated cleanup branches; refactor it by extracting smaller helpers: create writeTempBinary(binaryData, dir) that creates temp file, writes data, chmods, syncs and returns tmpPath (ensuring close/remove on error); prepareWindowsBackup(execPath, dir) that on windows creates oldPath by moving execPath to a temp backup and returns oldPath (or "" on non-windows) and handles cleanup on errors; swapBinaryWithRollback(tmpPath, execPath, oldPath) that renames tmpPath to execPath and on failure tries to restore oldPath back to execPath; and cleanupOldBinary(oldPath) that best-effort removes the backup; then make ReplaceAt call these helpers in sequence and propagate errors, preserving existing error messages and best-effort removals.cli/internal/diagnostics/collect.go (1)
40-101: 🛠️ Refactor suggestion | 🟠 MajorSplit
Collectinto smaller helpers.
Collectis over the 50-line cap. Extract health collection, docker collection, and config redaction into helper functions to meet the limit and improve maintainability.As per coding guidelines, “Functions must be less than 50 lines; files must be less than 800 lines.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/diagnostics/collect.go` around lines 40 - 101, The Collect function exceeds the 50-line limit—extract the Docker detection/compose output block into a helper (e.g., collectDockerInfo(ctx, state) returning DockerVersion, ComposeVersion, ContainerPS, RecentLogs and any errors), extract the health-check block into a helper (e.g., collectHealth(ctx, backendPort) returning HealthStatus, HealthBody and any errors), and extract the config redaction/marshal into a helper (e.g., redactConfig(state) returning ConfigRedacted string); then simplify Collect to call collectDockerInfo, collectHealth, redactConfig, set their returned fields on the Report, and append any returned errors to r.Errors so behavior stays identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/cmd/status.go`:
- Line 116: The defer currently swallows resp.Body.Close() errors; change the
anonymous defer to check the error and write a visible warning to the command
output (use the existing out writer). Replace defer func() { _ =
resp.Body.Close() }() with: defer func() { if err := resp.Body.Close(); err !=
nil { fmt.Fprintln(out, "warning: failed to close response body:", err) } }() so
the cleanup failure in the status command (where resp is used) is not silently
ignored.
In `@cli/internal/diagnostics/collect.go`:
- Line 79: The defer that currently swallows response close errors in Collect
must record any Close failure into the diagnostics.Errors; replace the anonymous
defer that does "_ = resp.Body.Close()" with a defer that calls
resp.Body.Close(), captures its error, and if non-nil appends a descriptive
error (including the returned error) to r.Errors (the diagnostics collector used
in Collect) so cleanup failures are recorded rather than ignored.
In `@cli/internal/health/check.go`:
- Line 67: The defer that currently discards resp.Body.Close() in checkOnce
should capture and propagate any close error when no earlier error exists:
replace the anonymous discard defer with a deferred function that stores the
close error (from resp.Body.Close()) into a named error return or checks the
current err variable and sets/returns the close error if err == nil; update
function signature of checkOnce to use a named error return if needed and ensure
the close error is returned rather than ignored.
In `@cli/internal/selfupdate/updater_test.go`:
- Line 394: Replace the silent os.Remove calls that discard errors (e.g., the
remove of oldPath and the other remove around lines referencing newPath) with
explicit error handling: call err := os.Remove(oldPath) and if err != nil &&
!errors.Is(err, os.ErrNotExist) { t.Fatalf("failed to remove %s: %v", oldPath,
err) } (and similarly for newPath); import "errors" if missing and keep behavior
tolerant only for os.ErrNotExist.
In `@cli/internal/selfupdate/updater.go`:
- Around line 239-240: The code currently discards errors from tmpFile.Close()
and os.Remove(tmpPath) in ReplaceAt and related branches (calls like _ =
tmpFile.Close(), _ = os.Remove(tmpPath)), masking filesystem failures and
rollback cleanup errors; update ReplaceAt (and the other error branches at the
noted locations) to capture and handle these errors explicitly by checking the
returned error values and combining them with the primary error using
errors.Join (or wrapping) so cleanup failures are reported alongside the
original error, and ensure rollback logic also surfaces any errors from
Close/Remove instead of silently swallowing them.
- Line 106: The deferred Close() calls currently swallow errors (e.g., defer
func() { _ = resp.Body.Close() }() at resp.Body usage and similar defers around
zip/reader closes); change each deferred anonymous func to capture Close()'s
error and merge or return it instead of discarding—e.g., use a named error
variable in the surrounding function (or capture the function's returned error)
and in the defer check closeErr := resp.Body.Close(); if err == nil { err =
closeErr } else if closeErr != nil { err = fmt.Errorf("%w; close error: %v",
err, closeErr) } (or log/propagate consistently). Update the defer blocks around
resp.Body.Close(), zip entry/reader closes (the defer at lines referenced) to
follow this pattern so I/O cleanup failures are surfaced.
---
Outside diff comments:
In `@cli/cmd/doctor.go`:
- Around line 27-84: runDoctor is over the 50-line limit because the GitHub
issue URL construction is inline; extract that logic into a new helper (e.g.,
buildGitHubIssueURL or createIssueURL) that accepts the diagnostics report (and
any needed version.RepoURL constant) and returns the final issue URL string.
Move the creation of issueTitle, issueBody, the QueryEscape/truncation logic,
and the final fmt.Sprintf into that helper, leaving runDoctor to call
buildGitHubIssueURL(report) and print the returned URL; update imports/usages as
needed and keep function behavior unchanged.
In `@cli/cmd/init.go`:
- Around line 46-52: Remove the duplicated comment above the config overwrite
check: delete one of the repeated "// Warn if re-initializing over existing
config (JWT secret will change)." lines found immediately before the if existing
:= config.StatePath(state.DataDir); fileExists(existing) { block so only a
single comment remains; the rest of the logic (isInteractive() check at function
entry, the if block using config.StatePath, fileExists, and fmt.Fprintf to
cmd.ErrOrStderr()) should be left unchanged.
In `@cli/internal/diagnostics/collect.go`:
- Around line 40-101: The Collect function exceeds the 50-line limit—extract the
Docker detection/compose output block into a helper (e.g.,
collectDockerInfo(ctx, state) returning DockerVersion, ComposeVersion,
ContainerPS, RecentLogs and any errors), extract the health-check block into a
helper (e.g., collectHealth(ctx, backendPort) returning HealthStatus, HealthBody
and any errors), and extract the config redaction/marshal into a helper (e.g.,
redactConfig(state) returning ConfigRedacted string); then simplify Collect to
call collectDockerInfo, collectHealth, redactConfig, set their returned fields
on the Report, and append any returned errors to r.Errors so behavior stays
identical.
In `@cli/internal/selfupdate/updater.go`:
- Around line 224-292: ReplaceAt is too large and has repeated cleanup branches;
refactor it by extracting smaller helpers: create writeTempBinary(binaryData,
dir) that creates temp file, writes data, chmods, syncs and returns tmpPath
(ensuring close/remove on error); prepareWindowsBackup(execPath, dir) that on
windows creates oldPath by moving execPath to a temp backup and returns oldPath
(or "" on non-windows) and handles cleanup on errors;
swapBinaryWithRollback(tmpPath, execPath, oldPath) that renames tmpPath to
execPath and on failure tries to restore oldPath back to execPath; and
cleanupOldBinary(oldPath) that best-effort removes the backup; then make
ReplaceAt call these helpers in sequence and propagate errors, preserving
existing error messages and best-effort removals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d86b2cab-3d84-4e98-9fb4-1be2e99a8270
📒 Files selected for processing (9)
cli/cmd/doctor.gocli/cmd/init.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/internal/diagnostics/collect.gocli/internal/health/check.gocli/internal/selfupdate/updater.gocli/internal/selfupdate/updater_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Build Web
- GitHub Check: Build Backend
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{py,ts,tsx,js,jsx,vue,go}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{py,ts,tsx,js,jsx,vue,go}: Functions must be less than 50 lines; files must be less than 800 lines
Handle errors explicitly; never silently swallow exceptions
Validate at system boundaries: user input, external APIs, config files
Files:
cli/internal/diagnostics/collect.gocli/internal/selfupdate/updater.gocli/cmd/start.gocli/cmd/status.gocli/internal/health/check.gocli/cmd/root.gocli/cmd/doctor.gocli/internal/selfupdate/updater_test.gocli/cmd/init.go
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go 1.26+ required for CLI development
Files:
cli/internal/diagnostics/collect.gocli/internal/selfupdate/updater.gocli/cmd/start.gocli/cmd/status.gocli/internal/health/check.gocli/cmd/root.gocli/cmd/doctor.gocli/internal/selfupdate/updater_test.gocli/cmd/init.go
🧠 Learnings (2)
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: CLI (Go binary) must be built with `cd cli && go build -o synthorg ./main.go`; working directory must be the module root (exception to 'never use cd' rule)
Applied to files:
cli/cmd/start.gocli/cmd/init.go
📚 Learning: 2026-03-14T16:17:59.179Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T16:17:59.179Z
Learning: Applies to cli/**/*.go : Go 1.26+ required for CLI development
Applied to files:
cli/cmd/start.gocli/cmd/doctor.gocli/cmd/init.go
🧬 Code graph analysis (3)
cli/cmd/start.go (2)
cli/internal/docker/client.go (1)
CheckMinVersions(69-78)cli/internal/health/check.go (1)
WaitForHealthy(22-52)
cli/cmd/doctor.go (2)
cli/internal/diagnostics/collect.go (1)
Collect(40-101)cli/internal/config/paths.go (1)
DataDir(17-23)
cli/cmd/init.go (2)
cli/internal/config/paths.go (1)
DataDir(17-23)cli/internal/config/state.go (1)
StatePath(37-39)
🔇 Additional comments (4)
cli/cmd/init.go (1)
70-74: LGTM!The explicit blank identifier assignments (
_, _ =) appropriately satisfy errcheck while acknowledging that write errors to stdout are non-recoverable in a CLI context.cli/cmd/root.go (1)
54-59: LGTM!Appropriate use of blank identifier to satisfy errcheck for error output to stderr.
cli/cmd/doctor.go (1)
37-50: LGTM on errcheck changes.The blank identifier pattern correctly suppresses errcheck warnings for diagnostic output operations.
cli/cmd/start.go (1)
46-76: LGTM!All errcheck suppressions are appropriate for CLI stdout/stderr operations. The function stays within the 50-line limit.
| return | ||
| } | ||
| defer resp.Body.Close() | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
Don’t silently swallow close errors in health output path.
Line 116 ignores resp.Body.Close() failures. Emit an explicit warning to out so cleanup errors are visible.
Proposed fix
- defer func() { _ = resp.Body.Close() }()
+ defer func() {
+ if cerr := resp.Body.Close(); cerr != nil {
+ fmt.Fprintf(out, " Backend: warning closing response body (%v)\n", cerr)
+ }
+ }()As per coding guidelines, “Handle errors explicitly; never silently swallow exceptions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| defer func() { _ = resp.Body.Close() }() | |
| defer func() { | |
| if cerr := resp.Body.Close(); cerr != nil { | |
| fmt.Fprintf(out, " Backend: warning closing response body (%v)\n", cerr) | |
| } | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/cmd/status.go` at line 116, The defer currently swallows
resp.Body.Close() errors; change the anonymous defer to check the error and
write a visible warning to the command output (use the existing out writer).
Replace defer func() { _ = resp.Body.Close() }() with: defer func() { if err :=
resp.Body.Close(); err != nil { fmt.Fprintln(out, "warning: failed to close
response body:", err) } }() so the cleanup failure in the status command (where
resp is used) is not silently ignored.
| r.Errors = append(r.Errors, fmt.Sprintf("health: %v", err)) | ||
| } else { | ||
| defer resp.Body.Close() | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
Record response close failures in diagnostics instead of ignoring them.
Line 79 currently hides close errors. In Collect, append close failures to r.Errors to keep cleanup explicit.
Proposed fix
- defer func() { _ = resp.Body.Close() }()
+ defer func() {
+ if cerr := resp.Body.Close(); cerr != nil {
+ r.Errors = append(r.Errors, fmt.Sprintf("health close: %v", cerr))
+ }
+ }()As per coding guidelines, “Handle errors explicitly; never silently swallow exceptions.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| defer func() { _ = resp.Body.Close() }() | |
| defer func() { | |
| if cerr := resp.Body.Close(); cerr != nil { | |
| r.Errors = append(r.Errors, fmt.Sprintf("health close: %v", cerr)) | |
| } | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/diagnostics/collect.go` at line 79, The defer that currently
swallows response close errors in Collect must record any Close failure into the
diagnostics.Errors; replace the anonymous defer that does "_ =
resp.Body.Close()" with a defer that calls resp.Body.Close(), captures its
error, and if non-nil appends a descriptive error (including the returned error)
to r.Errors (the diagnostics collector used in Collect) so cleanup failures are
recorded rather than ignored.
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
Propagate resp.Body.Close() errors instead of discarding them.
Line 67 silently swallows the close error. Since checkOnce already returns error, capture and return close failures when no prior error exists.
Proposed fix
-func checkOnce(ctx context.Context, url string) error {
+func checkOnce(ctx context.Context, url string) (retErr error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return err
}
resp, err := healthClient.Do(req)
if err != nil {
return err
}
- defer func() { _ = resp.Body.Close() }()
+ defer func() {
+ if cerr := resp.Body.Close(); cerr != nil && retErr == nil {
+ retErr = fmt.Errorf("closing health response body: %w", cerr)
+ }
+ }()As per coding guidelines, “Handle errors explicitly; never silently swallow exceptions.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/health/check.go` at line 67, The defer that currently discards
resp.Body.Close() in checkOnce should capture and propagate any close error when
no earlier error exists: replace the anonymous discard defer with a deferred
function that stores the close error (from resp.Body.Close()) into a named error
return or checks the current err variable and sets/returns the close error if
err == nil; update function signature of checkOnce to use a named error return
if needed and ensure the close error is returned rather than ignored.
| if runtime.GOOS == "windows" { | ||
| oldPath := fakeBinary + ".old" | ||
| os.Remove(oldPath) | ||
| _ = os.Remove(oldPath) |
There was a problem hiding this comment.
Avoid silent os.Remove suppression in test cleanup.
Lines 394 and 401 ignore delete failures. Keep cleanup explicit and tolerate only os.ErrNotExist intentionally.
Proposed fix
+ if err := os.Remove(oldPath); err != nil && !os.IsNotExist(err) {
+ t.Fatalf("remove old path: %v", err)
+ }
...
- _ = os.Remove(oldPath)
+ if err := os.Remove(oldPath); err != nil && !os.IsNotExist(err) {
+ t.Fatalf("remove old path: %v", err)
+ }As per coding guidelines, “Handle errors explicitly; never silently swallow exceptions.”
Also applies to: 401-401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/selfupdate/updater_test.go` at line 394, Replace the silent
os.Remove calls that discard errors (e.g., the remove of oldPath and the other
remove around lines referencing newPath) with explicit error handling: call err
:= os.Remove(oldPath) and if err != nil && !errors.Is(err, os.ErrNotExist) {
t.Fatalf("failed to remove %s: %v", oldPath, err) } (and similarly for newPath);
import "errors" if missing and keep behavior tolerant only for os.ErrNotExist.
| return Release{}, fmt.Errorf("querying GitHub releases: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
Close errors are being silently dropped in update download/extract paths.
Lines 106, 320, and 400 suppress Close() errors. These should be surfaced (or merged) so I/O cleanup failures are explicit, especially during update fetch/extract.
Proposed fix pattern (example for zip entry close)
result, readErr := io.ReadAll(io.LimitReader(rc, maxArchiveEntryBytes))
- _ = rc.Close()
- return result, readErr
+ if closeErr := rc.Close(); closeErr != nil && readErr == nil {
+ return nil, fmt.Errorf("closing zip entry: %w", closeErr)
+ }
+ return result, readErrAs per coding guidelines, “Handle errors explicitly; never silently swallow exceptions.”
Also applies to: 320-320, 400-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/selfupdate/updater.go` at line 106, The deferred Close() calls
currently swallow errors (e.g., defer func() { _ = resp.Body.Close() }() at
resp.Body usage and similar defers around zip/reader closes); change each
deferred anonymous func to capture Close()'s error and merge or return it
instead of discarding—e.g., use a named error variable in the surrounding
function (or capture the function's returned error) and in the defer check
closeErr := resp.Body.Close(); if err == nil { err = closeErr } else if closeErr
!= nil { err = fmt.Errorf("%w; close error: %v", err, closeErr) } (or
log/propagate consistently). Update the defer blocks around resp.Body.Close(),
zip entry/reader closes (the defer at lines referenced) to follow this pattern
so I/O cleanup failures are surfaced.
Comment on lines
+239
to
+240
| _ = tmpFile.Close() | ||
| _ = os.Remove(tmpPath) |
There was a problem hiding this comment.
ReplaceAt cleanup failures are masked across multiple error branches.
These branches swallow Close/Remove/rollback cleanup errors, which can hide filesystem problems and leak temp artifacts. Handle cleanup errors explicitly (e.g., aggregate with errors.Join).
As per coding guidelines, “Handle errors explicitly; never silently swallow exceptions.”
Also applies to: 244-245, 249-250, 254-255, 264-265, 268-269, 272-273, 282-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/internal/selfupdate/updater.go` around lines 239 - 240, The code
currently discards errors from tmpFile.Close() and os.Remove(tmpPath) in
ReplaceAt and related branches (calls like _ = tmpFile.Close(), _ =
os.Remove(tmpPath)), masking filesystem failures and rollback cleanup errors;
update ReplaceAt (and the other error branches at the noted locations) to
capture and handle these errors explicitly by checking the returned error values
and combining them with the primary error using errors.Join (or wrapping) so
cleanup failures are reported alongside the original error, and ensure rollback
logic also surfaces any errors from Close/Remove instead of silently swallowing
them.
Scheduler: - Revert to sleep-first periodic loop (prevents duplicate meetings on restart/deploy) - Emit meeting.started WS event before run_meeting (fixes dead event) - Wire event publisher from channels_plugin in create_app - Re-raise MemoryError/RecursionError in _resolve_participants and _run_and_publish (fixes inverted guard) - Ignore CancelledError in stop() results (normal shutdown) - Remove duplicate MEETING_EVENT_TRIGGERED log from controller - MappingProxyType for _FREQUENCY_SECONDS immutability Participant resolver: - Strip whitespace in list/tuple context values - Split _resolve_entry into _resolve_from_context + _resolve_from_registry - Move NoParticipantsResolvedError import to module level in test Go CLI: - Fix all golangci-lint v2 violations (errcheck, gofmt, staticcheck) - Format all Go files with gofmt Frontend: - Remove optional chaining on required MeetingMinutes arrays
…tend) Scheduler: - Revert to sleep-first periodic loop (prevents duplicate meetings) - Emit meeting.started WS event before run_meeting (fixes dead event) - Wire event publisher from channels_plugin in create_app - Re-raise MemoryError/RecursionError in _resolve_participants/_run_and_publish - Ignore CancelledError in stop() results (normal shutdown) - Remove duplicate MEETING_EVENT_TRIGGERED log from controller Participant resolver: - Strip whitespace in list/tuple context values - Split _resolve_entry into _resolve_from_context + _resolve_from_registry - Move NoParticipantsResolvedError import to module level in test Other: - MappingProxyType for _FREQUENCY_SECONDS - Remove optional chaining on required MeetingMinutes arrays
Comment on lines
+423
to
+446
| def _publish_started_event(self, meeting_type_name: str) -> None: | ||
| """Publish a meeting.started WS event before execution begins. | ||
|
|
||
| Best-effort: publish errors are logged and swallowed. | ||
| """ | ||
| if self._event_publisher is None: | ||
| return | ||
| try: | ||
| self._event_publisher( | ||
| "meeting.started", | ||
| { | ||
| "meeting_type": meeting_type_name, | ||
| "status": "in_progress", | ||
| }, | ||
| ) | ||
| except MemoryError, RecursionError: | ||
| raise | ||
| except Exception: | ||
| logger.warning( | ||
| MEETING_SCHEDULER_ERROR, | ||
| meeting_type=meeting_type_name, | ||
| note="started event publish failed", | ||
| exc_info=True, | ||
| ) |
There was a problem hiding this comment.
meeting.started payload missing meeting_id — frontend handler is dead code
_publish_started_event publishes a payload of {"meeting_type": ..., "status": "in_progress"} — deliberately before the meeting runs, so no meeting_id exists yet.
However, in web/src/stores/meetings.ts the handleWsEvent handler for meeting.started does:
case 'meeting.started': {
const meetingId = payload.meeting_id // always undefined here
if (typeof meetingId === 'string' && meetingId) {
void _refreshMeeting(meetingId) // never reached
}
break
}Because meeting_id is absent from the payload, typeof undefined === 'string' is false, so _refreshMeeting is never called for meeting.started events. The entire case branch is dead code.
Options to fix:
- Keep the event but emit it after the orchestrator creates an ID (i.e. from within
run_meetingat the point ameeting_idis assigned) so the payload can include it. - Since a
meeting_idcannot exist before the meeting runs, remove themeeting_idcheck from themeeting.startedbranch and instead refresh the full list (fetchMeetings) on this event rather than a single record. - Drop
meeting.starteduntil ameeting_idcan be provided in the payload.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/communication/meeting/scheduler.py
Line: 423-446
Comment:
**`meeting.started` payload missing `meeting_id` — frontend handler is dead code**
`_publish_started_event` publishes a payload of `{"meeting_type": ..., "status": "in_progress"}` — deliberately before the meeting runs, so no `meeting_id` exists yet.
However, in `web/src/stores/meetings.ts` the `handleWsEvent` handler for `meeting.started` does:
```ts
case 'meeting.started': {
const meetingId = payload.meeting_id // always undefined here
if (typeof meetingId === 'string' && meetingId) {
void _refreshMeeting(meetingId) // never reached
}
break
}
```
Because `meeting_id` is absent from the payload, `typeof undefined === 'string'` is `false`, so `_refreshMeeting` is **never called** for `meeting.started` events. The entire case branch is dead code.
Options to fix:
1. Keep the event but emit it after the orchestrator creates an ID (i.e. from within `run_meeting` at the point a `meeting_id` is assigned) so the payload can include it.
2. Since a `meeting_id` cannot exist before the meeting runs, remove the `meeting_id` check from the `meeting.started` branch and instead refresh the full list (`fetchMeetings`) on this event rather than a single record.
3. Drop `meeting.started` until a `meeting_id` can be provided in the payload.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+170
to
+174
| scheduler = state.app_state.meeting_scheduler | ||
| records = await scheduler.trigger_event( | ||
| data.event_name, | ||
| context=data.context, | ||
| ) |
There was a problem hiding this comment.
HTTP request blocks until all triggered meetings complete
trigger_event calls orchestrator.run_meeting(...) for each matching meeting type inside an asyncio.TaskGroup, and the trigger_meeting endpoint awaits the full result before sending a response.
Since LLM-driven meetings can take minutes per meeting, and multiple meetings can match the same event, the HTTP connection will be held open for the sum of all execution times. In practice this means:
- HTTP clients (browsers, API callers, load balancers) will time out long before meetings finish.
- The caller receives no response at all, making it impossible to tell if the trigger succeeded.
The typical pattern for long-running triggers is to fire the tasks as background work and return immediately with an accepted status. For example, create the tasks with asyncio.create_task (storing them in _tasks so stop() can cancel them), return 202 Accepted with the meeting IDs that will be populated, and let clients poll GET /meetings or subscribe to the meetings WS channel for completion events.
# Instead of awaiting the full execution:
records = await scheduler.trigger_event(data.event_name, context=data.context)
return Response(content=..., status_code=200)
# Fire and return 202 immediately, letting WS events deliver the result:
asyncio.create_task(scheduler.trigger_event(data.event_name, context=data.context))
return Response(content=..., status_code=202)Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai_company/api/controllers/meetings.py
Line: 170-174
Comment:
**HTTP request blocks until all triggered meetings complete**
`trigger_event` calls `orchestrator.run_meeting(...)` for each matching meeting type inside an `asyncio.TaskGroup`, and the `trigger_meeting` endpoint `await`s the full result before sending a response.
Since LLM-driven meetings can take minutes per meeting, and multiple meetings can match the same event, the HTTP connection will be held open for the sum of all execution times. In practice this means:
- HTTP clients (browsers, API callers, load balancers) will time out long before meetings finish.
- The caller receives no response at all, making it impossible to tell if the trigger succeeded.
The typical pattern for long-running triggers is to fire the tasks as background work and return immediately with an accepted status. For example, create the tasks with `asyncio.create_task` (storing them in `_tasks` so `stop()` can cancel them), return 202 Accepted with the meeting IDs that will be populated, and let clients poll `GET /meetings` or subscribe to the `meetings` WS channel for completion events.
```python
# Instead of awaiting the full execution:
records = await scheduler.trigger_event(data.event_name, context=data.context)
return Response(content=..., status_code=200)
# Fire and return 202 immediately, letting WS events deliver the result:
asyncio.create_task(scheduler.trigger_event(data.event_name, context=data.context))
return Response(content=..., status_code=202)
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the missing “meeting scheduler + triggering” layer for the communication subsystem, exposes meeting records via REST + WebSocket events, and adds a full Meeting Logs UI in the dashboard; also includes Go CLI lint/CI fixes.
Changes:
- Added
MeetingSchedulerservice (periodic + event-triggered) with participant resolution and lifecycle wiring. - Added meetings REST endpoints (
/meetings,/meetings/{id},/meetings/trigger) + WS channel/events (meetings,meeting.started/completed/failed), and updated web UI/store to consume them. - Updated Go CLI linting configuration and resolved errcheck/bodyclose-related issues; tweaked CI shell for Go tests.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/views/MeetingLogsPage.vue | Replaced placeholder with interactive meetings table, details sidebar, status filter, and trigger dialog + WS wiring |
| web/src/styles/theme.ts | Expanded Status union + added meeting status color tokens |
| web/src/stores/meetings.ts | Added Pinia store for meeting list/detail, trigger API call, and WS-driven refresh |
| web/src/api/types.ts | Added meeting domain types + WS meetings channel and meeting event types |
| web/src/api/endpoints/meetings.ts | Added REST client for list/get/trigger meeting endpoints |
| web/src/tests/views/MeetingLogsPage.test.ts | Removed old “Coming Soon” view tests (placeholder no longer applies) |
| web/src/tests/views/MeetingLogsPage.store-integration.test.ts | Added minimal store presence/integration test |
| web/src/tests/stores/meetings.test.ts | Added unit tests for meeting store API + WS refresh behavior |
| tests/unit/communication/test_config.py | Updated meeting config tests for MeetingFrequency enum usage |
| tests/unit/communication/meeting/test_scheduler.py | Added scheduler unit tests (periodic start/stop, triggering, publishing) |
| tests/unit/communication/meeting/test_participant.py | Added participant resolver tests (context/registry resolution + dedup) |
| tests/unit/communication/meeting/test_frequency.py | Added MeetingFrequency + frequency_to_seconds tests |
| tests/unit/communication/meeting/test_config_frequency.py | Added tests for config coercion/validation with MeetingFrequency |
| tests/unit/communication/conftest.py | Updated fixture to use MeetingFrequency enum |
| tests/unit/api/test_channels.py | Added meetings channel constant to channel set tests |
| tests/unit/api/test_app.py | Updated lifecycle tests for new startup/shutdown signature and scheduler lifecycle |
| tests/unit/api/controllers/test_meetings.py | Added REST controller tests for list/get/trigger + request validation bounds |
| tests/integration/communication/test_meeting_integration.py | Updated integration test to use MeetingFrequency enum |
| src/ai_company/observability/events/meeting.py | Added scheduler + API-level meeting event constants |
| src/ai_company/communication/meeting/scheduler.py | Implemented meeting scheduler (periodic + event-triggered) and WS publish hooks |
| src/ai_company/communication/meeting/participant.py | Added ParticipantResolver protocol + RegistryParticipantResolver implementation |
| src/ai_company/communication/meeting/frequency.py | Added MeetingFrequency StrEnum and seconds conversion mapping |
| src/ai_company/communication/meeting/errors.py | Added scheduler-specific error classes |
| src/ai_company/communication/meeting/init.py | Exported new scheduler/frequency/participant APIs |
| src/ai_company/communication/config.py | Switched MeetingTypeConfig.frequency to `MeetingFrequency |
| src/ai_company/api/ws_models.py | Added meeting WS event types to backend enum |
| src/ai_company/api/state.py | Extended AppState with meeting orchestrator + scheduler service accessors |
| src/ai_company/api/controllers/meetings.py | Implemented meetings REST controller (list/get/trigger) + bounded trigger request model |
| src/ai_company/api/channels.py | Added CHANNEL_MEETINGS and included it in ALL_CHANNELS |
| src/ai_company/api/app.py | Wired scheduler lifecycle into startup/shutdown and added meeting WS publisher integration |
| docs/design/communication.md | Documented scheduler design (frequency scheduling, triggers, participant resolution) |
| cli/internal/selfupdate/updater_test.go | Lint-related fixes (errcheck-style ignores; fmt.Fprintf usage) |
| cli/internal/selfupdate/updater.go | Lint fixes (defer body close wrapper; errcheck-style ignores; formatting) |
| cli/internal/health/check.go | Lint fix: ignore close error via deferred wrapper |
| cli/internal/diagnostics/collect.go | Lint fix: ignore close error via deferred wrapper |
| cli/cmd/version.go | Lint fix: ignore fmt.Fprintf return values |
| cli/cmd/update.go | Lint fix: ignore fmt.Fprint* return values |
| cli/cmd/uninstall.go | Lint fix: ignore fmt.Fprint* return values |
| cli/cmd/stop.go | Lint fix: ignore fmt.Fprintln return values |
| cli/cmd/status.go | Lint fix: ignore fmt.Fprint* return values; ignore close error wrapper |
| cli/cmd/start.go | Lint fix: ignore fmt.Fprint* return values |
| cli/cmd/root.go | Lint fix: ignore fmt.Fprintln return values |
| cli/cmd/init.go | Lint fix: ignore fmt.Fprintf return values |
| cli/cmd/doctor.go | Lint fix: ignore fmt.Fprint* return values |
| cli/.golangci.yml | Updated golangci-lint config for v2 (formatters section; removed gosimple) |
| CLAUDE.md | Updated repo structure/docs to reflect meeting scheduler + meetings store |
| .github/workflows/cli.yml | Set shell: bash for go test step for Windows compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+65
| onMounted(async () => { | ||
| try { | ||
| if (authStore.token && !wsStore.connected) { | ||
| wsStore.connect(authStore.token) | ||
| } | ||
| wsStore.subscribe(['meetings']) | ||
| wsStore.onChannelEvent('meetings', meetingStore.handleWsEvent) | ||
| } catch (err) { | ||
| console.error('WebSocket setup failed:', sanitizeForLog(err)) | ||
| } | ||
| try { | ||
| await meetingStore.fetchMeetings() | ||
| } catch (err) { | ||
| console.error('Initial data fetch failed:', sanitizeForLog(err)) | ||
| } | ||
| }) | ||
|
|
||
| onUnmounted(() => { | ||
| wsStore.unsubscribe(['meetings']) | ||
| wsStore.offChannelEvent('meetings', meetingStore.handleWsEvent) | ||
| }) |
Comment on lines
+431
to
+437
| self._event_publisher( | ||
| "meeting.started", | ||
| { | ||
| "meeting_type": meeting_type_name, | ||
| "status": "in_progress", | ||
| }, | ||
| ) |
Comment on lines
+483
to
+486
| # Wire meeting event publisher to the meetings WS channel. | ||
| if meeting_scheduler is not None and meeting_scheduler._event_publisher is None: # noqa: SLF001 | ||
| meeting_scheduler._event_publisher = _make_meeting_publisher( # noqa: SLF001 | ||
| channels_plugin, |
7 tasks
Aureliolo
added a commit
that referenced
this pull request
Mar 15, 2026
🤖 I have created a release *beep* *boop* --- ## [0.2.0](v0.1.4...v0.2.0) (2026-03-15) ##First probably usable release? Most likely not no and everything will break ### Features * add /get/ installation page for CLI installer ([#413](#413)) ([6a47e4a](6a47e4a)) * add cross-platform Go CLI for container lifecycle management ([#401](#401)) ([0353d9e](0353d9e)), closes [#392](#392) * add explicit ScanOutcome signal to OutputScanResult ([#394](#394)) ([be33414](be33414)), closes [#284](#284) * add meeting scheduler, event-triggered meetings, and Go CLI lint fixes ([#407](#407)) ([5550fa1](5550fa1)) * wire MultiAgentCoordinator into runtime ([#396](#396)) ([7a9e516](7a9e516)) ### Bug Fixes * CLA signatures branch + declutter repo root ([#409](#409)) ([cabe953](cabe953)) * correct Release Please branch name in release workflow ([#410](#410)) ([515d816](515d816)) * replace slsa-github-generator with attest-build-provenance, fix DAST ([#424](#424)) ([eeaadff](eeaadff)) * resolve CodeQL path-injection alerts in Go CLI ([#412](#412)) ([f41bf16](f41bf16)) ### Refactoring * rename package from ai_company to synthorg ([#422](#422)) ([df27c6e](df27c6e)), closes [#398](#398) ### Tests * add fuzz and property-based testing across all layers ([#421](#421)) ([115a742](115a742)) ### CI/CD * add SLSA L3 provenance for CLI binaries and container images ([#423](#423)) ([d3dc75d](d3dc75d)) * bump the major group with 4 updates ([#405](#405)) ([20c7a04](20c7a04)) ### Maintenance * bump github.com/spf13/cobra from 1.9.1 to 1.10.2 in /cli in the minor-and-patch group ([#402](#402)) ([e31edbb](e31edbb)) * narrow BSL Additional Use Grant and add CLA ([#408](#408)) ([5ab15bd](5ab15bd)), closes [#406](#406) --- 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: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Mar 15, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MeetingScheduler) — periodic scheduling via asyncio tasks + event-triggered meetings on demand, with lifecycle wiring into_safe_startup/_safe_shutdownMeetingFrequencyenum replacesNotBlankStr— compile-time validation, Pydantic coerces strings from YAMLParticipantResolverprotocol +RegistryParticipantResolver— resolves department names, agent names,"all", literal IDs to agent ID tuplesGET /meetings(paginated + filters),GET /meetings/{id},POST /meetings/triggerwith boundedTriggerMeetingRequestmeetingschannel,meeting.started/completed/failedevents, wired publisher fromchannels_pluginMeetingLogsPage(DataTable + detail sidebar + trigger dialog +canWriteguard), reactive sidebar via store, Pinia store with WS re-fetchCloses #264
Test plan
uv run pytest tests/unit/communication/meeting/ -n auto— scheduler, frequency, participant, config testsuv run pytest tests/unit/api/controllers/test_meetings.py -n auto— API endpoints + validationnpm --prefix web run test— store + WS handler + re-fetch testsuv run pytest tests/ -n auto --cov=ai_company --cov-fail-under=80— full suite + coverageuv run mypy src/ tests/— type checkingnpm --prefix web run type-check— frontend typescd cli && golangci-lint run— Go lint cleanReview coverage
Pre-reviewed by 16 local agents + 4 external reviewers (Copilot, Gemini, CodeRabbit, Greptile) across 5+ review rounds. 60+ findings addressed.