fix(agent): thread agent_id into build_session_agent_inner by sanil-23 · Pull Request #584 · tinyhumansai/openhuman

sanil-23 · 2026-04-15T20:47:14Z

Summary

Fixes agent_definition_name silently falling back to "main" for every Agent built via Agent::from_config_for_agent, because build_session_agent_inner dropped its agent_id: &str parameter via a let _ = agent_id; suppression instead of threading it into the Agent::builder() chain.
Latently activated by [Bug] onboarding flow keeps using main agent instead of welcome/orchestrator #525 / [Bug] agent prompt tool scopes leak full GitHub tool catalog #526 (PR feat(agent): welcome->orchestrator routing + per-agent tool scoping (#525, #526) #544): before that, only orchestrator reached this path and "main" was already its historical alias, so the bug was invisible. After feat(agent): welcome->orchestrator routing + per-agent tool scoping (#525, #526) #544 landed real dispatch routing that builds welcome via from_config_for_agent("welcome", ...), welcome sessions became the first production caller to pass a non-orchestrator id through this code, and the bug became observable on disk.
User-visible impact on welcome sessions: wrong transcript filename (sessions/DDMMYYYY/main_*.md instead of welcome_*.md), wrong metadata header (agent: main), and — most seriously — transcript resume cross-contamination where find_latest_transcript("main") loads the most recent orchestrator session's history into the welcome agent's history as a resume prefix before run_single.
Prompt content is NOT affected (context/prompt.rs only branches on ctx.agent_id for one is_skill_executor check at prompt.rs:655). skills_agent and the other typed sub-agents are NOT affected either (they go through subagent_runner, which bypasses this builder).
Two regression unit tests pin the builder contract (setter path + legacy fallback); both green.
Verified live end-to-end with yarn tauri dev against a fresh workspace: pre-fix writes sessions/16042026/main_0.md with agent: main for a welcome session; post-fix writes welcome_0.md with agent: welcome.

Problem

Agent::from_config_for_agent(config, agent_id) resolves the target AgentDefinition from the registry and hands off to build_session_agent_inner(config, agent_id, target_def). That inner function takes agent_id: &str as a parameter but never threads it into the Agent::builder() chain. A let _ = agent_id; was silencing the unused-variable warning, and an 8-line comment block at the call site rationalised the omission by claiming "event_channel is for transport identity, not the agent-definition id" — conflating two unrelated fields (event_channel and agent_definition_name).

When AgentBuilder::build() runs, the missing field falls back to the legacy default at builder.rs:310-312:

agent_definition_name: self.agent_definition_name.unwrap_or_else(|| "main".to_string()),

Every session built via from_config_for_agent came out stamped "main" at runtime, regardless of which id the caller asked for.

Scope — which ids actually reach this path

A grep of all callers of Agent::from_config_for_agent and Agent::from_config across the repo returns exactly two agent ids in production:

