Batuhan/cleanup 8

[batuhan]

Summary by CodeRabbit

• New Features

  • Introduced simplified new command for creating direct model or agent chats

• Removed Features

  • Removed per-room customization options (system prompts, temperature, context limits)
  • Removed numerous configuration commands (model, temp, system-prompt, context, tokens, tools, fork, regenerate)
  • Removed builder room management and MCP server configuration

[coderabbitai]

📝 Walkthrough

Walkthrough

Switches per-room model/agent state to a ResolvedTarget-driven runtime model, removes persistent system prompts and response-prefix plumbing, prunes many command subsystems (builder/MCP/manage/legacy AI commands), and refactors PortalMetadata/UserLoginMetadata and tooling/streaming/heartbeat flows to use runtime overrides and model capability lookups.

Changes

Cohort / File(s)	Summary
Documentation README.md, docs/matrix-ai-matrix-spec-v1.md, docs/msc/com.beeper.mscXXXX-commands.md	Reworded README features; replaced custom Matrix state-event docs with standard capability advertising; adjusted MSC examples/commands to status/new-centered usage.
Core metadata & identifiers pkg/connector/metadata.go, pkg/connector/identifiers.go, pkg/connector/chat_fork_test.go, pkg/connector/target_test_helpers_test.go	Replaced many per-room fields with slimmer PortalMetadata and new ResolvedTarget type; added UserProfile; updated isSimpleMode and cloning logic; added test helpers for ResolvedTarget-based metas.
Model resolution & prompts pkg/connector/client.go, pkg/connector/model_fallback.go, pkg/connector/client_capabilities_test.go, pkg/connector/simple_mode_prompt.go	Collapsed layered room/agent/provider overrides into runtime resolution: effectiveModel uses RuntimeModelOverride/ResolvedTarget; introduced profilePromptSupplement; model fallback uses agent primary/fallbacks and model-capabilities helpers.
Chat/portal/agent flows pkg/connector/chat.go, pkg/connector/agentstore.go, pkg/connector/agent_activity.go, pkg/connector/agent_display.go	Removed SystemPrompt persistence and many per-field metadata mutations; portal initialization now uses portal.OtherUserID and ResolvedTarget; PortalInitOpts trimmed (SystemPrompt removed); agent resolution uses resolveAgentID/resolved targets.
Commands & CLI surface pkg/connector/commands.go, pkg/connector/commands_parity.go, pkg/connector/commands_simple.go, pkg/connector/commands_mcp.go, pkg/connector/commands_manage.go, pkg/connector/command_registry.go	Dramatically reduced command surface: removed many legacy AI commands and entire MCP/manage subsystems; introduced user-facing command filtering and consolidated "new" / "status" oriented command set.
Builder room removal pkg/connector/builder.go, pkg/connector/commands_manage.go	Deleted builder room lifecycle code and associated manage command/handlers.
Response prefix & templating removed pkg/connector/response_prefix.go, pkg/connector/response_prefix_template.go, pkg/connector/response_finalization.go, pkg/connector/integrations_config.go, pkg/connector/integrations_example-config.yaml	Removed response-prefix computation, templating, config fields, and application to outputs; updated example config to remove responsePrefix.
Events & connector registration pkg/connector/events.go, pkg/connector/connector.go	Removed custom RoomCapabilities/RoomSettings/ModelCapabilities event types and content structs; simplified connector event handler registrations.
Tools & streaming pkg/connector/streaming_*.go, pkg/connector/streaming_params.go, pkg/connector/streaming_tool_selection.go, pkg/connector/streaming_chat_completions.go, pkg/connector/streaming_continuation.go, pkg/connector/tool_policy.go, pkg/connector/tool_execution.go, pkg/connector/tool_descriptions.go	Replaced direct meta.Capabilities checks with getModelCapabilitiesForMeta(meta); removed builder-room exceptions for boss tools; tool inclusion gated by agent presence and model capabilities.
Heartbeat, scheduler & cron pkg/connector/heartbeat_*.go, pkg/connector/scheduler_*.go, pkg/connector/scheduler_rooms.go	Staleness/target checks now use resolveAgentID(meta)/ResolvedTarget; set portal.OtherUserID and RuntimeModelOverride for scheduled deliveries; dropped response-prefix usage.
Provisioning & APIs pkg/connector/provisioning.go, pkg/connector/provisioning_test.go	Added HTTP provisioning endpoints (profile, agents, MCP servers) with full CRUD, payload validation, and tests; introduced client helper getClient and normalization/validation utilities.
Helpers & new utilities pkg/connector/desktop_api_helpers.go, pkg/connector/mcp_helpers.go, pkg/connector/target_test_helpers_test.go	Added desktop API arg parser and MCP helper utilities; introduced MCP parsing/validation and test metadata factories.
Tests updated pkg/connector/*_test.go (many)	Updated numerous tests to use ResolvedTarget helpers and model caches; adjusted test initializers for new metadata shapes.
Removed internals & directives pkg/connector/response_prefix_template.go, pkg/connector/queue_directive.go, pkg/connector/inbound_directive_apply.go, pkg/connector/parse_utils.go, pkg/connector/queue_notice.go, pkg/connector/commands_helpers.go, pkg/connector/parse_utils.go, pkg/connector/trace.go	Deleted many internal helpers: queue directive parsing, inbound directive apply functions, parse helpers, trace-level logic, response-prefix templating, and several command helper utilities.
Misc behavior & config tweaks pkg/connector/integration_host.go, pkg/connector/integrations.go, pkg/connector/system_prompts.go, pkg/connector/simple_mode_prompt_test.go, pkg/connector/sessions_tools.go	Removed builder-room influence from internal-room checks and sessions; stripped meta-based system-prompt overrides; simplified intro/greeting emission and simple-mode prompt construction.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client
participant Connector
participant Resolver
participant ModelProvider
participant Matrix
Client->>Connector: create/open chat (new/status)
Connector->>Resolver: resolve target (RuntimeModelOverride or ResolvedTarget)
Resolver-->>Connector: ResolvedTarget (Kind, GhostID, ModelID)
Connector->>ModelProvider: get model capabilities & effectiveModel(ResolvedTarget)
ModelProvider-->>Connector: modelID + capabilities
Connector->>Matrix: create/open room, set portal.OtherUserID, save ResolvedTarget
Matrix-->>Connector: room created/opened

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

"A rabbit hops in code so deep,
I chewed the threads and sowed a heap.
Targets resolved, prompts set free,
New chats hum soft as tea.
🐇✨"

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch batuhan/cleanup-8

[coderabbitai]

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

pkg/connector/model_fallback.go (1)

32-53: ⚠️ Potential issue | 🟠 Major

Keep the already-resolved model first in the fallback chain.

In agent rooms this now ignores meta's resolved runtime override and starts from the agent's primary model instead. That can send a request to the wrong model whenever RuntimeModelOverride is set.

Suggested fix

 func (oc *AIClient) modelFallbackChain(ctx context.Context, meta *PortalMetadata) []string {
+	models := []string{oc.effectiveModel(meta)}
 	agentID := resolveAgentID(meta)
 	if agentID != "" {
 		store := NewAgentStoreAdapter(oc)
 		agent, err := store.GetAgentByID(ctx, agentID)
 		if err == nil && agent != nil {
-			var models []string
 			if strings.TrimSpace(agent.Model.Primary) != "" {
 				models = append(models, ResolveAlias(agent.Model.Primary))
 			}
 			for _, fb := range agent.Model.Fallbacks {
 				if strings.TrimSpace(fb) == "" {
 					continue
 				}
 				models = append(models, ResolveAlias(fb))
 			}
 			return dedupeModels(models)
 		}
 	}
 
-	// No agent fallbacks - use the effective model only.
-	return dedupeModels([]string{oc.effectiveModel(meta)})
+	return dedupeModels(models)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/model_fallback.go` around lines 32 - 53, The modelFallbackChain
currently builds fallbacks starting from the agent's primary model which ignores
any already-resolved runtime override in meta; change modelFallbackChain so it
first inserts the resolved model from oc.effectiveModel(meta) (if non-empty)
into the models list, then append the agent's primary and its fallbacks (using
ResolveAlias and trimming empties) to preserve the runtime override as the first
candidate; keep using NewAgentStoreAdapter(oc).GetAgentByID(ctx, agentID) to
fetch agent info and return dedupeModels(models) at the end to remove
duplicates.

pkg/connector/chat.go (1)

69-89: ⚠️ Potential issue | 🟡 Minor

Remove or suppress unused function validateDMModelSwitch.

The function is not called anywhere in the codebase. Either remove it or add a //nolint:unused directive with a comment if it's intended for future use.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/chat.go` around lines 69 - 89, The function
validateDMModelSwitch is unused; either delete it or explicitly mark it as
intentionally unused by adding a nolint with a comment. If removing, delete the
whole validateDMModelSwitch function (references: validateDMModelSwitch,
ErrDMGhostImmutable, effectiveModel, modelUserID, resolveAgentID) to avoid dead
code; otherwise add a comment like "// nolint:unused -- kept for future DM model
validation" above the function to suppress linter warnings and keep the existing
logic.

🧹 Nitpick comments (12)

pkg/connector/trace.go (1)

3-15: Make the trace deprecation explicit instead of keeping dynamic-looking stubs.

These helpers now ignore *PortalMetadata and hard-disable tracing globally, but their signatures still read like metadata-driven behavior. That makes future call sites easy to misread. If tracing is intentionally gone, prefer deleting these wrappers and inlining the constants at the callers, or add a short deprecation comment here so the removal is explicit.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/trace.go` around lines 3 - 15, Remove the misleading
metadata-driven stubs by deleting the functions traceLevel, traceEnabled, and
traceFull from pkg/connector/trace.go and replace their call sites to use the
explicit constants they currently return ("off", false, false); update any
callers that pass *PortalMetadata to instead use the literal values and remove
unused meta variables. If inlining across many call sites is undesirable,
alternatively add a clear deprecation comment above these functions stating they
are deprecated and always return the constants, but prefer deletion and inlining
to avoid future confusion.

pkg/connector/integrations_example-config.yaml (1)

95-96: Document the supported replyToMode values inline.

This example file doubles as user-facing docs, but unlike the other enum-like settings above, replyToMode does not say which values are valid or what "first" means. Adding that here would make the migration away from responsePrefix much easier to follow.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/integrations_example-config.yaml` around lines 95 - 96, The
example config's replyToMode setting lacks inline docs; update the replyToMode
entry to list supported values (e.g., "first", "all", "none") and a short
description of each and what "first" does (e.g., "first" replies only to the
first message in a thread), and mention its relation/replacement for legacy
responsePrefix so users can migrate; locate the replyToMode YAML key and add a
one-line comment or brief block comment above it documenting the valid enum
values and their behavior.

pkg/connector/sessions_tools.go (1)

488-504: Consider checking context cancellation in the polling loop.

The polling loop doesn't check ctx.Done() between iterations. If the context is cancelled (e.g., client disconnect), the loop continues until the timeout expires. This could delay cleanup in cancellation scenarios.

♻️ Proposed fix to handle context cancellation

 	timeout := time.Duration(timeoutSeconds) * time.Second
 	deadline := time.Now().Add(timeout)
 	for time.Now().Before(deadline) {
+		select {
+		case <-ctx.Done():
+			result["status"] = "cancelled"
+			result["error"] = "context cancelled"
+			return tools.JSONResult(result), nil
+		default:
+		}
 		assistantMsg, found := oc.waitForNewAssistantMessage(ctx, targetPortal, lastAssistantID, lastAssistantTimestamp)
 		if found {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/sessions_tools.go` around lines 488 - 504, The polling loop in
the block using waitForNewAssistantMessage does not check ctx.Done(), so
cancellation can be ignored; modify the loop to select on ctx.Done() as well as
a sleep/tick or use time.After instead of time.Sleep so the loop exits
immediately when the context is cancelled. Specifically, update the loop that
references timeout, deadline, waitForNewAssistantMessage, lastAssistantID and
lastAssistantTimestamp to check ctx.Done() (e.g., select { case <-ctx.Done():
return nil, ctx.Err() or set result status to indicate cancellation; case
<-time.After(250*time.Millisecond): /* continue polling */ }) so the function
returns promptly on context cancellation.

pkg/connector/desktop_api_helpers.go (1)

8-35: Please lock this parser down with table-driven tests.

