Conversation
…ad code removal (23 commits)
- Migrate api.rs from daemon_log to tracing (log module deleted by ct03) - Add workspace_base field to HydrationWorkspaceOps struct - Make is_brain_member accessible outside #[cfg(test)] - Add missing log_level arg to start_daemon call in meeting.rs - Fix unicode escape in sessions_api.rs test assertion - Fix .clone/ typo to .claude/ in hydration.rs doc comment
…on errors Implement the missing deactivate_session method on SessionManager using the ephemeral session model's finalization infrastructure. The method inspects dirty state, pushes unpushed repos with rebase-retry (D-07), and transitions the session to Completed. Push failures are non-fatal. Key changes: - Add push_with_rebase_retry to session::finalization::deactivation (replaces the removed crate::workspace version) - Add DeactivationResult return type for deactivate_session - Fix 10 missing fields in test constructions (AssemblyConfig and SessionRecord) All 1,401 tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hase-d ref The e2e preflight auth check (`preflight_gh_auth`) checks GH_TOKEN env var first, falling through to `gh auth status` when unset. In workspace contexts with GH_CONFIG_DIR pointing at a bot's expired hosts.yml, the fallback always fails. Setting GH_TOKEN=$TESTS_GH_TOKEN bypasses this. Also fixes a pre-existing bug from fix-154: the `all:` recipe was updated to reference `phase-d-session` but `bridge-and-workspace:` was missed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds a console sessions view and API, shifts daemon/member workflows to session-based startup and shutdown, expands workspace hydration and credential handling, and updates finalization, logging, and exploratory test coverage to match the new session model. ChangesSessions and lifecycle changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant WebRouter
participant SessionsApiState
participant DaemonRun
Browser->>WebRouter: GET /api/teams/{team}/sessions
WebRouter->>SessionsApiState: list_for_console()
SessionsApiState-->>WebRouter: session summaries
DaemonRun->>SessionsApiState: start_session / stop_session / refresh credentials
WebRouter-->>Browser: JSON response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
This was referenced Jun 11, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/bm/src/daemon/config.rs (1)
72-110: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider consolidating logs directory creation.
log_dir(),log(), andmember_log()all independently create thelogs/directory usingfs::create_dir_all. The newlog_dir()helper could be reused in the other two methods to reduce duplication.♻️ Possible consolidation
pub fn log(&self) -> Result<PathBuf> { - let logs_dir = self.config_dir.join("logs"); - fs::create_dir_all(&logs_dir)?; + let logs_dir = self.log_dir()?; Ok(logs_dir.join(format!("daemon-{}.log", self.team_name))) } pub fn member_log(&self, member_name: &str) -> Result<PathBuf> { - let logs_dir = self.config_dir.join("logs"); - fs::create_dir_all(&logs_dir)?; + let logs_dir = self.log_dir()?; Ok(logs_dir.join(format!( "member-{}-{}.log", self.team_name, member_name ))) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/bm/src/daemon/config.rs` around lines 72 - 110, Replace the repeated creation of the logs directory in log() and member_log() by reusing the existing log_dir() helper: call self.log_dir()? (or map/and_then as appropriate) to obtain the logs_dir and then join the file name (format!("daemon-{}.log", self.team_name) in log and format!("member-{}-{}.log", self.team_name, member_name) in member_log), removing the duplicate fs::create_dir_all calls; keep log_dir() as the single place that ensures the directory exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/bm/src/chat/mod.rs`:
- Around line 409-416: The function inject_app_credentials_from_shared_dir
currently only sets GH_CONFIG_DIR; update it to also call
refresh_token_from_keyring(&gh_dir) after confirming hosts.yml exists and before
setting GH_CONFIG_DIR, and remove potential conflicting environment variables by
calling std::env::remove_var("GH_TOKEN") and
std::env::remove_var("GITHUB_TOKEN") so the gh CLI uses the refreshed config;
keep the same boolean return semantics. Ensure you reference the existing helper
refresh_token_from_keyring and perform the env cleanup in
inject_app_credentials_from_shared_dir so behavior matches the legacy credential
injection path.
- Around line 615-629: prepare_meeting_session_from_path currently constructs an
AgentSession with credential_dir: None which diverges from
prepare_chat_session_from_path's daemon-ephemeral behavior and can cause missing
or wrong GitHub credentials; change prepare_meeting_session_from_path to accept
the same team_name and member parameters used by prepare_chat_session_from_path
and set AgentSession.credential_dir to Some(shared_credential_dir_for(team_name,
member)) (mirroring the logic used by prepare_chat_session_from_path), and then
update its call site(s) (e.g., in crates/bm/src/commands/meeting.rs) to pass the
new team_name and member arguments.
In `@crates/bm/src/commands/chat.rs`:
- Line 33: The hardcoded log level string "info" should be replaced with a
configurable value exposed via a CLI flag (e.g., --log-level) or config option:
add a log_level field to the existing CLI options struct (the command parsing
code used by the chat command and the daemon start path), wire that flag into
the logger initialization call instead of the literal "info" in chat.rs, and do
the same for the other start path (start.rs) so both use the provided value.
Locate where "info" is passed (the chat command handler and the daemon start
function) and update those logger initialization sites to read the parsed
opt.log_level (or config.log_level) and validate/normalize the value before
applying it. Ensure the flag is documented in the CLI help text.
In `@crates/bm/src/commands/daemon.rs`:
- Line 14: The handler currently accepts log_level: &str with no checks; update
the code that receives this parameter (the function/command handling log_level)
to validate it against the allowed set {"trace","debug","info","warn","error"}
and either return a clear error (or map invalid values to a default) rather than
proceeding silently, and add a short doc comment on the command/function
describing the accepted values; ensure the validation occurs early (e.g., at the
start of the daemon command handler) and uses the exact symbol log_level for
clarity in the change.
- Line 93: The run_daemon function currently accepts log_level without
validation; add the same validation (or docstring/enforced enum) used by the
start function to ensure accepted log level values are consistent—either call
the existing log level validator used by start (or centralize into a new
validate_log_level function) inside run_daemon (referencing run_daemon and
start) and return an Err on invalid values or update the function docs to list
allowed values.
In `@crates/bm/src/daemon/config.rs`:
- Around line 137-141: In save_poll_state, the code currently discards
fs::write's Result and always emits tracing::trace!("Poll state saved"), which
can mislead when the write fails; change the logic to check the Result from
fs::write and only emit the "Poll state saved" trace on Ok, and emit a
tracing::error/tracing::warn with the error details on Err; also handle the Err
branch of serde_json::to_string_pretty by logging the serialization error
(include function name save_poll_state, type PollState and the fs::write Result
in the messages) so failures are correctly reported instead of silently ignored.
In `@crates/bm/src/web/sessions.rs`:
- Around line 71-134: Add a new tokio::test similar to
list_sessions_returns_summaries_with_required_fields that constructs an
initialized but empty SessionsApiState via
SessionsApiState::new(tmp.path().join("registry.json")), wraps it with
make_test_web_state(Some(sessions_state_clone)), mounts web_router and issues a
GET to "/api/teams/test-team/sessions", then assert the response is 200 and the
JSON body deserializes to an empty array (len == 0); reuse the same tmp/config
setup and request-building pattern from
list_sessions_returns_summaries_with_required_fields to ensure
list_for_console() handles an initialized-empty registry without panicking.
- Around line 11-20: The handler list_sessions currently ignores the AxumPath
parameter AxumPath(_team_name) and always uses State(state).sessions_state;
update list_sessions to validate the requested team name against the daemon’s
configured team (or Sessions registry owner) and return StatusCode::NOT_FOUND
when they differ, similar to list_members/team_overview behavior, or explicitly
remove the unused route param and add a TODO documenting the semantics;
reference the list_sessions function and WebState.sessions_state to locate and
implement the check and 404 behavior.
In `@crates/bm/src/workspace/hydration.rs`:
- Around line 2041-2052: The comment above the assertion incorrectly claims
hydrate_workspace() hardcodes project_names: vec![] in AssemblyConfig; locate
the comment near the test that asserts .claude/agents/pr-review.md exists and
update or remove it: either delete the "FAILS: ..." paragraph or change it to
state that hydrate_workspace() now passes self.project_names.clone() into
AssemblyConfig (so project-level agents are assembled), referencing
hydrate_workspace(), AssemblyConfig, and HydrationWorkspaceOps to ensure the
comment matches current behavior.
In `@crates/bm/tests/e2e/scenarios/session_lifecycle_journey.rs`:
- Around line 877-883: The call to wait_for_new_session_workspace currently uses
Duration::from_secs(10) which is shorter than other session-start waits and can
cause flakiness; update that call so it uses Duration::from_secs(20) instead
(i.e., replace the Duration::from_secs(10) argument passed to
wait_for_new_session_workspace near the new_ws declaration) to match the other
timeouts and improve meeting session stability.
In `@crates/bm/tests/exploratory/Justfile`:
- Around line 182-201: The script hardcodes pnpm paths (PNPM_DIR, use of
/global/5/.pnpm/ to compute ACP_PKG, ACP_INDEX and NP1/NP2/NP3) which is
brittle; update the regeneration in the ssh heredoc to discover the global pnpm
root dynamically (e.g. call pnpm root -g or pnpm store path) and derive
ACP_INDEX/NP* from that result, and add explicit existence checks for ACP_PKG,
ACP_INDEX and each NP* before writing the claude-agent-acp wrapper so the script
fails with a clear message rather than silently using wrong paths.
In `@crates/bm/tests/exploratory/phases/phase-b.sh`:
- Around line 56-83: MEMBER_NAME and MEMBER_ROLE extraction using grep|awk (in
the loop that reads MANIFEST/botminter.yml) is brittle for quoted, multi-word,
or commented YAML values; replace the ad-hoc parsing with a proper YAML query
(use yq to extract .name and .role from "$MANIFEST") and add a fallback to the
existing default values (echo "$MEMBER" and "engineer") if yq is missing or the
keys are not present; ensure you reference MEMBER_NAME and MEMBER_ROLE
assignments and the MANIFEST variable so the sed templating still receives safe,
fully-unquoted values.
In `@crates/bm/tests/exploratory/phases/phase-c.sh`:
- Line 255: The shell redirection on the `bm_hire` invocation is reversed so
stderr still goes to the terminal; change the redirect order on the `bm_hire
engineer --name pre-existing` command so stdout is sent to /dev/null first and
then stderr is redirected to stdout (i.e., use >/dev/null 2>&1) to suppress both
streams.
In `@crates/bm/tests/exploratory/phases/phase-f.sh`:
- Line 13: Quote the unquoted shell variable $TEAM in the bm bridge start
invocations to satisfy shellcheck and avoid word-splitting: locate the command
instances where OUT=$(PATH=/usr/bin:/bin bm bridge start -t $TEAM 2>&1) (and the
similar call later) and change the -t argument to use "$TEAM" (i.e., -t "$TEAM")
in both places so the variable is safely quoted.
In `@crates/bm/tests/exploratory/phases/phase-h.sh`:
- Line 140: The shell redirection order is wrong for the bm bridge start command
occurrences: change occurrences of "bm bridge start -t $TEAM_NAME 2>&1
>/dev/null" to "bm bridge start -t \"$TEAM_NAME\" >/dev/null 2>&1" so stdout is
sent to /dev/null before stderr is redirected to it, and quote $TEAM_NAME to
satisfy shellcheck SC2086; apply the same replacement for the other occurrences
of the same command in this script.
- Around line 16-17: Remove the dead variable definitions BOB_WS and STATE_FILE
that are flagged by ShellCheck (SC2034) because the script uses
session-workspace discovery instead of static paths; locate the declarations of
BOB_WS and STATE_FILE in phase-h.sh and delete those lines so there are no
unused variable assignments left in the script.
---
Outside diff comments:
In `@crates/bm/src/daemon/config.rs`:
- Around line 72-110: Replace the repeated creation of the logs directory in
log() and member_log() by reusing the existing log_dir() helper: call
self.log_dir()? (or map/and_then as appropriate) to obtain the logs_dir and then
join the file name (format!("daemon-{}.log", self.team_name) in log and
format!("member-{}-{}.log", self.team_name, member_name) in member_log),
removing the duplicate fs::create_dir_all calls; keep log_dir() as the single
place that ensures the directory exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 318d1882-1f11-4483-98ba-b99a71fb9ecc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (74)
Justfileconsole/e2e/console-pages.spec.tsconsole/e2e/fixtures.tsconsole/src/lib/api.tsconsole/src/lib/components/Sidebar.svelteconsole/src/lib/types.tsconsole/src/routes/teams/[team]/sessions/+page.sveltecrates/bm/Cargo.tomlcrates/bm/src/acp/client.rscrates/bm/src/acp/types.rscrates/bm/src/agent_main.rscrates/bm/src/brain/bridge_adapter.rscrates/bm/src/chat/mod.rscrates/bm/src/cli.rscrates/bm/src/commands/brain_run.rscrates/bm/src/commands/chat.rscrates/bm/src/commands/daemon.rscrates/bm/src/commands/meeting.rscrates/bm/src/commands/start.rscrates/bm/src/daemon/api.rscrates/bm/src/daemon/client.rscrates/bm/src/daemon/config.rscrates/bm/src/daemon/event.rscrates/bm/src/daemon/lifecycle.rscrates/bm/src/daemon/log.rscrates/bm/src/daemon/mod.rscrates/bm/src/daemon/process.rscrates/bm/src/daemon/run.rscrates/bm/src/daemon/sessions_api.rscrates/bm/src/formation/launch.rscrates/bm/src/formation/local/linux/mod.rscrates/bm/src/formation/mod.rscrates/bm/src/formation/start_members.rscrates/bm/src/main.rscrates/bm/src/session/cleanup.rscrates/bm/src/session/dirty_state.rscrates/bm/src/session/finalization/deactivation.rscrates/bm/src/session/finalization/subagent.rscrates/bm/src/session/history.rscrates/bm/src/session/manager.rscrates/bm/src/session/registry.rscrates/bm/src/session/retention.rscrates/bm/src/session/stop.rscrates/bm/src/session/types.rscrates/bm/src/web/files.rscrates/bm/src/web/members.rscrates/bm/src/web/mod.rscrates/bm/src/web/overview.rscrates/bm/src/web/process.rscrates/bm/src/web/sessions.rscrates/bm/src/web/state.rscrates/bm/src/web/sync.rscrates/bm/src/web/teams.rscrates/bm/src/workspace/hydration.rscrates/bm/src/workspace/mod.rscrates/bm/src/workspace/util.rscrates/bm/tests/e2e/scenarios/session_lifecycle_journey.rscrates/bm/tests/exploratory/Justfilecrates/bm/tests/exploratory/PLAN.mdcrates/bm/tests/exploratory/REPORT.mdcrates/bm/tests/exploratory/phases/phase-acp-isolated.shcrates/bm/tests/exploratory/phases/phase-b.shcrates/bm/tests/exploratory/phases/phase-c.shcrates/bm/tests/exploratory/phases/phase-d-session.shcrates/bm/tests/exploratory/phases/phase-d.shcrates/bm/tests/exploratory/phases/phase-e.shcrates/bm/tests/exploratory/phases/phase-f.shcrates/bm/tests/exploratory/phases/phase-g.shcrates/bm/tests/exploratory/phases/phase-h.shdocs/content/concepts/session-model.mdinvariants/exploratory-test-scope.mdprofiles/agentic-sdlc-minimal/bridges/tuwunel/Justfileprofiles/agentic-sdlc-planning/bridges/tuwunel/Justfileprofiles/scrum/bridges/tuwunel/Justfile
💤 Files with no reviewable changes (4)
- crates/bm/src/daemon/log.rs
- crates/bm/tests/exploratory/phases/phase-d.sh
- crates/bm/tests/exploratory/phases/phase-acp-isolated.sh
- crates/bm/src/workspace/util.rs
Comment on lines
+409
to
+416
| pub(crate) fn inject_app_credentials_from_shared_dir(credential_dir: &Path) -> bool { | ||
| let gh_dir = credential_dir.join("gh"); | ||
| if !gh_dir.join("hosts.yml").exists() { | ||
| return false; | ||
| } | ||
| std::env::set_var("GH_CONFIG_DIR", &gh_dir); | ||
| true | ||
| } |
There was a problem hiding this comment.
Missing token refresh and environment variable cleanup in D-02 shared credential path.
The inject_app_credentials_from_shared_dir function only sets GH_CONFIG_DIR but does not:
- Call
refresh_token_from_keyring(...)to ensure the token inhosts.ymlis current - Remove
GH_TOKENandGITHUB_TOKENto prevent auth conflicts
The legacy workspace path (lines 387-390) performs both operations. Without token refresh, ephemeral sessions could fail with expired installation tokens. Without removing conflicting env vars, the gh CLI might use the wrong credential source.
🔒 Proposed fix to match legacy credential injection behavior
-pub(crate) fn inject_app_credentials_from_shared_dir(credential_dir: &Path) -> bool {
+pub(crate) fn inject_app_credentials_from_shared_dir(
+ credential_dir: &Path,
+ ws_path: &Path,
+ team_name: &str,
+ member_name: &str,
+) -> bool {
let gh_dir = credential_dir.join("gh");
if !gh_dir.join("hosts.yml").exists() {
return false;
}
+ refresh_token_from_keyring(ws_path, team_name, member_name);
std::env::set_var("GH_CONFIG_DIR", &gh_dir);
+ std::env::remove_var("GH_TOKEN");
+ std::env::remove_var("GITHUB_TOKEN");
true
}Then update the call site at line 380:
if let Some(cred_dir) = credential_dir {
- return inject_app_credentials_from_shared_dir(cred_dir);
+ return inject_app_credentials_from_shared_dir(cred_dir, ws_path, team_name, member_name);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/src/chat/mod.rs` around lines 409 - 416, The function
inject_app_credentials_from_shared_dir currently only sets GH_CONFIG_DIR; update
it to also call refresh_token_from_keyring(&gh_dir) after confirming hosts.yml
exists and before setting GH_CONFIG_DIR, and remove potential conflicting
environment variables by calling std::env::remove_var("GH_TOKEN") and
std::env::remove_var("GITHUB_TOKEN") so the gh CLI uses the refreshed config;
keep the same boolean return semantics. Ensure you reference the existing helper
refresh_token_from_keyring and perform the env cleanup in
inject_app_credentials_from_shared_dir so behavior matches the legacy credential
injection path.
Comment on lines
+615
to
629
| /// Prepares a meeting session from an ephemeral workspace path provided by the daemon. | ||
| /// The meeting's `instructions` field IS the system prompt — no meta-prompt assembly. | ||
| pub fn prepare_meeting_session_from_path( | ||
| workspace_path: &Path, | ||
| instructions: &str, | ||
| ) -> Result<AgentSession> { | ||
| if instructions.trim().is_empty() { | ||
| bail!("Meeting instructions must not be empty"); | ||
| } | ||
| let ws_path = team_path.join(member); | ||
| if !ws_path.join(".botminter.workspace").exists() { | ||
| bail!( | ||
| "No workspace found for member '{}'. Run `bm teams sync` first.", | ||
| member | ||
| ); | ||
| } | ||
| Ok(AgentSession { | ||
| meta_prompt: instructions.to_string(), | ||
| ws_path, | ||
| ws_path: workspace_path.to_path_buf(), | ||
| credential_dir: None, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Meeting sessions should use shared credential directory for consistency with chat sessions.
prepare_meeting_session_from_path sets credential_dir: None (line 627), but meeting sessions are ephemeral and daemon-backed (per the function comment on line 615). Chat ephemeral sessions created by prepare_chat_session_from_path set credential_dir: Some(...) (line 152).
This inconsistency could cause:
- Meetings to fail with missing GitHub credentials when the legacy workspace
.config/ghdoesn't exist - Meetings to use personal auth instead of App credentials, breaking permission boundaries
Since meetings are ephemeral sessions provisioned by the daemon, they should follow the same D-02 shared credential pattern as chat sessions.
🔧 Proposed fix to align meeting credential handling
Update the function signature to accept team_name and member:
pub fn prepare_meeting_session_from_path(
workspace_path: &Path,
+ team_name: &str,
+ member: &str,
instructions: &str,
) -> Result<AgentSession> {
if instructions.trim().is_empty() {
bail!("Meeting instructions must not be empty");
}
+ let daemon_paths = crate::daemon::DaemonPaths::new(team_name)?;
+ let credential_dir = daemon_paths
+ .sessions_base()
+ .join("credentials")
+ .join(member);
+
Ok(AgentSession {
meta_prompt: instructions.to_string(),
ws_path: workspace_path.to_path_buf(),
- credential_dir: None,
+ credential_dir: Some(credential_dir),
})
}Then update the call site in crates/bm/src/commands/meeting.rs line 145.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/src/chat/mod.rs` around lines 615 - 629,
prepare_meeting_session_from_path currently constructs an AgentSession with
credential_dir: None which diverges from prepare_chat_session_from_path's
daemon-ephemeral behavior and can cause missing or wrong GitHub credentials;
change prepare_meeting_session_from_path to accept the same team_name and member
parameters used by prepare_chat_session_from_path and set
AgentSession.credential_dir to Some(shared_credential_dir_for(team_name,
member)) (mirroring the logic used by prepare_chat_session_from_path), and then
update its call site(s) (e.g., in crates/bm/src/commands/meeting.rs) to pass the
new team_name and member arguments.
| 0, | ||
| team.daemon.interval, | ||
| "127.0.0.1", | ||
| "info", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider making the log level configurable.
The log level is hardcoded to "info" here and in other daemon start paths (e.g., start.rs). Consider exposing this as a CLI flag (e.g., --log-level) or configuration option to give users control over daemon verbosity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/src/commands/chat.rs` at line 33, The hardcoded log level string
"info" should be replaced with a configurable value exposed via a CLI flag
(e.g., --log-level) or config option: add a log_level field to the existing CLI
options struct (the command parsing code used by the chat command and the daemon
start path), wire that flag into the logger initialization call instead of the
literal "info" in chat.rs, and do the same for the other start path (start.rs)
so both use the provided value. Locate where "info" is passed (the chat command
handler and the daemon start function) and update those logger initialization
sites to read the parsed opt.log_level (or config.log_level) and
validate/normalize the value before applying it. Ensure the flag is documented
in the CLI help text.
| port: u16, | ||
| interval: u64, | ||
| bind: &str, | ||
| log_level: &str, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider validating the log_level parameter.
The log_level parameter is accepted as a raw &str with no validation. Consider validating against known log levels (e.g., "trace", "debug", "info", "warn", "error") or documenting the accepted values in a doc comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/src/commands/daemon.rs` at line 14, The handler currently accepts
log_level: &str with no checks; update the code that receives this parameter
(the function/command handling log_level) to validate it against the allowed set
{"trace","debug","info","warn","error"} and either return a clear error (or map
invalid values to a default) rather than proceeding silently, and add a short
doc comment on the command/function describing the accepted values; ensure the
validation occurs early (e.g., at the start of the daemon command handler) and
uses the exact symbol log_level for clarity in the change.
| /// Handles the hidden `bm daemon-run` command. | ||
| pub fn run_daemon(team: &str, mode: &str, port: u16, interval: u64, bind: &str) -> Result<()> { | ||
| daemon::run_daemon(team, mode, port, interval, bind) | ||
| pub fn run_daemon(team: &str, mode: &str, port: u16, interval: u64, bind: &str, log_level: &str) -> Result<()> { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Same validation suggestion applies here.
Consider validating or documenting accepted log_level values for consistency with the start function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/src/commands/daemon.rs` at line 93, The run_daemon function
currently accepts log_level without validation; add the same validation (or
docstring/enforced enum) used by the start function to ensure accepted log level
values are consistent—either call the existing log level validator used by start
(or centralize into a new validate_log_level function) inside run_daemon
(referencing run_daemon and start) and return an Err on invalid values or update
the function docs to list allowed values.
Comment on lines
+56
to
+83
| # B9b: Deploy brain-prompt.md into the team repo member dirs (ephemeral model). | ||
| # ConfigAssembler.assemble() (CT-154-14) copies brain-prompt.md from | ||
| # team_repo/members/<member>/brain-prompt.md into the ephemeral session workspace. | ||
| # Do NOT create permanent workspace directories (~/.botminter/workspaces/<team>/<member>/). | ||
| BRAIN_TEMPLATE="$TEAM_REPO/brain/system-prompt.md" | ||
| if [ -f "$BRAIN_TEMPLATE" ]; then | ||
| SENTINEL_FAIL=false | ||
| for MEMBER in engineer-alice engineer-bob; do | ||
| MEMBER_DIR="$TEAM_REPO/members/$MEMBER" | ||
| MANIFEST="$MEMBER_DIR/botminter.yml" | ||
| MEMBER_NAME=$(grep '^name:' "$MANIFEST" 2>/dev/null | awk '{print $2}' | tr -d '"' || echo "$MEMBER") | ||
| MEMBER_ROLE=$(grep '^role:' "$MANIFEST" 2>/dev/null | awk '{print $2}' | tr -d '"' || echo "engineer") | ||
| sed \ | ||
| -e "s|{{member_name}}|${MEMBER_NAME}|g" \ | ||
| -e "s|{{team_name}}|${TEAM}|g" \ | ||
| -e "s|{{role}}|${MEMBER_ROLE}|g" \ | ||
| -e "s|{{gh_org}}|${ORG}|g" \ | ||
| -e "s|{{gh_repo}}|${REPO}|g" \ | ||
| "$BRAIN_TEMPLATE" > "$MEMBER_DIR/brain-prompt.md" || SENTINEL_FAIL=true | ||
| done | ||
| if $SENTINEL_FAIL; then | ||
| fail "B9b" "Brain deploy" "sed rendering failed for one or more members" | ||
| else | ||
| pass "B9b" "Deployed brain-prompt.md to team repo (alice and bob, per-member rendered)" | ||
| fi | ||
| else | ||
| fail "B9b" "Brain template" "brain/system-prompt.md not in team repo at $TEAM_REPO" | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Fragile YAML parsing for member name and role extraction.
The brain-prompt deployment at L66-67 uses grep | awk '{print $2}' | tr -d '"' to extract values from botminter.yml:
MEMBER_NAME=$(grep '^name:' "$MANIFEST" 2>/dev/null | awk '{print $2}' | tr -d '"' || echo "$MEMBER")This assumes:
- The value is always the second whitespace-separated field
- Removing
"is sufficient to unquote
This breaks if the YAML contains:
- Multi-word values:
name: "Alice Smith"→ extracts"Alice(missingSmith") - Inline comments:
name: alice # engineer→ extractsalice(breaks on#) - Flow-style:
name: 'alice'→ leaves single quotes
♻️ Proposed fix using yq for robust YAML parsing
- MEMBER_NAME=$(grep '^name:' "$MANIFEST" 2>/dev/null | awk '{print $2}' | tr -d '"' || echo "$MEMBER")
- MEMBER_ROLE=$(grep '^role:' "$MANIFEST" 2>/dev/null | awk '{print $2}' | tr -d '"' || echo "engineer")
+ MEMBER_NAME=$(yq eval '.name' "$MANIFEST" 2>/dev/null || echo "$MEMBER")
+ MEMBER_ROLE=$(yq eval '.role' "$MANIFEST" 2>/dev/null || echo "engineer")If yq is not available in the test environment, add a check in the preflight phase or document the grep/awk limitation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/tests/exploratory/phases/phase-b.sh` around lines 56 - 83,
MEMBER_NAME and MEMBER_ROLE extraction using grep|awk (in the loop that reads
MANIFEST/botminter.yml) is brittle for quoted, multi-word, or commented YAML
values; replace the ad-hoc parsing with a proper YAML query (use yq to extract
.name and .role from "$MANIFEST") and add a fallback to the existing default
values (echo "$MEMBER" and "engineer") if yq is missing or the keys are not
present; ensure you reference MEMBER_NAME and MEMBER_ROLE assignments and the
MANIFEST variable so the sed templating still receives safe, fully-unquoted
values.
| # Hire pre-existing as a member so sync will provision them | ||
| bm_hire superman --name pre-existing 2>&1 >/dev/null || true | ||
| # Hire pre-existing as a member so bridge identity add can provision them | ||
| bm_hire engineer --name pre-existing 2>&1 >/dev/null || true |
There was a problem hiding this comment.
Incorrect redirect order: 2>&1 before stdout redirect has no effect.
At L255, the command bm_hire engineer --name pre-existing 2>&1 >/dev/null redirects stderr to stdout (2>&1), then redirects stdout to /dev/null. This means:
- stderr → current stdout (likely the terminal)
- stdout → /dev/null
The intent is likely to suppress both stdout and stderr. The correct order is >/dev/null 2>&1:
🐛 Proposed fix
- bm_hire engineer --name pre-existing 2>&1 >/dev/null || true
+ bm_hire engineer --name pre-existing >/dev/null 2>&1 || true📝 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
| bm_hire engineer --name pre-existing 2>&1 >/dev/null || true | |
| bm_hire engineer --name pre-existing >/dev/null 2>&1 || true |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 255-255: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/tests/exploratory/phases/phase-c.sh` at line 255, The shell
redirection on the `bm_hire` invocation is reversed so stderr still goes to the
terminal; change the redirect order on the `bm_hire engineer --name
pre-existing` command so stdout is sent to /dev/null first and then stderr is
redirected to stdout (i.e., use >/dev/null 2>&1) to suppress both streams.
Source: Linters/SAST tools
| if echo "$OUT" | grep -qi "just\|skip\|not found"; then | ||
| pass "F1" "Graceful handling when just not in PATH" | ||
| # F1: Without just — bm bridge start should handle missing just gracefully | ||
| OUT=$(PATH=/usr/bin:/bin bm bridge start -t $TEAM 2>&1) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Quote $TEAM variable for shellcheck compliance.
Shellcheck flags unquoted $TEAM at lines 13 and 30. While low-risk in this controlled test environment, quoting the variable follows best practice and silences the linter.
♻️ Shellcheck-compliant quoting
-OUT=$(PATH=/usr/bin:/bin bm bridge start -t $TEAM 2>&1)
+OUT=$(PATH=/usr/bin:/bin bm bridge start -t "$TEAM" 2>&1)-OUT=$(bm members list -t $TEAM 2>&1)
+OUT=$(bm members list -t "$TEAM" 2>&1)Also applies to: 30-30
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 13-13: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/tests/exploratory/phases/phase-f.sh` at line 13, Quote the unquoted
shell variable $TEAM in the bm bridge start invocations to satisfy shellcheck
and avoid word-splitting: locate the command instances where
OUT=$(PATH=/usr/bin:/bin bm bridge start -t $TEAM 2>&1) (and the similar call
later) and change the -t argument to use "$TEAM" (i.e., -t "$TEAM") in both
places so the variable is safely quoted.
Source: Linters/SAST tools
Comment on lines
+16
to
17
| BOB_WS="$TEAM_DIR/engineer-bob" | ||
| STATE_FILE="$HOME/.botminter/state.json" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove unused variables BOB_WS and STATE_FILE.
Shellcheck flags these variables as unused (SC2034). The script now uses session-workspace discovery instead of static paths, so these definitions are dead code.
♻️ Cleanup unused definitions
ALICE_WS="$TEAM_DIR/engineer-alice"
-BOB_WS="$TEAM_DIR/engineer-bob"
-STATE_FILE="$HOME/.botminter/state.json"📝 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
| BOB_WS="$TEAM_DIR/engineer-bob" | |
| STATE_FILE="$HOME/.botminter/state.json" | |
| ALICE_WS="$TEAM_DIR/engineer-alice" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 16-16: BOB_WS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 17-17: STATE_FILE appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/tests/exploratory/phases/phase-h.sh` around lines 16 - 17, Remove
the dead variable definitions BOB_WS and STATE_FILE that are flagged by
ShellCheck (SC2034) because the script uses session-workspace discovery instead
of static paths; locate the declarations of BOB_WS and STATE_FILE in phase-h.sh
and delete those lines so there are no unused variable assignments left in the
script.
Source: Linters/SAST tools
| else | ||
| # Try to bring it up | ||
| bm teams sync --bridge -v 2>&1 >/dev/null | ||
| bm bridge start -t $TEAM_NAME 2>&1 >/dev/null |
There was a problem hiding this comment.
Fix redirection order: 2>&1 must come after output redirect.
At lines 140, 592, and 789, the pattern 2>&1 >/dev/null redirects stderr to stdout's original destination (the terminal) before redirecting stdout to /dev/null, leaving stderr visible. The correct order is >/dev/null 2>&1 to discard both streams.
🐛 Corrected redirection order
- bm bridge start -t $TEAM_NAME 2>&1 >/dev/null
+ bm bridge start -t "$TEAM_NAME" >/dev/null 2>&1Apply the same fix at lines 592 and 789. Also quote $TEAM_NAME to satisfy shellcheck SC2086.
Also applies to: 592-592, 789-789
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 140-140: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 140-140: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bm/tests/exploratory/phases/phase-h.sh` at line 140, The shell
redirection order is wrong for the bm bridge start command occurrences: change
occurrences of "bm bridge start -t $TEAM_NAME 2>&1 >/dev/null" to "bm bridge
start -t \"$TEAM_NAME\" >/dev/null 2>&1" so stdout is sent to /dev/null before
stderr is redirected to it, and quote $TEAM_NAME to satisfy shellcheck SC2086;
apply the same replacement for the other occurrences of the same command in this
script.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reconciles two divergent branches from the Epic #85 (Ephemeral Workspaces) implementation:
The branches were squashed individually, then ct03 was rebased onto fix-154. Post-rebase compilation errors were resolved:
deactivate_sessiononSessionManagerusing the finalization infrastructurepush_with_rebase_retryandDEFAULT_MAX_RETRIEStosession::finalization::deactivationfinalization_agent_pid,project_names,workspace_base)Additional fixes:
GH_TOKEN=$TESTS_GH_TOKENin all e2e recipes sopreflight_gh_auth()uses the test token instead of falling through to the bot's expiredhosts.ymlphase-d→phase-d-sessionreference inbridge-and-workspacerecipe (pre-existing bug from fix-154)Test Results
Supersedes #37 and #38.
Ref: #154, #85
Test plan
just test— all unit, conformance, and E2E tests passjust exploratory-test— 153/154 pass (G8 is a pre-existing cleanup race)Summary by CodeRabbit
New Features
Improvements
Bug Fixes