Caller	Agent id
`agent/triage/escalation.rs:130`	`"orchestrator"` (via `Agent::from_config` legacy wrapper)
`cron/scheduler.rs:197`	`"orchestrator"` (via `Agent::from_config`)
`local_ai/ops.rs:41`	`"orchestrator"` (via `Agent::from_config`)
`channels/providers/web.rs:826`	`"orchestrator"` OR `"welcome"` (via `pick_target_agent_id` — the #525/#526 routing)
`agent/welcome_proactive.rs:94`	`"welcome"` (hardcoded, fired from `set_onboarding_completed(true)`)

No caller passes "skills_agent", "researcher", "planner", "code_executor", "critic", "archivist", "tool_maker", "trigger_triage", "trigger_reactor", "morning_briefing", or "fork". Those agent ids are spawned exclusively via agent/harness/subagent_runner.rs::run_subagent, which constructs its own system prompt via render_subagent_system_prompt and writes transcripts under the explicit id it is handed. The pre-existing sessions/15042026/skills_agent_0.md on disk with agent: skills_agent stamped in its metadata header confirms the subagent_runner path has always been correct.

Why this was latent

The legacy .unwrap_or_else(|| "main".to_string()) default was designed for a world where the only top-level agent was the orchestrator, and the orchestrator was called "main" in logs and file paths. You can see that assumption baked into context/debug_dump.rs:249:

if options.agent_id == "main" || options.agent_id == "orchestrator_main" {

Every downstream consumer already treats "main" as an orchestrator alias, which made the bug benign for the orchestrator path — the field's value was wrong but the downstream effect was zero.

#525 / #526 (PR #544) added real dispatch routing that builds the welcome agent via from_config_for_agent("welcome", ...) pre-onboarding. That was the first production caller to pass a non-orchestrator id through this code path, and it latently activated the bug.

User-visible impact — three surfaces, all silently wrong pre-fix for welcome

1. Session transcript filename

transcript::resolve_new_transcript_path(workspace_dir, agent_name) at transcript.rs:166 uses agent_name as the {agent} prefix in sessions/DDMMYYYY/{agent}_{index}.md:

Pre-fix: welcome session → sessions/16042026/main_0.md
Post-fix: welcome session → sessions/16042026/welcome_0.md

2. Transcript metadata header

transcript::write_transcript at transcript.rs:94 stamps the agent: line inside the  block from meta.agent_name, populated from self.agent_definition_name in turn.rs:1140:

<!-- session_transcript
agent: main           ← pre-fix; should be `welcome`
dispatcher: native
...
-->

3. Transcript resume cross-contamination — the nastiest one

Agent::try_load_session_transcript at turn.rs:1061 calls transcript::find_latest_transcript(workspace_dir, self.agent_definition_name) on every session start. That function scans all dated session dirs for {agent_name}_*.md and returns the most recent one.

Pre-fix, a welcome session stamped "main" asks for the latest main_*.md across all dated dirs and finds the most recent orchestrator session's transcript (which is also filed under main_*.md because of the historical alias). It loads its 5 messages — system prompt, user message, assistant tool-call history, tool result — into the welcome agent's history as a resume prefix before the welcome agent's run_single call. The welcome agent then runs its inference on top of that cross-contaminated history.

This was reproduced live on 2026-04-16 against G:/projects/vezures/.openhuman:

01:37:55 [welcome::proactive] starting proactive welcome
01:37:55 [agent::builder] building session agent id=welcome    (builder.rs:423)
01:37:55 [agent_loop] turn start message_chars=1030 history_len=0
01:37:55 [transcript] found previous transcript path=…\15042026\main_0.md  ← BUG
01:37:55 [transcript] loaded 5 messages from …\15042026\main_0.md          ← BUG
01:37:55 [transcript] loaded 5 messages for resume (cache_boundary=9464)
01:38:00 [transcript] resumed from cached transcript prefix_len=5 new_tail=1
01:38:06 [transcript] new session transcript path=…\16042026\main_0.md      ← BUG
01:38:06 [transcript] wrote 6 messages

Inspecting the written sessions/16042026/main_0.md afterwards showed a mix of yesterday's orchestrator email thread (send an email to alan@mahadao.com say hi from openhuman → spawn_subagent(agent_id="skills_agent", toolkit="gmail", ...) → tool result Email sent successfully!) with today's welcome agent's response appended on top. The welcome agent's LLM call had been fed an unrelated orchestrator session as context.

What is NOT affected

Prompt rendering. A grep across the entire context::prompt module finds exactly one reader of ctx.agent_id:

// prompt.rs:655
let is_skill_executor = ctx.agent_id == "skills_agent";

That is the only branch. Welcome / main / orchestrator all fall through the same delegator path regardless of ctx.agent_id. The welcome agent's persona prompt comes from a SystemPromptBuilder::for_subagent(target_def.system_prompt) constructed at session-build time and stamped into the Agent as .prompt_builder(prompt_builder) — that builder renders welcome-correct bytes independently of agent_definition_name. Verified on disk: the clean pre-fix main_0.md opens with # Welcome Agent\n\nYou are the **Welcome** agent..., confirming the system prompt body was welcome-flavored even though the filename and metadata were wrong.

Solution

One functional line added in build_session_agent_inner:

.agent_definition_name(agent_id.to_string())

Plus supporting scaffolding:

Delete the let _ = agent_id; suppression.
Replace the misleading 8-line comment block at the call site (which conflated event_channel with agent_definition_name) with an accurate description of the three downstream surfaces and the concrete pre-fix manifestation.
Expand the docstring on AgentBuilder::agent_definition_name to document all three surfaces and note the latent prompt-section foot-gun the fix closes for future code (any future prompt section that branches on a non-skills_agent id would silently never fire if this field stayed at "main").
Add pub fn agent_definition_name(&self) -> &str accessor on Agent in runtime.rs, for runtime introspection and test assertions.
Add new log::debug! call at the stamping site so the fix is greppable in runtime logs: [agent::builder] stamping agent_definition_name=<id> onto session agent.
Add two regression unit tests in session/tests.rs:
- agent_builder_threads_agent_definition_name_when_set — parameterised over ["welcome", "skills_agent", "orchestrator", "trigger_triage"], asserts .agent_definition_name(id).build() produces an Agent whose accessor returns that id verbatim. welcome and orchestrator are the two ids that reach from_config_for_agent in production today; skills_agent and trigger_triage are defensive coverage in case a future commit adds a new top-level caller.
- agent_builder_falls_back_to_main_when_definition_name_unset — builds via a minimal helper without the setter, asserts the legacy "main" fallback still fires (so direct builder users in tests / CLI still work).

Both tests pass: 2 passed; 0 failed; 0 ignored; 3477 filtered out; finished in 0.21s.

The fix does NOT touch:

subagent_runner.rs or any sub-agent spawn path
Any agent/agents/*/agent.toml definition
Any spawn_subagent / dispatch_subagent tool implementation
Any prompt-rendering code in context::prompt
The AgentBuilder::build() fallback itself (preserved for direct-builder callers)

Submission Checklist

Unit tests — two new regression tests in src/openhuman/agent/harness/session/tests.rs pinning the builder contract. Both green.
E2E / integration — verified end-to-end with yarn tauri dev against OPENHUMAN_WORKSPACE=G:/projects/vezures/.openhuman:
- Pre-fix run (stashed fix, rebuilt sidecar from the upstream baseline): welcome_proactive wrote sessions/16042026/main_0.md with agent: main in the metadata. On an earlier attempt with existing main_*.md transcripts on disk, the welcome session also demonstrated the resume cross-contamination path, loading 5 messages from yesterday's orchestrator transcript as the welcome agent's prefix.
- Post-fix run: welcome_proactive wrote sessions/16042026/welcome_0.md with agent: welcome. The subsequent web-chat turn routed through pick_target_agent_id to orchestrator, which wrote sessions/16042026/orchestrator_0.md with agent: orchestrator. The new debug log [agent::builder] stamping agent_definition_name=<id> onto session agent fired on both turns.
Doc comments — docstring on AgentBuilder::agent_definition_name rewritten; accessor docstring on Agent::agent_definition_name added; test docstrings describe the bug surfaces accurately; call-site comment block replaced with a correct narrative.
Inline comments — the new log::debug! call at the stamping site gives the fix a grep-friendly runtime trace.

Impact

Runtime/platform: desktop only. No mobile or web-only code paths touched.
Behavioral change:
- Orchestrator sessions moved from historical main_*.md to orchestrator_*.md filenames. Behaviorally identical everywhere downstream because "main" was already an orchestrator alias.
- Welcome sessions move from main_*.md to welcome_*.md filenames with agent: welcome metadata, and no longer cross-contaminate on resume.
- Agent::try_load_session_transcript looks up by the new correct agent_definition_name. A freshly-patched orchestrator session will not discover existing main_*.md files on resume — it starts cold on the first turn after the upgrade. Welcome's cold start is the correct behavior.
- Net effect: one cold-start for orchestrator per install after upgrade; welcome sessions start clean on fresh installs and on first run after upgrade.
Performance: none. One extra String allocation per session construction — negligible on a path that only runs at session start.
Security: closes a small information-leak foot-gun (welcome transcripts were misattributed as main in forensics) and closes the cross-contamination path, which was loading orchestrator tool-call history (tool arguments and results) into welcome agent LLM context unintentionally.
Migration: none. Config schema unchanged, no database migrations.
Backward compat: the legacy "main" fallback in AgentBuilder::build() is preserved; only build_session_agent_inner now overrides it. Direct builder users (tests, CLI, custom agents) that never call the setter still get the "main" default.

Issues: none linked. The bug was latently activated by [Bug] onboarding flow keeps using main agent instead of welcome/orchestrator #525 / [Bug] agent prompt tool scopes leak full GitHub tool catalog #526 (merged in PR feat(agent): welcome->orchestrator routing + per-agent tool scoping (#525, #526) #544).
Follow-up PR(s) / TODOs:
- welcome_proactive publishes its generated message to socket.io as event=proactive_message room=system thread_id=proactive:welcome, but the React app has no subscriber for that event — it is logged via an onAny catch-all in socketService.ts:182 and dropped. The welcome agent's message is never rendered anywhere user-facing. Out of scope for this PR; should be filed as a separate frontend issue.
- Potential refactor: collapse the "main" alias for orchestrator by updating debug_dump.rs:249, the Agent::from_config legacy wrapper, and any other "main" | "orchestrator" disjunctions to use "orchestrator" directly. Would eliminate the historical alias and the foot-gun it creates. Out of scope for this PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Agent definition names are now properly propagated through the system, ensuring consistent transcript filenames and accurate metadata headers.
- Agent identifiers are now correctly reflected in session transcripts and prompt context.
Tests
- Added regression tests to verify agent definition name handling and default fallback behavior.

build_session_agent_inner took an agent_id: &str parameter but never threaded it into the Agent::builder() chain — a `let _ = agent_id;` silenced the unused-variable warning. AgentBuilder::build() fell back to the legacy "main" default at builder.rs:310-312, so every session built via Agent::from_config_for_agent carried agent_definition_name="main" regardless of the id the caller asked for. Only two ids reach this path in production: "orchestrator" (legacy Agent::from_config wrapper, cron, local_ai, escalation) and "welcome" (channels/providers/web.rs routing after tinyhumansai#525/tinyhumansai#526, and welcome_proactive). Orchestrator is benign — "main" was already an alias for orchestrator everywhere downstream (see debug_dump.rs:249). Welcome is the visible bug — agent_definition_name feeds three surfaces that are all silently wrong pre-fix: 1. Transcript filename — welcome sessions land as main_*.md instead of welcome_*.md. 2. Transcript metadata — the  block stamps `agent: main` instead of `agent: welcome`. 3. Transcript resume cross-contamination — try_load_session_transcript calls find_latest_transcript(workspace_dir, "main") which scans all dated session dirs for the latest main_*.md. It finds the last orchestrator session and resumes from it, loading its system prompt + user messages + assistant tool-call history into the welcome agent's `history` as a resume prefix before run_single runs. The written transcript mixes yesterday's orchestrator email thread with today's welcome message under the same main_0.md filename. Reproduced live 2026-04-16: a welcome_proactive session loaded 5 messages from yesterday's 15042026/main_0.md and wrote 6 messages back that included a spawn_subagent(skills_agent, toolkit=gmail) tool-call the welcome agent never made. Prompt rendering is NOT affected. context/prompt.rs only reads ctx.agent_id for the is_skill_executor branch at prompt.rs:655, so welcome/main/orchestrator all fall through the same delegator path. The welcome persona prompt is rendered by the stamped SystemPromptBuilder::for_subagent(target_def.system_prompt) built at session-build time, independent of agent_definition_name. The pre-fix main_0.md on disk contains `# Welcome Agent\n\nYou are the Welcome agent...` as its system prompt body — correct content, wrong filename and metadata. skills_agent and other typed sub-agents are unaffected — they go through subagent_runner, which constructs its prompt directly and writes transcripts under the explicit id it receives. The pre-existing sessions/15042026/skills_agent_0.md on disk with `agent: skills_agent` confirms this code path has always been correct. Changes: * src/openhuman/agent/harness/session/builder.rs: - Add .agent_definition_name(agent_id.to_string()) to the builder chain in build_session_agent_inner. - Delete the `let _ = agent_id;` suppression. - Replace the misleading 8-line comment block at the call site (which claimed event_channel was for transport identity and therefore stamping agent_id wasn't needed — conflating two unrelated fields) with an accurate description of the three load-bearing surfaces. - Expand the docstring on AgentBuilder::agent_definition_name to document the surfaces and the latent prompt-section foot-gun the fix closes for future code. - New log::debug! call at the stamping site for grep-friendly runtime traces ([agent::builder] stamping agent_definition_name=<id> onto session agent). * src/openhuman/agent/harness/session/runtime.rs: - Add pub fn agent_definition_name(&self) -> &str accessor on Agent so tests and runtime callers can introspect the stamped id without reaching into the pub(super) field. * src/openhuman/agent/harness/session/tests.rs: - Add build_minimal_agent_with_definition_name helper. - agent_builder_threads_agent_definition_name_when_set — parameterised over welcome/skills_agent/orchestrator/ trigger_triage, asserts the setter threads the id through. - agent_builder_falls_back_to_main_when_definition_name_unset — pins the legacy fallback contract direct builder users rely on. Tests: 2 passed; 0 failed; 3477 filtered out; finished in 0.21s. cargo check --lib clean; cargo fmt clean. Verified live against G:/projects/vezures/.openhuman on 2026-04-16: - Pre-fix welcome_proactive run wrote sessions/16042026/main_0.md with `agent: main` in the metadata header. - Post-fix welcome_proactive run wrote sessions/16042026/welcome_0.md with `agent: welcome` in the metadata header. - Post-fix web-chat dispatch to orchestrator wrote sessions/16042026/orchestrator_0.md (moved off the historical main_*.md alias — behaviorally unchanged for orchestrator). - The new [agent::builder] stamping debug line fires on both welcome and orchestrator paths. Does not touch: subagent_runner, spawn_subagent / dispatch_subagent, any agent/agents/*/agent.toml, any context::prompt code, or the AgentBuilder::build() fallback itself. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-04-15T20:47:30Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c67cc47d-d60a-4670-a6ff-7829a753408b

📥 Commits

Reviewing files that changed from the base of the PR and between 098206c and 7687eb2.

📒 Files selected for processing (3)

src/openhuman/agent/harness/session/builder.rs
src/openhuman/agent/harness/session/runtime.rs
src/openhuman/agent/harness/session/tests.rs

📝 Walkthrough

Walkthrough

The changes enhance agent identity management by updating AgentBuilder semantics to properly propagate the resolved agent definition ID through the constructed Agent. A new public accessor method enables retrieval of this ID, while regression tests verify correct behavior with and without explicit definition names.

Changes

Cohort / File(s)	Summary
Agent Definition ID Propagation `src/openhuman/agent/harness/session/builder.rs`, `src/openhuman/agent/harness/session/runtime.rs`	Updated `AgentBuilder::agent_definition_name` documentation to clarify it represents the agent definition ID (e.g., `welcome`, `orchestrator`). Modified `Agent::build_session_agent_inner` to stamp the resolved `agent_id` into the builder chain. Added public accessor `Agent::agent_definition_name(&self) -> &str` to retrieve the session's definition ID.
Test Coverage `src/openhuman/agent/harness/session/tests.rs`	Added `build_minimal_agent_with_definition_name()` test helper for constructing in-memory agents. Introduced regression tests verifying that definition names are correctly threaded through agent construction and that unset names default to `"main"`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 An agent now knows its true name,
From builder's forge to runtime's flame,
Welcome, orchestrator, skills so grand,
Identity stamped through every land!
thump thump ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: threading agent_id into build_session_agent_inner to ensure agent_definition_name is properly set rather than defaulting to 'main'.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

senamakel approved these changes Apr 15, 2026

View reviewed changes

senamakel merged commit 19aa50a into tinyhumansai:main Apr 15, 2026
8 checks passed

coderabbitai Bot mentioned this pull request Apr 16, 2026

feat(threads): dedicated threads controller with per-thread session scoping #590

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(agent): thread agent_id into build_session_agent_inner#584

fix(agent): thread agent_id into build_session_agent_inner#584
senamakel merged 1 commit intotinyhumansai:mainfrom
sanil-23:fix/agent-definition-name-threading

sanil-23 commented Apr 15, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 15, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sanil-23 commented Apr 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Scope — which ids actually reach this path

Why this was latent

User-visible impact — three surfaces, all silently wrong pre-fix for welcome

What is NOT affected

Solution

Submission Checklist

Impact

Related

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sanil-23 commented Apr 15, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 15, 2026 •

edited

Loading