-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: native agent-teams & optional MCB integration #1726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
feat: native agent-teams & optional MCB integration #1726
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 52 files
Confidence score: 3/5
- Some risk due to two moderate issues that can affect runtime behavior and cleanup reliability, so this isn’t a no-brainer merge.
- In
src/tools/agent-teams/teammate-tools.ts, ifclearInboxthrows,resetOwnerTasksis skipped, potentially leaving tasks owned by a removed teammate. - In
src/features/compat-shims/hooks-config-adapter.ts, parsing valid JSON like "null" can cause a TypeError when accessingparsed.disabled_hooks/parsed.hookswithout guarding for non-object/null. - Pay close attention to
src/tools/agent-teams/teammate-tools.tsandsrc/features/compat-shims/hooks-config-adapter.ts- error handling around cleanup and JSON parsing assumptions.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/compat-shims/hooks-config-adapter.ts">
<violation number="1" location="src/features/compat-shims/hooks-config-adapter.ts:38">
P2: Valid JSON like "null" parses to null, but the code assumes an object and immediately accesses parsed.disabled_hooks/parsed.hooks, which will throw a TypeError at runtime. Add a guard for non-object/null parsed values before calling normalizeRawHooks.</violation>
</file>
<file name="src/tools/agent-teams/teammate-tools.ts">
<violation number="1" location="src/tools/agent-teams/teammate-tools.ts:53">
P2: resetOwnerTasks is skipped if clearInbox throws, which can leave tasks owned by a removed teammate. Wrap the inbox cleanup so task reset still runs even when clearInbox fails.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
I have read the CLA Document and I hereby sign the CLA |
1 similar comment
|
I have read the CLA Document and I hereby sign the CLA |
|
I've addressed the review comments from @cubic-dev-ai in commit 6e2a9b7:
Additionally, I've implemented the requested optional MCB layer (commit 1392f59):
|
|
Correction:
Additionally, I've implemented the requested optional MCB layer (commit 1392f59):
|
8acf006 to
192cbf4
Compare
|
recheck |
- McbAvailabilityStatus with per-tool tracking and TTL cache - withMcbFallback<T>() wrapper: try operation → degraded on failure - Auto-marks tools unavailable on failure, short-circuits repeat calls - 9 BDD tests (availability + graceful-wrapper)
- hooks-config-adapter: reject null/array/primitive JSON with warning - teammate-tools: wrap clearInbox in try/finally so resetOwnerTasks always runs
- Add MCB optional layer config schema (src/config/schema/mcb.ts) - Add config gate logic to disable MCB by default (src/features/mcb-integration/config-gate.ts) - Add 'mcb' config field to OhMyOpenCodeConfigSchema - Wire initialization in src/index.ts to lock MCB availability based on config - Add wisdom-capture hook (uses MCB fallback) and ambiguity-detector hook - Fix type errors in wisdom-capture tests
192cbf4 to
9c1471c
Compare
|
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 20 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".gitignore">
<violation number="1" location=".gitignore:42">
P2: The global ignore pattern `mcb.*` is overly broad and matches existing source files (e.g., `src/config/schema/mcb.ts`), which can cause new MCB-related files to be silently ignored by Git.</violation>
</file>
<file name="src/tools/agent-teams/send-message-tool.ts">
<violation number="1" location="src/tools/agent-teams/send-message-tool.ts:183">
P2: Plan approval responses are delivered to the inbox but never resume the recipient teammate, unlike other message types. This can leave agents waiting for approval without being woken to process the response.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…y hooks - Register oc-mcb builtin skill via SkillMcpManager (mcb serve stdio) - Add command, args, env, data_dir fields to McbConfigSchema (additive) - Create session-lifecycle handler: resetWarningState + fire-and-forget recovery - Wire handleMcbSessionCreated in event.ts on session.created - Add 20 integration tests (skill registration, config, availability, real binary) - Update docs to reflect stdio-first architecture and recovery mechanism
…ate on plan approval - Change mcb.* to /mcb.* in .gitignore to avoid ignoring source files like src/config/schema/mcb.ts - Add resumeTeammateWithMessage() call after plan_approved and plan_rejected paths in send-message-tool.ts - Fixes PR review comments code-yeongyu#3 and code-yeongyu#4 from cubic-dev-ai
|
All 4 review comments from cubic-dev-ai have been addressed:
All fixes verified: typecheck clean, 59 agent-teams tests passing, 0 failures. |
- add MCP stdio client helper and default argument builders for strict MCB schemas - add tiered E2E suites that exercise real mcb serve tool listing and core tool calls - align mcb integration types and exports with discovered v0.2.1-dev tool contracts
MCB Roundtrip Evidence (v0.2.1-dev)Pushed commit Write/Read operations verified in tests
Command evidence
Notes
|
…ams-mcb-integration
- Add test-mcb.toml: local-only providers (fastembed, edgevec, Moka, tokio) - mcb-client-helper: support --config path and env vars for StdioClientTransport - mcb-roundtrip-helpers: use all 11 valid MCB AgentType enum values - e2e-roundtrip-session: 3 tests with inline bun:sqlite DB verification - session create → agent_sessions table (FAILS: MCB persistence bug) - session get → cross-reference DB state (FAILS: no session ID returned) - memory store → observations table (PASSES: MCB persists observations) - e2e-roundtrip-index: 2 tests with inline bun:sqlite DB verification - index start → collections/file_hashes tables (FAILS: 0 rows persisted) - search → cross-reference file_hashes (FAILS: 0 rows persisted) - Remove all silent if(hasTable) guards: tests FAIL explicitly per constraint - All DB paths isolated via MCP__AUTH__USER_DB_PATH to /tmp/ with cleanup
…ct verification Memory test now verifies: MCB response contains observation_id, row exists with exact content='e2e-memory-test', observation_type='code', and project_id='test-project' — not just row count > 0.
…ssion_summary_id - tier2 memory: update error assertion (MCB now returns proper validation 'Project ID cannot be empty' instead of generic 'internal error') - tier2 validate: update to expect 'domain_crate' config error (MCB bug: list_rules should not need project config variables) - session create: add session_summary_id + model to data payload per MCB create.rs requirements (still fails: FK constraint on session_summaries) - tier2: 7/7 pass (was 5/7)
Summary
This PR introduces native agent-teams orchestration and an optional integration layer for MCB (Memory Context Bus).
Key Features
mcb.enabled: truein config.Additional Hooks
Fixes
hooks-config-adapter.ts.try/finallyblock inteammate-tools.tsto ensure task reset happens even if inbox cleanup fails.Verification