The 2-arg URL heuristic and whitespace-collapsing rules are subtle enough that a small case matrix ([], whitespace-only, token-only, token+url, name+token, name+token+url) would prevent accidental behavior changes later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/desktop_api_helpers.go` around lines 8 - 35, Add table-driven
tests for parseDesktopAPIAddArgs that exercise the heuristic and
whitespace-collapsing behavior: create a slice of test cases covering empty
args, whitespace-only, token-only, token+url (2 args where second is URL),
name+token (2 args where first is name), name+token+url (3+ args where remaining
parts join to form URL), and variations with extra internal/leading/trailing
whitespace; for each case call parseDesktopAPIAddArgs and assert the returned
name, token, baseURL and error match expected values. Include negative checks
(expecting an error) for missing args and positive checks that rely on
isLikelyHTTPURL and normalizeDesktopInstanceName behavior; structure tests in a
table-driven loop and give each case a descriptive name so future regressions
are obvious.

pkg/connector/scheduler_rooms.go (2)

31-34: Duplicate normalizeAgentID calls can be consolidated.

normalizeAgentID(record.Job.AgentID) is computed twice (Line 15 and Line 31). Consider storing the result in a local variable to avoid redundant work.

♻️ Proposed optimization

 func (s *schedulerRuntime) ensureCronRoomLocked(ctx context.Context, record *scheduledCronJob) error {
 	if record == nil {
 		return nil
 	}
-	portalID := fmt.Sprintf("cron:%s:%s", normalizeAgentID(record.Job.AgentID), strings.TrimSpace(record.Job.ID))
+	normalizedAgentID := normalizeAgentID(record.Job.AgentID)
+	portalID := fmt.Sprintf("cron:%s:%s", normalizedAgentID, strings.TrimSpace(record.Job.ID))
 	portal, err := s.getOrCreateScheduledPortal(ctx, portalID, fmt.Sprintf("Cron: %s", strings.TrimSpace(record.Job.Name)), func(meta *PortalMetadata) {
 		// ... unchanged ...
 	})
 	if err != nil {
 		return err
 	}
-	portal.OtherUserID = agentUserID(normalizeAgentID(record.Job.AgentID))
+	portal.OtherUserID = agentUserID(normalizedAgentID)
 	if err := portal.Save(ctx); err != nil {
 		return err
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/scheduler_rooms.go` around lines 31 - 34, The code calls
normalizeAgentID(record.Job.AgentID) twice; compute it once into a local
variable (e.g., normalizedAgentID) and reuse it for both the earlier assignment
and when setting portal.OtherUserID (and/or passing into agentUserID) to avoid
duplicate work — update the block that assigns portal.OtherUserID and where
normalizeAgentID is used so portal.Save(ctx) remains unchanged.

---

59-62: Same optimization opportunity as above.

normalizeAgentID(state.AgentID) is computed at both Line 43 and Line 59. Apply the same consolidation pattern here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/scheduler_rooms.go` around lines 59 - 62, Compute
normalizeAgentID(state.AgentID) once and reuse it instead of calling it twice;
assign its result to a local variable (e.g., normalizedAgentID) and pass that
into agentUserID(...) when setting portal.OtherUserID and anywhere else in this
function that also uses normalizeAgentID(state.AgentID). Update the code around
the portal.OtherUserID assignment and any earlier usage to reference the new
local variable, then call portal.Save(ctx) as before.

pkg/connector/simple_mode_prompt_test.go (1)

119-119: Consider consistent multi-line formatting for readability.

Lines 119 and 146 use single-line struct initialization while earlier tests use multi-line. Consider consistent formatting for maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/simple_mode_prompt_test.go` at line 119, The test uses
single-line struct initialization for PortalMetadata and ResolvedTarget (meta :=
&PortalMetadata{ResolvedTarget: &ResolvedTarget{Kind: ResolvedTargetModel,
GhostID: modelUserID("openai/gpt-5.2"), ModelID: "openai/gpt-5.2"}}) while other
tests use multi-line formatting; reformat this initialization into the same
multi-line style used elsewhere (expand &PortalMetadata, &ResolvedTarget fields
across multiple lines) for consistency and readability, touching the meta
variable and the nested ResolvedTarget fields (Kind, GhostID, ModelID).

pkg/connector/tools.go (1)

1571-1597: Trailing empty string argument in format string is unnecessary.

The format string on lines 1576-1597 ends with %s on line 1597, matched by an empty string "". This appears to be a leftover from removing modelChanged. Consider removing both the trailing %s and the empty string argument to clean up the code.

♻️ Proposed cleanup

 	status := fmt.Sprintf(`Session Status
 ==============
 Time: %s %s (%s)
 Day: %s
 
 Model: %s
 Provider: %s
 Max Context: %d messages
 Max Tokens: %d
 
-	Session: %s
-Chat: %s%s%s`,
+Session: %s
+Chat: %s%s`,
 		timeStr, timezone, now.Format("MST"),
 		dayOfWeek,
 		modelName,
 		provider,
 		maxContext,
 		maxTokens,
 		sessionID,
 		title,
 		agentInfo,
-		"",
 	)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/tools.go` around lines 1571 - 1597, The fmt.Sprintf call that
builds status contains a trailing "%s" in the format string matched by the empty
string argument "" (leftover from removed modelChanged); locate the fmt.Sprintf
in pkg/connector/tools.go that sets status (the block using resolveAgentID,
agentInfo, timeStr, modelName, provider, maxContext, maxTokens, sessionID,
title) and remove the trailing "%s" from the format template and the
corresponding "" argument so the format string and argument list match.

pkg/connector/provisioning.go (2)

281-318: Minor redundancy in model validation.

The function validates models twice for fallbacks:

1. Lines 286-302: Adds fallbacks to models slice and validates them
2. Lines 303-316: Re-validates and re-resolves fallbacks separately

This is inefficient but correct. Consider consolidating to validate once.

♻️ Suggested simplification

 func validateAgentModels(ctx context.Context, client *AIClient, agent *agents.AgentDefinition) error {
 	if agent == nil || client == nil {
 		return nil
 	}
-	models := []string{}
-	if strings.TrimSpace(agent.Model.Primary) != "" {
-		models = append(models, strings.TrimSpace(agent.Model.Primary))
-	}
-	models = append(models, normalizeStringList(agent.Model.Fallbacks)...)
-	for _, model := range models {
-		resolved, valid, err := client.resolveModelID(ctx, model)
+	if primary := strings.TrimSpace(agent.Model.Primary); primary != "" {
+		resolved, valid, err := client.resolveModelID(ctx, primary)
 		if err != nil {
 			return err
 		}
 		if !valid || resolved == "" {
-			return fmt.Errorf("invalid model: %s", model)
-		}
-		if model == agent.Model.Primary {
-			agent.Model.Primary = resolved
-			continue
+			return fmt.Errorf("invalid model: %s", primary)
 		}
+		agent.Model.Primary = resolved
 	}
 	if len(agent.Model.Fallbacks) > 0 {
 		resolvedFallbacks := make([]string, 0, len(agent.Model.Fallbacks))
 		for _, fallback := range normalizeStringList(agent.Model.Fallbacks) {
 			resolved, valid, err := client.resolveModelID(ctx, fallback)
 			if err != nil {
 				return err
 			}
 			if !valid || resolved == "" {
 				return fmt.Errorf("invalid model: %s", fallback)
 			}
 			resolvedFallbacks = append(resolvedFallbacks, resolved)
 		}
 		agent.Model.Fallbacks = resolvedFallbacks
 	}
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/provisioning.go` around lines 281 - 318, The
validateAgentModels function performs duplicate validation for fallbacks: first
by appending normalized fallbacks into models and validating them in the initial
loop, then re-validating and re-resolving them again when building
resolvedFallbacks; simplify by validating/resolving each model exactly once —
call client.resolveModelID(ctx, ...) for agent.Model.Primary and for each entry
in normalizeStringList(agent.Model.Fallbacks) (using the same resolution logic
as for primary), collect resolved fallbacks into resolvedFallbacks and set
agent.Model.Primary/resolvedFallbacks accordingly, and remove the first loop’s
pre-population of models for fallbacks so resolveModelID is invoked only once
per model.

---

662-703: Complex connect flow with multiple persistence points.

The connectMCPServer function has multiple login.Save calls (lines 680, 691, 698) with different state configurations. While this ensures state is persisted at each step, it makes the flow harder to reason about and could leave the server in intermediate states if saves fail.

Consider documenting the expected state transitions or consolidating saves where possible.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/provisioning.go` around lines 662 - 703, connectMCPServer
mutates and persists login state in many places (calls to setLoginMCPServer +
login.Save at different branches) which can leave intermediate state if a save
fails; refactor so you assemble the final cfg and persist once where possible:
keep using mcpServerNeedsToken/verifyMCPServerConnection but avoid calling
setLoginMCPServer/login.Save multiple times—instead update a local cfg and only
call setLoginMCPServer(loginMetadata(login), target.Name, cfg) followed by
login.Save(ctx) once after the final cfg is determined (or, if you must persist
an intermediate unauthenticated state, centralize that logic into a helper like
persistLoginMCPServer that wraps setLoginMCPServer + login.Save and is called in
explicit, documented places); also ensure client.invalidateMCPToolCache is
called consistently after persistence and propagate errors from login.Save
immediately.

docs/msc/com.beeper.mscXXXX-commands.md (1)

78-81: Consider documenting authorization for the new command.

The security considerations mention checking power levels for commands that modify room or session state. The new command creates new chats, which could be considered state modification. Consider clarifying whether new requires power-level checks.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/msc/com.beeper.mscXXXX-commands.md` around lines 78 - 81, Update the
Security Considerations to explicitly state the authorization policy for the
"new" command: specify whether the "new" command requires a minimum room power
level (or other ACL) before creating chats and, if so, the required power-level
threshold and the expected behavior when a sender lacks permission (reject with
an error message). If the command is allowed for all users, document that
explicitly and note any mitigations (rate limits, audit logging). Reference the
existing "Command authorization" bullet and the "new" command name so readers
can find the rule and the expected error/response behavior.

pkg/connector/client.go (1)

1547-1556: Consider simplifying redundant capability checks.

The function checks getModelCapabilitiesForMeta(meta).SupportsReasoning and then also looks up findModelInfo(modelID).SupportsReasoning as a fallback. Since getModelCapabilitiesForMeta likely already consults model info internally, this may be redundant. However, this belt-and-suspenders approach is safe and doesn't cause functional issues.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/client.go` around lines 1547 - 1556, The code redundantly
checks SupportsReasoning twice (via getModelCapabilitiesForMeta(meta) and again
by fetching model info from findModelInfo(effectiveModel(meta))). Simplify by
relying on getModelCapabilitiesForMeta(meta) only: remove the second branch that
trims effectiveModel(meta) and calls findModelInfo(...). Keep the initial check
using getModelCapabilitiesForMeta(meta).SupportsReasoning and return "low"
there, otherwise return "off" — ensure functions getModelCapabilitiesForMeta,
effectiveModel, and findModelInfo are left intact for future use.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/matrix-ai-matrix-spec-v1.md`:
- Around line 286-288: The "State Events" paragraph is currently
ambiguous—update the text around the <a id="state"></a> / "State Events" header
to explicitly state that only custom room-level editable configuration and
capability advertising was removed while persistent agent/member state like
"com.beeper.ai.agents" is still used; alternatively, if you prefer to remove
inconsistency, delete the residual "com.beeper.ai.agents" state-event sections
elsewhere in the doc so the spec consistently reflects that no custom state
remains. Locate the "State Events" header and the referenced
"com.beeper.ai.agents" sections and either narrow the sentence to mention "room
settings/capabilities only" and keep the agent state docs, or remove the agent
state sections entirely to maintain internal consistency.

In `@pkg/connector/client.go`:
- Around line 1246-1268: The effectiveModel function currently returns an empty
string when ResolvedTarget.Kind == ResolvedTargetAgent and either
store.GetAgentByID fails or agent.Model.Primary is empty; change this so that
instead of returning "", the code falls through to call
oc.defaultModelForProvider() (i.e., remove the early return in the
ResolvedTargetAgent branch and only return ResolveAlias(agent.Model.Primary)
when present), using the same NewAgentStoreAdapter/ store.GetAgentByID logic to
attempt agent lookup but delegating to oc.defaultModelForProvider() on any
failure or missing agent.Model.Primary.

In `@pkg/connector/handleai.go`:
- Around line 366-369: The welcome notice can be sent before
effectiveModel(meta) resolves, producing "You are chatting with ." — modify the
branch around resolveAgentID(meta) in scheduleWelcomeMessage to guard the
displayName: call oc.effectiveModel(meta) and modelContactName(...) as before
but if modelID or displayName is empty then either skip/skip sending
oc.sendSystemNotice(bgCtx, portal, ...) or use a safe fallback like "an AI"
(e.g., "You are chatting with an AI. AI can make mistakes."); ensure the change
targets the code that calls oc.sendSystemNotice so you don't send the blank-name
notice.
- Around line 225-230: autoGreetingBlockReason currently treats an empty
resolveAgentID(meta) as "no-agent", which blocks auto-greeting for rooms that
resolve to a model-only target; update autoGreetingBlockReason so it only
returns "no-agent" when there is truly no target (neither agent nor model).
Concretely, change the resolveAgentID(meta) == "" branch to consult the same
predicate used by scheduleAutoGreeting (the logic around lines 366-369 that
allows model-only rooms) — i.e., only return "no-agent" if resolveAgentID(meta)
== "" AND the room does not resolve to a model; otherwise allow auto-greeting to
proceed. Ensure the fix references autoGreetingBlockReason, resolveAgentID, and
the same model-resolution check used by scheduleAutoGreeting.

In `@pkg/connector/integrations_config.go`:
- Around line 164-171: ChannelDefaultsConfig and ChannelConfig had their
ResponsePrefix fields removed but upgradeConfig currently drops any existing
responsePrefix values silently; update the upgradeConfig function to detect
legacy "responsePrefix" entries (e.g., in the parsed YAML/JSON map or raw config
blob) and migrate them into the new config model or emit a clear
deprecation/warning so users aren't silently losing settings. Locate the
upgradeConfig function and add logic to look for "responsePrefix" for both
channel defaults and per-channel entries, then either copy the value to the
appropriate new location, store it in a preserved deprecated field, or log a
warning with the channel identifier; ensure the code references
ChannelDefaultsConfig and ChannelConfig when applying the migration so the
change is explicit and traceable.

In `@pkg/connector/metadata.go`:
- Around line 222-236: Remove the redundant shallow assignment
clone.ResolvedTarget = src.ResolvedTarget: keep the deep-copy block that creates
target := *src.ResolvedTarget and sets clone.ResolvedTarget = &target, and
delete the earlier line that assigns src.ResolvedTarget directly so
ResolvedTarget is only set via the explicit copy; ensure that only the deep-copy
branch touching src.ResolvedTarget/clone.ResolvedTarget remains.

In `@pkg/connector/provisioning.go`:
- Around line 393-396: The current check-then-create using
GetAgentByID(agent.ID) is vulnerable to TOCTOU races; remove the non-atomic
existence pre-check and instead enforce uniqueness at persist time (DB unique
constraint or transactional/locked insert) and handle duplicate-key errors
returned by SaveAgent (or the store method that persists agents) by returning a
409/appropriate error via mautrix.MInvalidParam.WithMessage(...).Write(w).
Alternatively, wrap creation in a DB transaction or use an atomic upsert API
from the store and map its duplicate-key error to the existing error response;
refer to GetAgentByID, agent.ID and the SaveAgent/store create method when
making the change.
- Around line 612-616: The code in the resolveNamedMCPServer call uses a fragile
string comparison on err.Error(); change this to detect the “not found”
condition via a sentinel error or specific error type instead: introduce or use
a sentinel like ErrMCPServerNotFound returned by resolveNamedMCPServer and
replace `err.Error() != "not found"` with `errors.Is(err, ErrMCPServerNotFound)`
(importing the errors package), and keep the mautrix.MInvalidParam response only
for non-not-found errors (i.e., if err != nil && !errors.Is(err,
ErrMCPServerNotFound) { ... }).

In `@pkg/connector/remote_message.go`:
- Around line 83-87: The code removed the fallback that populated model
metadata, leaving ConvertAIResponse/ConvertMessage with empty
AIResponseParams.Model and PortalModel; restore a fallback so model is set if
Metadata.Model is empty—e.g., in the block around MessageMetadata access, set
model = m.Metadata.Model if present else fall back to m.PortalModel (or the
message-level Model field) before building AIResponseParams; ensure the same
populated value is used for both Metadata.Model and PortalModel so downstream
logic (ConvertAIResponse / to_matrix conversion) always receives a non-empty
model.

In `@pkg/connector/scheduler_cron.go`:
- Around line 329-335: The code sets portal.OtherUserID when empty (using
normalizedCronAgentID and agentUserID) but does not persist that mutation;
update the block in scheduler_cron.go so that after assigning portal.OtherUserID
you call portal.Save(ctx) and handle the error (returning an error tuple like
"error", err.Error(), "" on failure) similar to the pattern used in
scheduler_rooms.go; ensure you reference portal.OtherUserID,
normalizedCronAgentID(&record.Job.AgentID), agentUserID(...), and
portal.Save(ctx) when making the change.

In `@pkg/connector/subagent_spawn.go`:
- Around line 302-304: The current guard skips assigning
childMeta.RuntimeReasoning when reasoningEffort == "" which prevents honoring
"thinking=off"; change the logic in subagent_spawn.go (around the
reasoningEffort handling) to assign childMeta.RuntimeReasoning = reasoningEffort
unconditionally (i.e., remove the if reasoningEffort != "" guard) so an empty
string clears any inherited/default runtime reasoning; ensure you only update
the RuntimeReasoning field and keep existing surrounding behavior.

In `@pkg/connector/tool_availability_configured_test.go`:
- Around line 9-12: Reorder the import block so imports are grouped and sorted:
put any standard library packages first (none in this diff), then external
packages (e.g. "maunium.net/go/mautrix/bridgev2" and
"maunium.net/go/mautrix/bridgev2/database"), and lastly internal packages (e.g.
"github.com/beeper/ai-bridge/pkg/shared/toolspec"); update the import block in
pkg/connector/tool_availability_configured_test.go to follow this grouping and
run goimports/gofmt to ensure correct formatting.

---

Outside diff comments:
In `@pkg/connector/chat.go`:
- Around line 69-89: The function validateDMModelSwitch is unused; either delete
it or explicitly mark it as intentionally unused by adding a nolint with a
comment. If removing, delete the whole validateDMModelSwitch function
(references: validateDMModelSwitch, ErrDMGhostImmutable, effectiveModel,
modelUserID, resolveAgentID) to avoid dead code; otherwise add a comment like
"// nolint:unused -- kept for future DM model validation" above the function to
suppress linter warnings and keep the existing logic.

In `@pkg/connector/model_fallback.go`:
- Around line 32-53: The modelFallbackChain currently builds fallbacks starting
from the agent's primary model which ignores any already-resolved runtime
override in meta; change modelFallbackChain so it first inserts the resolved
model from oc.effectiveModel(meta) (if non-empty) into the models list, then
append the agent's primary and its fallbacks (using ResolveAlias and trimming
empties) to preserve the runtime override as the first candidate; keep using
NewAgentStoreAdapter(oc).GetAgentByID(ctx, agentID) to fetch agent info and
return dedupeModels(models) at the end to remove duplicates.

---

Nitpick comments:
In `@docs/msc/com.beeper.mscXXXX-commands.md`:
- Around line 78-81: Update the Security Considerations to explicitly state the
authorization policy for the "new" command: specify whether the "new" command
requires a minimum room power level (or other ACL) before creating chats and, if
so, the required power-level threshold and the expected behavior when a sender
lacks permission (reject with an error message). If the command is allowed for
all users, document that explicitly and note any mitigations (rate limits, audit
logging). Reference the existing "Command authorization" bullet and the "new"
command name so readers can find the rule and the expected error/response
behavior.

In `@pkg/connector/client.go`:
- Around line 1547-1556: The code redundantly checks SupportsReasoning twice
(via getModelCapabilitiesForMeta(meta) and again by fetching model info from
findModelInfo(effectiveModel(meta))). Simplify by relying on
getModelCapabilitiesForMeta(meta) only: remove the second branch that trims
effectiveModel(meta) and calls findModelInfo(...). Keep the initial check using
getModelCapabilitiesForMeta(meta).SupportsReasoning and return "low" there,
otherwise return "off" — ensure functions getModelCapabilitiesForMeta,
effectiveModel, and findModelInfo are left intact for future use.

In `@pkg/connector/desktop_api_helpers.go`:
- Around line 8-35: Add table-driven tests for parseDesktopAPIAddArgs that
exercise the heuristic and whitespace-collapsing behavior: create a slice of
test cases covering empty args, whitespace-only, token-only, token+url (2 args
where second is URL), name+token (2 args where first is name), name+token+url
(3+ args where remaining parts join to form URL), and variations with extra
internal/leading/trailing whitespace; for each case call parseDesktopAPIAddArgs
and assert the returned name, token, baseURL and error match expected values.
Include negative checks (expecting an error) for missing args and positive
checks that rely on isLikelyHTTPURL and normalizeDesktopInstanceName behavior;
structure tests in a table-driven loop and give each case a descriptive name so
future regressions are obvious.

In `@pkg/connector/integrations_example-config.yaml`:
- Around line 95-96: The example config's replyToMode setting lacks inline docs;
update the replyToMode entry to list supported values (e.g., "first", "all",
"none") and a short description of each and what "first" does (e.g., "first"
replies only to the first message in a thread), and mention its
relation/replacement for legacy responsePrefix so users can migrate; locate the
replyToMode YAML key and add a one-line comment or brief block comment above it
documenting the valid enum values and their behavior.

In `@pkg/connector/provisioning.go`:
- Around line 281-318: The validateAgentModels function performs duplicate
validation for fallbacks: first by appending normalized fallbacks into models
and validating them in the initial loop, then re-validating and re-resolving
them again when building resolvedFallbacks; simplify by validating/resolving
each model exactly once — call client.resolveModelID(ctx, ...) for
agent.Model.Primary and for each entry in
normalizeStringList(agent.Model.Fallbacks) (using the same resolution logic as
for primary), collect resolved fallbacks into resolvedFallbacks and set
agent.Model.Primary/resolvedFallbacks accordingly, and remove the first loop’s
pre-population of models for fallbacks so resolveModelID is invoked only once
per model.
- Around line 662-703: connectMCPServer mutates and persists login state in many
places (calls to setLoginMCPServer + login.Save at different branches) which can
leave intermediate state if a save fails; refactor so you assemble the final cfg
and persist once where possible: keep using
mcpServerNeedsToken/verifyMCPServerConnection but avoid calling
setLoginMCPServer/login.Save multiple times—instead update a local cfg and only
call setLoginMCPServer(loginMetadata(login), target.Name, cfg) followed by
login.Save(ctx) once after the final cfg is determined (or, if you must persist
an intermediate unauthenticated state, centralize that logic into a helper like
persistLoginMCPServer that wraps setLoginMCPServer + login.Save and is called in
explicit, documented places); also ensure client.invalidateMCPToolCache is
called consistently after persistence and propagate errors from login.Save
immediately.

In `@pkg/connector/scheduler_rooms.go`:
- Around line 31-34: The code calls normalizeAgentID(record.Job.AgentID) twice;
compute it once into a local variable (e.g., normalizedAgentID) and reuse it for
both the earlier assignment and when setting portal.OtherUserID (and/or passing
into agentUserID) to avoid duplicate work — update the block that assigns
portal.OtherUserID and where normalizeAgentID is used so portal.Save(ctx)
remains unchanged.
- Around line 59-62: Compute normalizeAgentID(state.AgentID) once and reuse it
instead of calling it twice; assign its result to a local variable (e.g.,
normalizedAgentID) and pass that into agentUserID(...) when setting
portal.OtherUserID and anywhere else in this function that also uses
normalizeAgentID(state.AgentID). Update the code around the portal.OtherUserID
assignment and any earlier usage to reference the new local variable, then call
portal.Save(ctx) as before.

In `@pkg/connector/sessions_tools.go`:
- Around line 488-504: The polling loop in the block using
waitForNewAssistantMessage does not check ctx.Done(), so cancellation can be
ignored; modify the loop to select on ctx.Done() as well as a sleep/tick or use
time.After instead of time.Sleep so the loop exits immediately when the context
is cancelled. Specifically, update the loop that references timeout, deadline,
waitForNewAssistantMessage, lastAssistantID and lastAssistantTimestamp to check
ctx.Done() (e.g., select { case <-ctx.Done(): return nil, ctx.Err() or set
result status to indicate cancellation; case <-time.After(250*time.Millisecond):
/* continue polling */ }) so the function returns promptly on context
cancellation.

In `@pkg/connector/simple_mode_prompt_test.go`:
- Line 119: The test uses single-line struct initialization for PortalMetadata
and ResolvedTarget (meta := &PortalMetadata{ResolvedTarget:
&ResolvedTarget{Kind: ResolvedTargetModel, GhostID:
modelUserID("openai/gpt-5.2"), ModelID: "openai/gpt-5.2"}}) while other tests
use multi-line formatting; reformat this initialization into the same multi-line
style used elsewhere (expand &PortalMetadata, &ResolvedTarget fields across
multiple lines) for consistency and readability, touching the meta variable and
the nested ResolvedTarget fields (Kind, GhostID, ModelID).

In `@pkg/connector/tools.go`:
- Around line 1571-1597: The fmt.Sprintf call that builds status contains a
trailing "%s" in the format string matched by the empty string argument ""
(leftover from removed modelChanged); locate the fmt.Sprintf in
pkg/connector/tools.go that sets status (the block using resolveAgentID,
agentInfo, timeStr, modelName, provider, maxContext, maxTokens, sessionID,
title) and remove the trailing "%s" from the format template and the
corresponding "" argument so the format string and argument list match.

In `@pkg/connector/trace.go`:
- Around line 3-15: Remove the misleading metadata-driven stubs by deleting the
functions traceLevel, traceEnabled, and traceFull from pkg/connector/trace.go
and replace their call sites to use the explicit constants they currently return
("off", false, false); update any callers that pass *PortalMetadata to instead
use the literal values and remove unused meta variables. If inlining across many
call sites is undesirable, alternatively add a clear deprecation comment above
these functions stating they are deprecated and always return the constants, but
prefer deletion and inlining to avoid future confusion.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ff76f6db-1638-4fe6-9280-bfb507b8f746

📥 Commits

Reviewing files that changed from the base of the PR and between bc60804 and 6067ddd.

📒 Files selected for processing (78)

• README.md
• docs/matrix-ai-matrix-spec-v1.md
• docs/msc/com.beeper.mscXXXX-commands.md
• pkg/connector/agent_activity.go
• pkg/connector/agent_display.go
• pkg/connector/agentstore.go
• pkg/connector/builder.go
• pkg/connector/chat.go
• pkg/connector/chat_fork_test.go
• pkg/connector/client.go
• pkg/connector/client_capabilities_test.go
• pkg/connector/command_registry.go
• pkg/connector/commands.go
• pkg/connector/commands_manage.go
• pkg/connector/commands_mcp.go
• pkg/connector/commands_parity.go
• pkg/connector/commands_simple.go
• pkg/connector/connector.go
• pkg/connector/defaults_alignment_test.go
• pkg/connector/desktop_api_helpers.go
• pkg/connector/error_logging.go
• pkg/connector/events.go
• pkg/connector/group_activation.go
• pkg/connector/handleai.go
• pkg/connector/heartbeat_context.go
• pkg/connector/heartbeat_delivery.go
• pkg/connector/heartbeat_execute.go
• pkg/connector/history_limit_test.go
• pkg/connector/identifiers.go
• pkg/connector/inbound_directive_apply.go
• pkg/connector/inbound_prompt_runtime_test.go
• pkg/connector/integration_host.go
• pkg/connector/integrations.go
• pkg/connector/integrations_config.go
• pkg/connector/integrations_example-config.yaml
• pkg/connector/mcp_helpers.go
• pkg/connector/metadata.go
• pkg/connector/model_fallback.go
• pkg/connector/prompt_params.go
• pkg/connector/provisioning.go
• pkg/connector/provisioning_test.go
• pkg/connector/reasoning_fallback.go
• pkg/connector/remote_message.go
• pkg/connector/response_finalization.go
• pkg/connector/response_finalization_test.go
• pkg/connector/response_prefix.go
• pkg/connector/response_prefix_template.go
• pkg/connector/room_settings_event_content_test.go
• pkg/connector/scheduler_cron.go
• pkg/connector/scheduler_rooms.go
• pkg/connector/session_greeting_test.go
• pkg/connector/sessions_tools.go
• pkg/connector/sessions_visibility_test.go
• pkg/connector/simple_mode_prompt.go
• pkg/connector/simple_mode_prompt_test.go
• pkg/connector/status_text.go
• pkg/connector/streaming_chat_completions.go
• pkg/connector/streaming_continuation.go
• pkg/connector/streaming_finish_reason_test.go
• pkg/connector/streaming_init_test.go
• pkg/connector/streaming_params.go
• pkg/connector/streaming_state.go
• pkg/connector/streaming_tool_selection.go
• pkg/connector/streaming_tool_selection_test.go
• pkg/connector/subagent_spawn.go
• pkg/connector/system_prompts.go
• pkg/connector/system_prompts_test.go
• pkg/connector/target_test_helpers_test.go
• pkg/connector/tool_availability_configured_test.go
• pkg/connector/tool_descriptions.go
• pkg/connector/tool_execution.go
• pkg/connector/tool_policy.go
• pkg/connector/tool_policy_apply_patch_test.go
• pkg/connector/tools.go
• pkg/connector/tools_message_actions.go
• pkg/connector/tools_unique_test.go
• pkg/connector/trace.go
• pkg/connector/typing_queue.go

💤 Files with no reviewable changes (17)

• pkg/connector/streaming_state.go
• pkg/connector/heartbeat_context.go
• pkg/connector/agent_display.go
• pkg/connector/commands_manage.go
• pkg/connector/history_limit_test.go
• pkg/connector/room_settings_event_content_test.go
• pkg/connector/commands_simple.go
• pkg/connector/integrations.go
• pkg/connector/prompt_params.go
• pkg/connector/builder.go
• pkg/connector/response_prefix_template.go
• pkg/connector/typing_queue.go
• pkg/connector/sessions_visibility_test.go
• pkg/connector/response_prefix.go
• pkg/connector/inbound_directive_apply.go
• pkg/connector/commands_mcp.go
• pkg/connector/commands_parity.go

📜 Review details

🧰 Additional context used

🧬 Code graph analysis (23)

pkg/connector/tools_message_actions.go (1)

pkg/connector/models_api.go (1)

• ResolveAlias (11-13)

pkg/connector/defaults_alignment_test.go (4)

pkg/connector/metadata.go (3)

• UserLoginMetadata (110-145)
• ModelCache (17-21)
• PortalMetadata (167-200)

pkg/connector/login.go (1)

• ProviderOpenRouter (21-21)

pkg/connector/events.go (1)

• ModelInfo (102-119)

pkg/connector/identifiers.go (2)

• ResolvedTarget (136-141)
• ResolvedTargetModel (132-132)

pkg/connector/simple_mode_prompt_test.go (2)

pkg/connector/identifiers.go (2)

• ResolvedTarget (136-141)
• ResolvedTargetModel (132-132)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/connector/integrations_config.go (1)

pkg/runtime/types.go (1)

• ReplyToMode (61-61)

pkg/connector/tool_policy.go (2)

pkg/connector/events.go (2)

• SourceModelLimit (65-65)
• SourceGlobalDefault (64-64)

pkg/agents/tools/boss.go (1)

• IsBossTool (381-384)

pkg/connector/response_finalization.go (1)

pkg/agents/prompt.go (1)

• HeartbeatToken (94-94)

pkg/connector/agentstore.go (1)

pkg/connector/identifiers.go (1)

• ResolvedTarget (136-141)

pkg/connector/provisioning.go (3)

pkg/connector/metadata.go (4)

• UserLoginMetadata (110-145)
• UserProfile (48-53)
• MCPServerConfig (74-84)
• ServiceTokens (56-65)

pkg/connector/agentstore.go (4)

• FromAgentDefinitionContent (248-280)
• ToAgentDefinitionContent (217-245)
• AgentStoreAdapter (23-26)
• NewAgentStoreAdapter (28-30)

pkg/agents/presets.go (1)

• IsPreset (31-38)

pkg/connector/trace.go (1)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/connector/tool_policy_apply_patch_test.go (2)

pkg/connector/metadata.go (2)

• UserLoginMetadata (110-145)
• ModelCache (17-21)

pkg/connector/login.go (1)

• ProviderOpenAI (20-20)

pkg/connector/streaming_init_test.go (2)

pkg/connector/identifiers.go (2)

• ResolvedTarget (136-141)
• ResolvedTargetModel (132-132)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/connector/scheduler_rooms.go (1)

pkg/integrations/cron/types.go (1)

• Job (60-73)

pkg/connector/streaming_continuation.go (1)

pkg/agents/boss.go (1)

• IsBossAgent (31-33)

pkg/connector/client_capabilities_test.go (2)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/connector/identifiers.go (1)

• ResolvedTarget (136-141)

pkg/connector/client.go (6)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/connector/models_api.go (1)

• ResolveAlias (11-13)

pkg/connector/identifiers.go (3)

• ResolvedTarget (136-141)
• ResolvedTargetModel (132-132)
• ResolvedTargetAgent (133-133)

pkg/connector/agentstore.go (1)

• NewAgentStoreAdapter (28-30)

pkg/connector/models.go (1)

• DefaultModelOpenRouter (20-20)

pkg/agents/prompt.go (1)

• DefaultSystemPrompt (100-100)

pkg/connector/metadata.go (2)

pkg/connector/identifiers.go (2)

• ResolvedTarget (136-141)
• ResolvedTargetModel (132-132)

pkg/shared/jsonutil/jsonutil.go (1)

• DeepCloneAny (42-71)

pkg/connector/group_activation.go (2)

pkg/connector/client.go (1)

• AIClient (265-346)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/connector/commands.go (1)

bridges/opencode/opencode/types.go (1)

• Event (206-209)

pkg/connector/identifiers.go (4)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

bridges/codex/metadata.go (1)

• PortalMetadata (24-32)

bridges/opencode/metadata.go (1)

• PortalMetadata (17-28)

pkg/bridgeadapter/metadata_helpers.go (1)

• EnsurePortalMetadata (28-33)

pkg/connector/chat.go (5)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/agents/boss.go (1)

• IsBossAgent (31-33)

pkg/connector/identifiers.go (1)

• ResolvedTarget (136-141)

pkg/connector/models_api.go (1)

• ResolveAlias (11-13)

pkg/bridgeadapter/identifier_helpers.go (1)

• HumanUserID (22-24)

pkg/connector/target_test_helpers_test.go (2)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

pkg/connector/identifiers.go (3)

• ResolvedTarget (136-141)
• ResolvedTargetModel (132-132)
• ResolvedTargetAgent (133-133)

pkg/connector/tool_availability_configured_test.go (2)

pkg/connector/metadata.go (2)

• UserLoginMetadata (110-145)
• ModelCache (17-21)

pkg/connector/events.go (1)

• ModelInfo (102-119)

pkg/connector/chat_fork_test.go (1)

pkg/connector/identifiers.go (2)

• ResolvedTarget (136-141)
• ResolvedTargetModel (132-132)

🪛 GitHub Actions: Go

pkg/connector/chat.go

[error] 69-69: go-staticcheck-repo-mod: pkg/connector/chat.go:69:21: func (*AIClient).validateDMModelSwitch is unused (U1000)

pkg/connector/tool_availability_configured_test.go

[error] 1-1: pre-commit hook go-imports-repo failed: Files were modified by this hook during 'pre-commit run'.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Agent model resolution may return empty string instead of fallback.

When ResolvedTarget.Kind is ResolvedTargetAgent but the agent lookup fails (line 1258-1261), or the agent has no Model.Primary, the function returns an empty string instead of falling through to defaultModelForProvider(). This could cause downstream issues where callers expect a valid model ID.

Consider falling through to the default when agent resolution fails:

🐛 Proposed fix

 		case ResolvedTargetAgent:
 			store := NewAgentStoreAdapter(oc)
 			agent, err := store.GetAgentByID(context.Background(), meta.ResolvedTarget.AgentID)
 			if err == nil && agent != nil && agent.Model.Primary != "" {
 				return ResolveAlias(agent.Model.Primary)
 			}
-			return ""
-		default:
-			return ""
+			// Fall through to default model
 		}
 	}
 	return oc.defaultModelForProvider()

📝 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

	// effectiveModel returns the full prefixed model ID (e.g., "openai/gpt-5.2")
	// Priority: Room → Agent → User → Provider → Global
	// Exception: Boss agent rooms always use the Boss agent's model (no overrides)
	// based only on the resolved room target.
	func (oc *AIClient) effectiveModel(meta *PortalMetadata) string {
	// Check if an agent is assigned
	if meta != nil {
	agentID := resolveAgentID(meta)
	if agentID != "" {
	// Load the agent to get its model
	if meta != nil && strings.TrimSpace(meta.RuntimeModelOverride) != "" {
	return ResolveAlias(meta.RuntimeModelOverride)
	}
	if meta != nil && meta.ResolvedTarget != nil {
	switch meta.ResolvedTarget.Kind {
	case ResolvedTargetModel:
	return ResolveAlias(meta.ResolvedTarget.ModelID)
	case ResolvedTargetAgent:
	store := NewAgentStoreAdapter(oc)
	agent, err := store.GetAgentByID(context.Background(), agentID)
	if err == nil && agent != nil {
	// Boss agent rooms always use the Boss model - no overrides allowed
	if agents.IsBossAgent(agentID) && agent.Model.Primary != "" {
	return ResolveAlias(agent.Model.Primary)
	}
	// For other agents, room override takes priority, then agent model
	if meta.Model != "" {
	return ResolveAlias(meta.Model)
	}
	if override := oc.agentModelOverride(agentID); override != "" {
	return ResolveAlias(override)
	}
	if agent.Model.Primary != "" {
	return ResolveAlias(agent.Model.Primary)
	}
	agent, err := store.GetAgentByID(context.Background(), meta.ResolvedTarget.AgentID)
	if err == nil && agent != nil && agent.Model.Primary != "" {
	return ResolveAlias(agent.Model.Primary)
	}
	return ""
	default:
	return ""
	}
	}
	// Room-level model override (for rooms without an agent)
	if meta != nil && meta.Model != "" {
	return ResolveAlias(meta.Model)
	}
	// User-level default
	loginMeta := loginMetadata(oc.UserLogin)
	if loginMeta.Defaults != nil && loginMeta.Defaults.Model != "" {
	return ResolveAlias(loginMeta.Defaults.Model)
	}
	// Provider default from config
	return oc.defaultModelForProvider()
	}
	// effectiveModel returns the full prefixed model ID (e.g., "openai/gpt-5.2")
	// based only on the resolved room target.
	func (oc *AIClient) effectiveModel(meta *PortalMetadata) string {
	if meta != nil && strings.TrimSpace(meta.RuntimeModelOverride) != "" {
	return ResolveAlias(meta.RuntimeModelOverride)
	}
	if meta != nil && meta.ResolvedTarget != nil {
	switch meta.ResolvedTarget.Kind {
	case ResolvedTargetModel:
	return ResolveAlias(meta.ResolvedTarget.ModelID)
	case ResolvedTargetAgent:
	store := NewAgentStoreAdapter(oc)
	agent, err := store.GetAgentByID(context.Background(), meta.ResolvedTarget.AgentID)
	if err == nil && agent != nil && agent.Model.Primary != "" {
	return ResolveAlias(agent.Model.Primary)
	}
	// Fall through to default model
	}
	}
	return oc.defaultModelForProvider()
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/client.go` around lines 1246 - 1268, The effectiveModel
function currently returns an empty string when ResolvedTarget.Kind ==
ResolvedTargetAgent and either store.GetAgentByID fails or agent.Model.Primary
is empty; change this so that instead of returning "", the code falls through to
call oc.defaultModelForProvider() (i.e., remove the early return in the
ResolvedTargetAgent branch and only return ResolveAlias(agent.Model.Primary)
when present), using the same NewAgentStoreAdapter/ store.GetAgentByID logic to
attempt agent lookup but delegating to oc.defaultModelForProvider() on any
failure or missing agent.Model.Primary.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don’t block auto-greeting for model-only rooms.

Line 229 still equates “no resolved agent” with “no target”, but Lines 366-369 now explicitly support rooms that only resolve to a model. After this change those rooms will get the welcome notice but scheduleAutoGreeting will exit with "no-agent", so the automatic first assistant message never runs.

💡 Suggested fix

-func autoGreetingBlockReason(meta *PortalMetadata) string {
+func (oc *AIClient) autoGreetingBlockReason(meta *PortalMetadata) string {
 	switch {
 	case isInternalControlRoom(meta):
 		return "internal-control-room"
-	case resolveAgentID(meta) == "":
-		return "no-agent"
+	case resolveAgentID(meta) == "" && oc.effectiveModel(meta) == "":
+		return "no-target"
 	}
 	return ""
 }

-	if autoGreetingBlockReason(meta) != "" {
+	if oc.autoGreetingBlockReason(meta) != "" {
 		return
 	}

-			if reason := autoGreetingBlockReason(currentMeta); reason != "" {
+			if reason := oc.autoGreetingBlockReason(currentMeta); reason != "" {
 				oc.Log().Debug().Stringer("room_id", roomID).Str("reason", reason).Msg("auto-greeting loop exiting: blocked by portal state")
 				return
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/handleai.go` around lines 225 - 230, autoGreetingBlockReason
currently treats an empty resolveAgentID(meta) as "no-agent", which blocks
auto-greeting for rooms that resolve to a model-only target; update
autoGreetingBlockReason so it only returns "no-agent" when there is truly no
target (neither agent nor model). Concretely, change the resolveAgentID(meta) ==
"" branch to consult the same predicate used by scheduleAutoGreeting (the logic
around lines 366-369 that allows model-only rooms) — i.e., only return
"no-agent" if resolveAgentID(meta) == "" AND the room does not resolve to a
model; otherwise allow auto-greeting to proceed. Ensure the fix references
autoGreetingBlockReason, resolveAgentID, and the same model-resolution check
used by scheduleAutoGreeting.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Guard the welcome copy when the model name isn’t resolved yet.

scheduleWelcomeMessage only waits for a Matrix room ID, so this branch can run before effectiveModel(meta) resolves. In that case displayName can be empty and the notice becomes You are chatting with ..

💡 Suggested fix

 	if resolveAgentID(meta) == "" {
 		modelID := oc.effectiveModel(meta)
 		displayName := modelContactName(modelID, oc.findModelInfo(modelID))
-		oc.sendSystemNotice(bgCtx, portal, fmt.Sprintf("You are chatting with %s. AI can make mistakes.", displayName))
+		if displayName == "" {
+			oc.sendSystemNotice(bgCtx, portal, "AI can make mistakes.")
+		} else {
+			oc.sendSystemNotice(bgCtx, portal, fmt.Sprintf("You are chatting with %s. AI can make mistakes.", displayName))
+		}
 	} else {

📝 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

	if resolveAgentID(meta) == "" {
	modelID := oc.effectiveModel(meta)
	displayName := modelContactName(modelID, oc.findModelInfo(modelID))
	oc.sendSystemNotice(bgCtx, portal, fmt.Sprintf("You are chatting with %s. AI can make mistakes.", displayName))
	if resolveAgentID(meta) == "" {
	modelID := oc.effectiveModel(meta)
	displayName := modelContactName(modelID, oc.findModelInfo(modelID))
	if displayName == "" {
	oc.sendSystemNotice(bgCtx, portal, "AI can make mistakes.")
	} else {
	oc.sendSystemNotice(bgCtx, portal, fmt.Sprintf("You are chatting with %s. AI can make mistakes.", displayName))
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/handleai.go` around lines 366 - 369, The welcome notice can be
sent before effectiveModel(meta) resolves, producing "You are chatting with ." —
modify the branch around resolveAgentID(meta) in scheduleWelcomeMessage to guard
the displayName: call oc.effectiveModel(meta) and modelContactName(...) as
before but if modelID or displayName is empty then either skip/skip sending
oc.sendSystemNotice(bgCtx, portal, ...) or use a safe fallback like "an AI"
(e.g., "You are chatting with an AI. AI can make mistakes."); ensure the change
targets the code that calls oc.sendSystemNotice so you don't send the blank-name
notice.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Search for any remaining references to ResponsePrefix in the codebase

# Search for struct field access patterns
rg -nP '\.(ResponsePrefix|responsePrefix)\b' -g '!*.md' -g '!*_test.go'

# Search for YAML key usage in configs or tests
rg -n 'responsePrefix|response_prefix' -g '*.yaml' -g '*.yml' -g '*config*.go'

# Search for ResponsePrefix in struct literals
ast-grep --pattern 'ResponsePrefix: $_'

Repository: beeper/AgentRemote

Length of output: 44

---

🏁 Script executed:

# First, view the file to confirm the changes
cat -n pkg/connector/integrations_config.go | sed -n '160,195p'

Repository: beeper/AgentRemote

Length of output: 1691

---

🏁 Script executed:

# More comprehensive search for ResponsePrefix usage
rg "ResponsePrefix" -i --type go --type yaml --type yml

Repository: beeper/AgentRemote

Length of output: 89

---

🏁 Script executed:

# Check for any YAML config files that might reference responsePrefix
fd -e yaml -e yml . | head -20

Repository: beeper/AgentRemote

Length of output: 210

---

🏁 Script executed:

# Search in test files too
rg "ResponsePrefix" -i --type go

Repository: beeper/AgentRemote

Length of output: 44

---

🏁 Script executed:

# Search in example config for responsePrefix
cat pkg/connector/integrations_example-config.yaml | grep -i "responseprefix" || echo "No responsePrefix found in example config"

Repository: beeper/AgentRemote

Length of output: 103

---

🏁 Script executed:

# View the upgradeConfig function to see how config migration is handled
cat -n pkg/connector/integrations_config.go | sed -n '464,510p'

Repository: beeper/AgentRemote

Length of output: 2775

---

🏁 Script executed:

# Check if there's any git history showing the ResponsePrefix removal
git log --oneline -n 20 -- pkg/connector/integrations_config.go

Repository: beeper/AgentRemote

Length of output: 107

---

🏁 Script executed:

# Search more broadly for any response prefix related code
rg "response.*prefix|responseprefix" -i --type go -A 2 -B 2

Repository: beeper/AgentRemote

Length of output: 44

---

Removing ResponsePrefix fields without config migration path.

The removal of the ResponsePrefix field from ChannelDefaultsConfig and ChannelConfig is a breaking change. While no remaining code references were found (cleanup appears complete), the upgradeConfig function does not include migration logic for responsePrefix config values. Users upgrading will have these settings silently dropped without warning. Consider adding explicit migration logic to either preserve the values, warn users, or explicitly drop them with a deprecation notice.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/integrations_config.go` around lines 164 - 171,
ChannelDefaultsConfig and ChannelConfig had their ResponsePrefix fields removed
but upgradeConfig currently drops any existing responsePrefix values silently;
update the upgradeConfig function to detect legacy "responsePrefix" entries
(e.g., in the parsed YAML/JSON map or raw config blob) and migrate them into the
new config model or emit a clear deprecation/warning so users aren't silently
losing settings. Locate the upgradeConfig function and add logic to look for
"responsePrefix" for both channel defaults and per-channel entries, then either
copy the value to the appropriate new location, store it in a preserved
deprecated field, or log a warning with the channel identifier; ensure the code
references ChannelDefaultsConfig and ChannelConfig when applying the migration
so the change is explicit and traceable.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fragile error comparison using string matching.

Comparing err.Error() != "not found" is fragile and could break if the error message changes. Consider using errors.Is with a sentinel error or checking for a specific error type.

🛡️ Suggested improvement

Define a sentinel error in the MCP resolution logic:

var ErrMCPServerNotFound = errors.New("mcp server not found")

Then use:

-	if err != nil && err.Error() != "not found" {
+	if err != nil && !errors.Is(err, ErrMCPServerNotFound) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/provisioning.go` around lines 612 - 616, The code in the
resolveNamedMCPServer call uses a fragile string comparison on err.Error();
change this to detect the “not found” condition via a sentinel error or specific
error type instead: introduce or use a sentinel like ErrMCPServerNotFound
returned by resolveNamedMCPServer and replace `err.Error() != "not found"` with
`errors.Is(err, ErrMCPServerNotFound)` (importing the errors package), and keep
the mautrix.MInvalidParam response only for non-not-found errors (i.e., if err
!= nil && !errors.Is(err, ErrMCPServerNotFound) { ... }).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "OpenAIRemoteMessage construction sites:"
rg -nP --type=go -C4 'OpenAIRemoteMessage\s*\{'

echo
echo "MessageMetadata struct literals:"
rg -nP --type=go -C4 'MessageMetadata\s*\{'

echo
echo "Direct assignments to Metadata.Model:"
rg -nP --type=go -C4 '\.Metadata\.Model\s*='

Repository: beeper/AgentRemote

Length of output: 20579

---

🏁 Script executed:

cd pkg/connector && head -n 95 remote_message.go | tail -n 25

Repository: beeper/AgentRemote

Length of output: 933

---

🏁 Script executed:

rg -n "ConvertMessage|ConvertAIResponse" --type=go -B2 -A2

Repository: beeper/AgentRemote

Length of output: 2652

---

🏁 Script executed:

rg -n "\.Model\s*=" --type=go | grep -i "metadata\|message"

Repository: beeper/AgentRemote

Length of output: 231

---

🏁 Script executed:

sed -n '107,125p' pkg/connector/remote_message.go

Repository: beeper/AgentRemote

Length of output: 534

---

🏁 Script executed:

sed -n '369,388p' pkg/connector/msgconv/to_matrix.go

Repository: beeper/AgentRemote

Length of output: 701

---

🏁 Script executed:

rg -n "\.Metadata\.Model\s*=" --type=go -B3 -A1 | grep -E "OpenAIRemoteMessage|\.Metadata\.Model"

Repository: beeper/AgentRemote

Length of output: 126

---

🏁 Script executed:

rg -n "MessageMetadata\s*\{" --type=go -A10 | grep -E "Model|MessageMetadata"

Repository: beeper/AgentRemote

Length of output: 4643

---

Ensure model attribution is populated before removing the fallback.

The removed fallback was the only mechanism providing model metadata to ConvertAIResponse. Since MessageMetadata is never constructed with a Model field across the codebase (streaming, response finalization, etc.), and model is passed as both Metadata.Model and PortalModel to AIResponseParams, the downstream fallback in to_matrix.go:405 cannot compensate—both parameters end up empty. Verify that every AI response path explicitly sets Metadata.Model before ConvertMessage is called, or restore a fallback mechanism.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/remote_message.go` around lines 83 - 87, The code removed the
fallback that populated model metadata, leaving ConvertAIResponse/ConvertMessage
with empty AIResponseParams.Model and PortalModel; restore a fallback so model
is set if Metadata.Model is empty—e.g., in the block around MessageMetadata
access, set model = m.Metadata.Model if present else fall back to m.PortalModel
(or the message-level Model field) before building AIResponseParams; ensure the
same populated value is used for both Metadata.Model and PortalModel so
downstream logic (ConvertAIResponse / to_matrix conversion) always receives a
non-empty model.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if portalByRoomID returns a cached/shared portal object
rg -n -A5 'func.*portalByRoomID' --type=go

Repository: beeper/AgentRemote

Length of output: 494

---

🏁 Script executed:

#!/bin/bash
# Get full context around lines 329-335 in scheduler_cron.go
sed -n '320,345p' pkg/connector/scheduler_cron.go

Repository: beeper/AgentRemote

Length of output: 1203

---

🏁 Script executed:

#!/bin/bash
# Look for where the portal is used after the mutation, and check for updates/saves
sed -n '329,380p' pkg/connector/scheduler_cron.go

Repository: beeper/AgentRemote

Length of output: 2565

---

🏁 Script executed:

#!/bin/bash
# Find the Bridge.GetPortalByMXID implementation to understand if it returns cached objects
rg -n 'func.*GetPortalByMXID' --type=go -A5

Repository: beeper/AgentRemote

Length of output: 44

---

🏁 Script executed:

#!/bin/bash
# Search for patterns of portal.OtherUserID being set in the codebase to see how commonly this is done
rg -n 'portal\.OtherUserID\s*=' --type=go | head -20

Repository: beeper/AgentRemote

Length of output: 1260

---

🏁 Script executed:

#!/bin/bash
# Check if there's a Save/Update method called on portal objects after mutation
rg -n 'portal\.OtherUserID.*=' -A3 --type=go | grep -E '(Save|Update|Put|Insert)' | head -20

Repository: beeper/AgentRemote

Length of output: 410

---

🏁 Script executed:

#!/bin/bash
# Look for where Bridge.GetPortalByMXID is defined to understand object ownership
rg -n 'GetPortalByMXID' --type=go -B2 | grep -E '(func|interface|type)' | head -10

Repository: beeper/AgentRemote

Length of output: 189

---

🏁 Script executed:

#!/bin/bash
# Check if there are any comments or patterns around portal mutations indicating intent
rg -B2 'portal\.OtherUserID\s*=' --type=go | head -30

Repository: beeper/AgentRemote

Length of output: 1378

---

🏁 Script executed:

#!/bin/bash
# Check if portal.Save is called in scheduler_cron.go after the mutation
sed -n '320,380p' pkg/connector/scheduler_cron.go | grep -i save

Repository: beeper/AgentRemote

Length of output: 44

---

🏁 Script executed:

#!/bin/bash
# Get the full function context to see if portal is saved anywhere
rg -n 'func.*executeCron' --type=go -A100 pkg/connector/scheduler_cron.go | head -120

Repository: beeper/AgentRemote

Length of output: 4693

---

🏁 Script executed:

#!/bin/bash
# Check the specific context in scheduler_rooms.go to understand the persistent mutation pattern
sed -n '25,65p' pkg/connector/scheduler_rooms.go

Repository: beeper/AgentRemote

Length of output: 1179

---

🏁 Script executed:

#!/bin/bash
# Check if there's any documentation or tests explaining the expected behavior
rg -n 'OtherUserID.*cron' --type=go -B3 -A3

Repository: beeper/AgentRemote

Length of output: 44

---

Missing portal.Save(ctx) after mutation of portal.OtherUserID.

The code sets portal.OtherUserID on line 330 but does not persist the change. This is inconsistent with the same mutation pattern in scheduler_rooms.go (lines 31-32, 59-60), where portal.OtherUserID is immediately followed by portal.Save(ctx). Add the persistence call after line 330 to ensure the association is saved:

if portal.OtherUserID == "" {
    portal.OtherUserID = agentUserID(normalizedCronAgentID(&record.Job.AgentID))
    if err := portal.Save(ctx); err != nil {
        return "error", err.Error(), ""
    }
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/scheduler_cron.go` around lines 329 - 335, The code sets
portal.OtherUserID when empty (using normalizedCronAgentID and agentUserID) but
does not persist that mutation; update the block in scheduler_cron.go so that
after assigning portal.OtherUserID you call portal.Save(ctx) and handle the
error (returning an error tuple like "error", err.Error(), "" on failure)
similar to the pattern used in scheduler_rooms.go; ensure you reference
portal.OtherUserID, normalizedCronAgentID(&record.Job.AgentID),
agentUserID(...), and portal.Save(ctx) when making the change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Apply thinking=off by clearing RuntimeReasoning explicitly.

off is a valid input, but it maps to "", so this guard drops the override entirely. If childMeta already carries a default/inherited reasoning value, the spawned session will still run with reasoning enabled.

Suggested fix

  childMeta.SubagentParentRoomID = portal.MXID.String()
- if reasoningEffort != "" {
- 	childMeta.RuntimeReasoning = reasoningEffort
- }
+ childMeta.RuntimeReasoning = reasoningEffort

📝 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

	if reasoningEffort != "" {
	childMeta.ReasoningEffort = reasoningEffort
	childMeta.RuntimeReasoning = reasoningEffort
	}
	childMeta.SubagentParentRoomID = portal.MXID.String()
	childMeta.RuntimeReasoning = reasoningEffort

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/subagent_spawn.go` around lines 302 - 304, The current guard
skips assigning childMeta.RuntimeReasoning when reasoningEffort == "" which
prevents honoring "thinking=off"; change the logic in subagent_spawn.go (around
the reasoningEffort handling) to assign childMeta.RuntimeReasoning =
reasoningEffort unconditionally (i.e., remove the if reasoningEffort != ""
guard) so an empty string clears any inherited/default runtime reasoning; ensure
you only update the RuntimeReasoning field and keep existing surrounding
behavior.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

pkg/connector/trace.go (1)

3-11: These stub functions have active call sites throughout the codebase—removing them would require coordinated cleanup.

Both traceEnabled and traceFull are called in 12 and 8 locations respectively (across handlematrix.go, internal_dispatch.go, sessions_tools.go, and client.go), making them active stubs rather than dead code. Since they unconditionally return false, tracing is effectively disabled everywhere. If tracing will remain permanently disabled, consider:

1. Removing the function definitions and all call sites together, or
2. Keeping them as placeholders if tracing may be re-enabled in the future.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/trace.go` around lines 3 - 11, The two stub helpers
traceEnabled and traceFull currently always return false while being called from
many places (e.g., handlematrix.go, internal_dispatch.go, sessions_tools.go,
client.go); replace the stubs with real checks that read tracing configuration
from the PortalMetadata passed in (e.g., inspect
meta.TraceEnabled/meta.TraceFull or meta.Config["trace"] or similar fields)
and/or fall back to an env var (e.g., PORTAL_TRACE or PORTAL_TRACE_FULL) so
tracing can be enabled without removing call sites; if those metadata fields do
not exist yet, add boolean accessors on PortalMetadata (or a GetTraceLevel
method) and use them in traceEnabled and traceFull, ensuring nil-safe checks on
meta.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/connector/identifiers.go`:
- Around line 142-148: portalMeta currently calls
bridgeadapter.EnsurePortalMetadata[PortalMetadata](portal) but then accesses
portal.OtherUserID without guarding against portal being nil; modify portalMeta
to check portal != nil before reading portal.OtherUserID (e.g., only call
resolveTargetFromGhostID and assign meta.ResolvedTarget when portal is non-nil,
or use a safe default like an empty string), referencing the portalMeta
function, EnsurePortalMetadata, PortalMetadata, and resolveTargetFromGhostID so
the nil check occurs before any access to portal.OtherUserID.

---

Nitpick comments:
In `@pkg/connector/trace.go`:
- Around line 3-11: The two stub helpers traceEnabled and traceFull currently
always return false while being called from many places (e.g., handlematrix.go,
internal_dispatch.go, sessions_tools.go, client.go); replace the stubs with real
checks that read tracing configuration from the PortalMetadata passed in (e.g.,
inspect meta.TraceEnabled/meta.TraceFull or meta.Config["trace"] or similar
fields) and/or fall back to an env var (e.g., PORTAL_TRACE or PORTAL_TRACE_FULL)
so tracing can be enabled without removing call sites; if those metadata fields
do not exist yet, add boolean accessors on PortalMetadata (or a GetTraceLevel
method) and use them in traceEnabled and traceFull, ensuring nil-safe checks on
meta.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1f63bd9f-3800-4b54-8ee4-c315897808d8

📥 Commits

Reviewing files that changed from the base of the PR and between 6067ddd and 6738f93.

📒 Files selected for processing (12)

• pkg/connector/chat.go
• pkg/connector/command_aliases.go
• pkg/connector/commands_helpers.go
• pkg/connector/connector.go
• pkg/connector/handlematrix.go
• pkg/connector/identifiers.go
• pkg/connector/parse_utils.go
• pkg/connector/queue_directive.go
• pkg/connector/queue_notice.go
• pkg/connector/status_text.go
• pkg/connector/tool_availability_configured_test.go
• pkg/connector/trace.go

💤 Files with no reviewable changes (6)

• pkg/connector/command_aliases.go
• pkg/connector/commands_helpers.go
• pkg/connector/handlematrix.go
• pkg/connector/queue_directive.go
• pkg/connector/queue_notice.go
• pkg/connector/parse_utils.go

📜 Review details

🧰 Additional context used

🧬 Code graph analysis (3)

pkg/connector/tool_availability_configured_test.go (3)

pkg/connector/metadata.go (2)

• UserLoginMetadata (110-145)
• ModelCache (17-21)

pkg/connector/events.go (1)

• ModelInfo (102-119)

pkg/agents/types.go (1)

• ModelInfo (181-198)

pkg/connector/identifiers.go (4)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

bridges/codex/metadata.go (1)

• PortalMetadata (24-32)

bridges/opencode/metadata.go (1)

• PortalMetadata (17-28)

pkg/bridgeadapter/metadata_helpers.go (1)

• EnsurePortalMetadata (28-33)

pkg/connector/chat.go (7)

pkg/connector/metadata.go (1)

• PortalMetadata (167-200)

bridges/codex/metadata.go (1)

• PortalMetadata (24-32)

bridges/opencode/metadata.go (1)

• PortalMetadata (17-28)

pkg/agents/boss.go (1)

• IsBossAgent (31-33)

pkg/connector/identifiers.go (1)

• ResolvedTarget (114-119)

pkg/connector/models_api.go (1)

• ResolveAlias (11-13)

pkg/bridgeadapter/identifier_helpers.go (1)

• HumanUserID (22-24)

🔇 Additional comments (14)

pkg/connector/connector.go (1)

146-158: Looks good.

The narrower registration path is clear, and the guarded type assertion avoids turning an unexpected Matrix connector implementation into a startup panic.

pkg/connector/identifiers.go (3)

108-119: LGTM! Clean abstraction for target resolution.

The ResolvedTarget struct with associated constants provides a clear, type-safe representation for distinguishing between model and agent targets. Using an empty string for ResolvedTargetUnknown is idiomatic Go.

---

121-140: LGTM! Robust ghost ID parsing.

The function correctly handles:

• Empty ghost ID (returns nil)
• Model ghost IDs with proper URL unescaping
• Agent ghost IDs with compound ID filtering (strings.Contains(ghostID, ":model-"))
• Trimming whitespace on extracted IDs

---

150-155: LGTM! Simplified agent ID resolution.

The function now cleanly delegates to ResolvedTarget.AgentID, with proper nil checks for both meta and meta.ResolvedTarget.

pkg/connector/tool_availability_configured_test.go (2)

9-12: Import ordering is now correct.

The imports are properly grouped: external packages (maunium.net/go/mautrix/bridgev2, maunium.net/go/mautrix/bridgev2/database) followed by a blank line and internal packages (github.com/beeper/ai-bridge/pkg/shared/toolspec). The past review comment has been addressed.

---

28-32: LGTM! Consistent test setup with ModelCache.

All tests now properly inject a UserLogin with a ModelCache containing a model entry that supports tool calling. This aligns with the broader refactor to use resolved-target-based metadata and ensures tool availability tests have the required model capabilities configured.

Also applies to: 57-61, 80-84, 99-103

pkg/connector/status_text.go (1)

81-89: LGTM! Simplified capability reporting.

The status text now uses getRoomCapabilities(ctx, meta) for a consolidated view of room capabilities, replacing the previous per-field extraction. The "Features:" line provides a clear, concise summary of all supported capabilities.

pkg/connector/chat.go (7)

37-43: LGTM! Simplified agent detection.

Both hasAssignedAgent and hasBossAgent now delegate to resolveAgentID(meta), which uses the new ResolvedTarget-based resolution. This is cleaner and consistent with the refactored architecture.

---

555-573: LGTM! Agent chat creation with ResolvedTarget.

The flow correctly:

1. Initializes the portal with simplified PortalInitOpts
2. Sets portal.OtherUserID to the agent ghost
3. Populates pm.ResolvedTarget from the agent ghost ID
4. Conditionally sets RuntimeModelOverride when applyModelOverride is true

---

641-646: LGTM! Simplified PortalInitOpts.

Removing SystemPrompt from PortalInitOpts aligns with the PR objective of removing per-room system prompts. The struct now contains only the essential fields for portal initialization.

---

648-661: LGTM! Clean fork metadata cloning.

The cloneForkPortalMetadata function now:

1. Copies only essential fields (Slug, Title)
2. Deep-copies ResolvedTarget if present (avoiding shared pointer issues)

This is a clean approach that avoids carrying over stale configuration from the source portal.

---

697-700: LGTM! Minimal metadata initialization.

When CopyFrom is nil, only Slug and Title are set, letting the ResolvedTarget be derived later from portal.OtherUserID via portalMeta().

---

1000-1004: LGTM! Simplified BroadcastRoomState.

The function now focuses on its core responsibilities: updating capabilities and broadcasting command descriptions. This aligns with the removal of per-room settings broadcasts.

---

1215-1215: LGTM! Default chat uses ResolvedTarget.

The default Beeper agent chat correctly sets pm.ResolvedTarget from the agent ghost ID, consistent with the pattern used elsewhere in the file.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Consider guarding against nil portal before accessing OtherUserID.

While bridgeadapter.EnsurePortalMetadata handles nil portal by returning a new empty struct, Line 145 accesses portal.OtherUserID unconditionally. If portal is nil, this will panic.

🛡️ Proposed fix

 func portalMeta(portal *bridgev2.Portal) *PortalMetadata {
 	meta := bridgeadapter.EnsurePortalMetadata[PortalMetadata](portal)
-	if meta != nil {
+	if meta != nil && portal != nil {
 		meta.ResolvedTarget = resolveTargetFromGhostID(portal.OtherUserID)
 	}
 	return meta
 }

📝 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

	func portalMeta(portal *bridgev2.Portal) *PortalMetadata {
	return bridgeadapter.EnsurePortalMetadata[PortalMetadata](portal)
	meta := bridgeadapter.EnsurePortalMetadata[PortalMetadata](portal)
	if meta != nil {
	meta.ResolvedTarget = resolveTargetFromGhostID(portal.OtherUserID)
	}
	return meta
	}
	func portalMeta(portal *bridgev2.Portal) *PortalMetadata {
	meta := bridgeadapter.EnsurePortalMetadata[PortalMetadata](portal)
	if meta != nil && portal != nil {
	meta.ResolvedTarget = resolveTargetFromGhostID(portal.OtherUserID)
	}
	return meta
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/identifiers.go` around lines 142 - 148, portalMeta currently
calls bridgeadapter.EnsurePortalMetadata[PortalMetadata](portal) but then
accesses portal.OtherUserID without guarding against portal being nil; modify
portalMeta to check portal != nil before reading portal.OtherUserID (e.g., only
call resolveTargetFromGhostID and assign meta.ResolvedTarget when portal is
non-nil, or use a safe default like an empty string), referencing the portalMeta
function, EnsurePortalMetadata, PortalMetadata, and resolveTargetFromGhostID so
the nil check occurs before any access to portal.OtherUserID.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/msc/com.beeper.mscXXXX-commands.md (1)

38-43: ⚠️ Potential issue | 🟡 Minor

Remove the stray code fence before Structured Invocation.

Line 38 opens a json fence right before the heading, so the heading and prose render as code and the next example block becomes malformed markdown.

📝 Proposed fix

-```json
 ### Structured Invocation

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/msc/com.beeper.mscXXXX-commands.md` around lines 38 - 43, Remove the
stray opening code fence ("```json") immediately before the "### Structured
Invocation" heading so the heading and following prose render as normal
Markdown; ensure that the example JSON blocks remain wrapped in proper
triple-backtick fences and that only actual code examples use ```json, leaving
the "Structured Invocation" heading and its paragraph outside any code fence.

♻️ Duplicate comments (1)

pkg/connector/identifiers.go (1)

142-146: ⚠️ Potential issue | 🔴 Critical

Guard portal before reading OtherUserID.

bridgeadapter.EnsurePortalMetadata returns a new metadata object even when portal is nil, so meta != nil does not protect Line 145. portalMeta(nil) will still panic here.

🛡️ Proposed fix

 func portalMeta(portal *bridgev2.Portal) *PortalMetadata {
 	meta := bridgeadapter.EnsurePortalMetadata[PortalMetadata](portal)
-	if meta != nil {
+	if meta != nil && portal != nil {
 		meta.ResolvedTarget = resolveTargetFromGhostID(portal.OtherUserID)
 	}
 	return meta
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/identifiers.go` around lines 142 - 146, The portalMeta function
reads portal.OtherUserID without guarding portal; because
bridgeadapter.EnsurePortalMetadata can return a metadata object even when portal
is nil, calling portalMeta(nil) panics. Modify portalMeta (referencing
portalMeta, bridgeadapter.EnsurePortalMetadata and resolveTargetFromGhostID) to
check portal != nil before accessing portal.OtherUserID and only set
meta.ResolvedTarget when portal is non-nil (or compute a safe default when
portal is nil), ensuring you still call EnsurePortalMetadata but avoid
dereferencing portal.

🧹 Nitpick comments (3)

pkg/connector/mcp_helpers.go (1)

137-152: Accepting nil context is an anti-pattern in Go.

Go convention expects callers to always pass a non-nil context (at minimum context.Background() or context.TODO()). Accepting nil here masks potential caller bugs where context is forgotten. Consider requiring a non-nil context and letting callers explicitly choose context.Background() when appropriate.

Also, the 10-second timeout cap is a magic number that could be extracted to a package-level constant for clarity.

Suggested refactor

 func (oc *AIClient) verifyMCPServerConnection(ctx context.Context, server namedMCPServer) (int, error) {
-	if ctx == nil {
-		ctx = context.Background()
-	}
 	callCtx := ctx
 	var cancel context.CancelFunc
 	if _, hasDeadline := callCtx.Deadline(); !hasDeadline {
 		timeout := oc.mcpRequestTimeout()
-		if timeout > 10*time.Second {
-			timeout = 10 * time.Second
+		const maxVerifyTimeout = 10 * time.Second
+		if timeout > maxVerifyTimeout {
+			timeout = maxVerifyTimeout
 		}
 		callCtx, cancel = context.WithTimeout(ctx, timeout)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/mcp_helpers.go` around lines 137 - 152, The function
verifyMCPServerConnection currently accepts a nil ctx and substitutes
context.Background(), which hides caller bugs; change it to require a non-nil
context by removing the ctx == nil branch and instead return an error (or panic)
if ctx is nil so callers must pass context.Background()/TODO explicitly; also
replace the magic "10*time.Second" cap with a package-level constant (e.g.,
MCPRequestTimeoutCap = 10 * time.Second) and use oc.mcpRequestTimeout() capped
by that constant when creating the callCtx; keep the rest of the
context/WithTimeout logic and the defer cancel behavior unchanged.

pkg/connector/events.go (1)

73-73: Consider using ToolType instead of string for the Type field.

A ToolType alias is already defined on line 49 with specific constants (ToolTypeBuiltin, ToolTypeProvider, etc.). Using the typed alias would provide better type safety and self-documentation.

♻️ Suggested change

 type ToolInfo struct {
 	Name        string        `json:"name"`
 	DisplayName string        `json:"display_name"`
-	Type        string        `json:"type"`
+	Type        ToolType      `json:"type"`
 	Description string        `json:"description,omitempty"`
 	Enabled     bool          `json:"enabled"`
 	Available   bool          `json:"available"`
 	Source      SettingSource `json:"source,omitempty"`
 	Reason      string        `json:"reason,omitempty"`
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/events.go` at line 73, Replace the untyped string for the Type
field with the existing alias ToolType to improve type safety: change the struct
field declaration that currently reads Type string `json:"type"` to use Type
ToolType `json:"type"`, and update any usages or assignments (in
functions/methods that reference the struct) to construct or cast values to
ToolType (referencing the ToolType constants like ToolTypeBuiltin,
ToolTypeProvider) so JSON marshaling/unmarshaling behavior remains the same.

pkg/connector/error_logging.go (1)

42-49: Remove the blank assignment.

Line 49 (_ = meta) is unnecessary—Go doesn't require blank assignments for unused function parameters. This creates noise and falsely suggests meta still contributes to the request summary when it no longer does. Remove the line, and consider eliminating the stale meta parameter from this helper entirely.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/error_logging.go` around lines 42 - 49, The helper
addRequestSummary currently contains an unnecessary blank assignment "_ = meta";
remove that line and, since meta is unused, either remove the meta parameter
from addRequestSummary's signature or start using it if intended—update all
callers of addRequestSummary accordingly (look for references to
addRequestSummary and its meta parameter) so the function signature and usages
remain consistent.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/connector/chat.go`:
- Around line 652-659: cloneForkPortalMetadata currently only copies
ResolvedTarget into the new PortalMetadata and thus drops any runtime room
override; update the cloning logic in cloneForkPortalMetadata to also copy the
room/runtime override fields from src into clone (e.g., clone.RuntimeOverride =
src.RuntimeOverride or perform a deep copy if it's a pointer/struct), ensuring
you check for nil before dereferencing and preserve non-default override values
so the forked portal keeps the same runtime/model/reasoning overrides as the
source.
- Line 782: The call to oc.createAndOpenAgentChat(runCtx, portal, agent,
modelID, false) drops the resolved model override because false disables
RuntimeModelOverride; change it to pass the same predicate/boolean used by
resolveAgentModelForNewChat so the new portal retains the chosen model (i.e.,
compute a keepOverride boolean from resolveAgentModelForNewChat or reuse the
existing predicate and pass it instead of false), and apply the identical change
to the explicit "agent <agent_id>" branch above so both branches preserve the
RuntimeModelOverride consistently.
- Around line 698-700: initPortalForChat is creating minimal metadata (only
Slug/Title) so ResolvedTarget never gets hydrated; update initPortalForChat to
run the constructed metadata through portalMeta (or otherwise call the same
hydration logic used by cloneForkPortalMetadata) so ResolvedTarget is populated,
and change composeChatInfo to pass the actual meta into effectiveModel instead
of calling effectiveModel(nil) so isSimpleMode and model resolution use the
hydrated metadata (refer to symbols: initPortalForChat, portalMeta,
ResolvedTarget, composeChatInfo, effectiveModel, isSimpleMode,
cloneForkPortalMetadata).

In `@pkg/connector/desktop_api_helpers.go`:
- Line 35: The current return builds baseURL by strings.Join(trimmed[2:], " "),
which can produce invalid URLs when extra args exist; change the logic in the
function that returns normalizeDesktopInstanceName(trimmed[0]), trimmed[1], ...
so that the baseURL is taken from trimmed[2] only (or validate/parse trimmed[2]
with net/url.Parse and handle errors) instead of joining trimmed[2:] with
spaces; update any callers expecting joined parts accordingly and reference the
trimmed slice and normalizeDesktopInstanceName to locate the change.

---

Outside diff comments:
In `@docs/msc/com.beeper.mscXXXX-commands.md`:
- Around line 38-43: Remove the stray opening code fence ("```json") immediately
before the "### Structured Invocation" heading so the heading and following
prose render as normal Markdown; ensure that the example JSON blocks remain
wrapped in proper triple-backtick fences and that only actual code examples use
```json, leaving the "Structured Invocation" heading and its paragraph outside
any code fence.

---

Duplicate comments:
In `@pkg/connector/identifiers.go`:
- Around line 142-146: The portalMeta function reads portal.OtherUserID without
guarding portal; because bridgeadapter.EnsurePortalMetadata can return a
metadata object even when portal is nil, calling portalMeta(nil) panics. Modify
portalMeta (referencing portalMeta, bridgeadapter.EnsurePortalMetadata and
resolveTargetFromGhostID) to check portal != nil before accessing
portal.OtherUserID and only set meta.ResolvedTarget when portal is non-nil (or
compute a safe default when portal is nil), ensuring you still call
EnsurePortalMetadata but avoid dereferencing portal.

---

Nitpick comments:
In `@pkg/connector/error_logging.go`:
- Around line 42-49: The helper addRequestSummary currently contains an
unnecessary blank assignment "_ = meta"; remove that line and, since meta is
unused, either remove the meta parameter from addRequestSummary's signature or
start using it if intended—update all callers of addRequestSummary accordingly
(look for references to addRequestSummary and its meta parameter) so the
function signature and usages remain consistent.

In `@pkg/connector/events.go`:
- Line 73: Replace the untyped string for the Type field with the existing alias
ToolType to improve type safety: change the struct field declaration that
currently reads Type string `json:"type"` to use Type ToolType `json:"type"`,
and update any usages or assignments (in functions/methods that reference the
struct) to construct or cast values to ToolType (referencing the ToolType
constants like ToolTypeBuiltin, ToolTypeProvider) so JSON
marshaling/unmarshaling behavior remains the same.

In `@pkg/connector/mcp_helpers.go`:
- Around line 137-152: The function verifyMCPServerConnection currently accepts
a nil ctx and substitutes context.Background(), which hides caller bugs; change
it to require a non-nil context by removing the ctx == nil branch and instead
return an error (or panic) if ctx is nil so callers must pass
context.Background()/TODO explicitly; also replace the magic "10*time.Second"
cap with a package-level constant (e.g., MCPRequestTimeoutCap = 10 *
time.Second) and use oc.mcpRequestTimeout() capped by that constant when
creating the callCtx; keep the rest of the context/WithTimeout logic and the
defer cancel behavior unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c57f2099-9c89-4d38-afa1-178b93d0038d

📥 Commits

Reviewing files that changed from the base of the PR and between 6738f93 and 15a9a38.

📒 Files selected for processing (36)

• README.md
• docs/matrix-ai-matrix-spec-v1.md
• docs/msc/com.beeper.mscXXXX-commands.md
• pkg/connector/agent_activity.go
• pkg/connector/agent_display.go
• pkg/connector/agentstore.go
• pkg/connector/builder.go
• pkg/connector/chat.go
• pkg/connector/chat_fork_test.go
• pkg/connector/client.go
• pkg/connector/client_capabilities_test.go
• pkg/connector/command_aliases.go
• pkg/connector/command_registry.go
• pkg/connector/commands.go
• pkg/connector/commands_helpers.go
• pkg/connector/commands_manage.go
• pkg/connector/commands_parity.go
• pkg/connector/connector.go
• pkg/connector/defaults_alignment_test.go
• pkg/connector/desktop_api_helpers.go
• pkg/connector/error_logging.go
• pkg/connector/events.go
• pkg/connector/group_activation.go
• pkg/connector/handleai.go
• pkg/connector/handlematrix.go
• pkg/connector/heartbeat_context.go
• pkg/connector/heartbeat_delivery.go
• pkg/connector/heartbeat_execute.go
• pkg/connector/history_limit_test.go
• pkg/connector/identifiers.go
• pkg/connector/inbound_prompt_runtime_test.go
• pkg/connector/integration_host.go
• pkg/connector/integrations.go
• pkg/connector/integrations_config.go
• pkg/connector/integrations_example-config.yaml
• pkg/connector/mcp_helpers.go

💤 Files with no reviewable changes (10)

• pkg/connector/agent_display.go
• pkg/connector/commands_manage.go
• pkg/connector/integrations.go
• pkg/connector/commands_helpers.go
• pkg/connector/heartbeat_context.go
• pkg/connector/history_limit_test.go
• pkg/connector/command_aliases.go
• pkg/connector/handlematrix.go
• pkg/connector/builder.go
• pkg/connector/commands_parity.go

🚧 Files skipped from review as they are similar to previous changes (8)

• pkg/connector/command_registry.go
• pkg/connector/integrations_example-config.yaml
• docs/matrix-ai-matrix-spec-v1.md
• pkg/connector/agentstore.go
• pkg/connector/handleai.go
• pkg/connector/client_capabilities_test.go
• pkg/connector/inbound_prompt_runtime_test.go
• pkg/connector/connector.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Forked chats now drop runtime room overrides.

cloneForkPortalMetadata only preserves ResolvedTarget, so forking an agent room with a non-default model or reasoning override silently reverts the fork to the agent defaults. That changes the effective runtime before the first message.

🐛 Proposed fix

 func cloneForkPortalMetadata(src *PortalMetadata, slug, title string) *PortalMetadata {
 	if src == nil {
 		return nil
 	}
 	clone := &PortalMetadata{
+		RuntimeModelOverride: src.RuntimeModelOverride,
+		RuntimeReasoning:     src.RuntimeReasoning,
 		Slug:  slug,
 		Title: title,
 	}
+	if len(src.DisabledTools) > 0 {
+		clone.DisabledTools = append([]string(nil), src.DisabledTools...)
+	}
 	if src.ResolvedTarget != nil {
 		target := *src.ResolvedTarget
 		clone.ResolvedTarget = &target
 	}
 	return clone
 }

📝 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

	clone := &PortalMetadata{
	Slug: slug,
	Title: title,
	}
	if src.ResolvedTarget != nil {
	target := *src.ResolvedTarget
	clone.ResolvedTarget = &target
	}
	clone := &PortalMetadata{
	RuntimeModelOverride: src.RuntimeModelOverride,
	RuntimeReasoning: src.RuntimeReasoning,
	Slug: slug,
	Title: title,
	}
	if len(src.DisabledTools) > 0 {
	clone.DisabledTools = append([]string(nil), src.DisabledTools...)
	}
	if src.ResolvedTarget != nil {
	target := *src.ResolvedTarget
	clone.ResolvedTarget = &target
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/chat.go` around lines 652 - 659, cloneForkPortalMetadata
currently only copies ResolvedTarget into the new PortalMetadata and thus drops
any runtime room override; update the cloning logic in cloneForkPortalMetadata
to also copy the room/runtime override fields from src into clone (e.g.,
clone.RuntimeOverride = src.RuntimeOverride or perform a deep copy if it's a
pointer/struct), ensuring you check for nil before dereferencing and preserve
non-default override values so the forked portal keeps the same
runtime/model/reasoning overrides as the source.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C4 'func portalMeta\(|func isSimpleMode\(|ResolvedTarget\s*=.*resolveTargetFromGhostID|resolveTargetFromGhostID\(portal\.OtherUserID\)' pkg/connector

Repository: beeper/agentremote

Length of output: 3856

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get context around lines 698-700 in chat.go
echo "=== Lines 680-730 of chat.go ==="
sed -n '680,730p' pkg/connector/chat.go | cat -n

Repository: beeper/agentremote

Length of output: 1840

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Searching for isSimpleMode() calls ==="
rg -n 'isSimpleMode\(' pkg/connector --context=3

echo ""
echo "=== Searching for effectiveModel() definition and calls ==="
rg -n 'func effectiveModel|effectiveModel\(' pkg/connector --context=2 | head -80

Repository: beeper/agentremote

Length of output: 12745

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Direct portal.Metadata access (potential issue) ==="
rg -n 'portal\.Metadata' pkg/connector --context=2 | head -100

echo ""
echo "=== portalMeta() calls (hydration) ==="
rg -n 'portalMeta\(' pkg/connector | head -30

Repository: beeper/agentremote

Length of output: 3628

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== integration_host.go context around line 800 ==="
sed -n '790,810p' pkg/connector/integration_host.go | cat -n

echo ""
echo "=== composeChatInfo() function ==="
rg -n 'func.*composeChatInfo' pkg/connector --context=30

Repository: beeper/agentremote

Length of output: 4292

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== cloneForkPortalMetadata() function ==="
rg -n 'func cloneForkPortalMetadata' pkg/connector --context=20

echo ""
echo "=== effectiveModel() function definition ==="
rg -n 'func \(.*\) effectiveModel' pkg/connector --context=5

Repository: beeper/agentremote

Length of output: 4039

---

Fresh model chats will lose simple-mode behavior due to missing ResolvedTarget hydration.

The newly initialized metadata in initPortalForChat() (lines 698-700) contains only Slug and Title. Although portal.OtherUserID is set on line 706, the metadata itself is never passed through portalMeta() to populate ResolvedTarget. Additionally, composeChatInfo() calls effectiveModel(nil) (line 930), bypassing the metadata entirely. As a result:

• isSimpleMode(meta) returns false (metadata's ResolvedTarget is nil)
• effectiveModel() falls back to the provider default instead of the model ID

This only affects fresh chats; forked chats work correctly because cloneForkPortalMetadata() explicitly preserves the source's ResolvedTarget.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/chat.go` around lines 698 - 700, initPortalForChat is creating
minimal metadata (only Slug/Title) so ResolvedTarget never gets hydrated; update
initPortalForChat to run the constructed metadata through portalMeta (or
otherwise call the same hydration logic used by cloneForkPortalMetadata) so
ResolvedTarget is populated, and change composeChatInfo to pass the actual meta
into effectiveModel instead of calling effectiveModel(nil) so isSimpleMode and
model resolution use the hydrated metadata (refer to symbols: initPortalForChat,
portalMeta, ResolvedTarget, composeChatInfo, effectiveModel, isSimpleMode,
cloneForkPortalMetadata).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

!ai new drops the selected model for agent rooms.

resolveAgentModelForNewChat can return the current room override or a validated fallback, but passing false here means the new portal won't retain that choice in RuntimeModelOverride. The new chat resolves back to the agent primary model instead of matching the source room.

🐛 Proposed fix

-		oc.createAndOpenAgentChat(runCtx, portal, agent, modelID, false)
+		useOverride := strings.TrimSpace(modelID) != strings.TrimSpace(oc.agentDefaultModel(agent))
+		oc.createAndOpenAgentChat(runCtx, portal, agent, modelID, useOverride)

Please apply the same predicate to the explicit agent <agent_id> branch above as well.

📝 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

	oc.createAndOpenAgentChat(runCtx, portal, agent, modelID, false)
	useOverride := strings.TrimSpace(modelID) != strings.TrimSpace(oc.agentDefaultModel(agent))
	oc.createAndOpenAgentChat(runCtx, portal, agent, modelID, useOverride)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/chat.go` at line 782, The call to
oc.createAndOpenAgentChat(runCtx, portal, agent, modelID, false) drops the
resolved model override because false disables RuntimeModelOverride; change it
to pass the same predicate/boolean used by resolveAgentModelForNewChat so the
new portal retains the chosen model (i.e., compute a keepOverride boolean from
resolveAgentModelForNewChat or reuse the existing predicate and pass it instead
of false), and apply the identical change to the explicit "agent <agent_id>"
branch above so both branches preserve the RuntimeModelOverride consistently.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Joining URL parts with spaces may produce invalid URLs.

If multiple arguments are passed after the token (e.g., ["name", "token", "https://example.com", "extra"]), joining with spaces creates "https://example.com extra" which is not a valid URL. Consider whether this is the intended behavior or if only trimmed[2] should be used as the baseURL.

🔧 Alternative: Use only the third argument as baseURL

-	return normalizeDesktopInstanceName(trimmed[0]), trimmed[1], strings.TrimSpace(strings.Join(trimmed[2:], " ")), nil
+	return normalizeDesktopInstanceName(trimmed[0]), trimmed[1], trimmed[2], nil

📝 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

	return normalizeDesktopInstanceName(trimmed[0]), trimmed[1], strings.TrimSpace(strings.Join(trimmed[2:], " ")), nil
	return normalizeDesktopInstanceName(trimmed[0]), trimmed[1], trimmed[2], nil

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/connector/desktop_api_helpers.go` at line 35, The current return builds
baseURL by strings.Join(trimmed[2:], " "), which can produce invalid URLs when
extra args exist; change the logic in the function that returns
normalizeDesktopInstanceName(trimmed[0]), trimmed[1], ... so that the baseURL is
taken from trimmed[2] only (or validate/parse trimmed[2] with net/url.Parse and
handle errors) instead of joining trimmed[2:] with spaces; update any callers
expecting joined parts accordingly and reference the trimmed slice and
normalizeDesktopInstanceName to locate the change.