feat(session): configurable daemon wait timeout + retry hint#536
feat(session): configurable daemon wait timeout + retry hint#536RyderFreeman4Logos merged 3 commits intomainfrom
Conversation
Add `daemon_wait_seconds` field to `[session]` config (default 250s). The 250s default is intentional — it returns control to the caller periodically so the caller's KV cache stays warm. Callers should retry on exit 124, not treat it as failure. On timeout (exit 124), emit a structured CSA:SESSION_WAIT_TIMEOUT directive to stderr with session ID, elapsed time, and the retry command. This allows orchestrators to mechanically continue polling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the workspace version to 0.1.200 and makes the daemon wait timeout configurable via the project configuration, defaulting to 250 seconds. It also adds a structured retry hint to stderr when a session wait times out. Review feedback suggests including the --cd argument in the retry hint for accuracy, logging warnings for configuration loading errors instead of silently ignoring them, and centralizing the default timeout constant to improve maintainability.
| eprintln!( | ||
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}\" -->", | ||
| resolved.session_id, elapsed, resolved.session_id, | ||
| ); |
There was a problem hiding this comment.
The structured retry hint does not include the --cd argument if it was provided to the original command. If an orchestrator or agent relies on this hint to re-invoke the command from a different working directory, the retry might fail because it won't be able to locate the session or project root.
| eprintln!( | |
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}\" -->", | |
| resolved.session_id, elapsed, resolved.session_id, | |
| ); | |
| let cd_arg = cd.as_ref().map(|path| format!(" --cd \"{}\"", path)).unwrap_or_default(); | |
| eprintln!( | |
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}{}\" -->", | |
| resolved.session_id, elapsed, resolved.session_id, cd_arg, | |
| ); |
| if let Some(ref root) = project_root | ||
| && let Ok(Some(config)) = csa_config::ProjectConfig::load(root) | ||
| { | ||
| return config.session.daemon_wait_seconds; | ||
| } |
There was a problem hiding this comment.
Errors during project configuration loading are silently ignored here. If the configuration file exists but is malformed (e.g., a syntax error in config.toml), the tool will silently fall back to the default timeout. It is better to log a warning so the user knows why their custom configuration isn't being applied.
if let Some(ref root) = project_root {
match csa_config::ProjectConfig::load(root) {
Ok(Some(config)) => return config.session.daemon_wait_seconds,
Err(e) => tracing::warn!("Failed to load project config: {e}"),
_ => {}
}
}| /// Default daemon wait timeout: 250s for KV cache warmth. | ||
| fn default_daemon_wait_seconds() -> u64 { | ||
| 250 | ||
| } |
There was a problem hiding this comment.
The default timeout value of 250 is hardcoded in multiple places across different crates. It is better to define it as a public constant in this crate so that it can be reused by other crates (like cli-sub-agent), avoiding magic numbers and ensuring consistency.
| /// Default daemon wait timeout: 250s for KV cache warmth. | |
| fn default_daemon_wait_seconds() -> u64 { | |
| 250 | |
| } | |
| /// Default daemon wait timeout: 250s for KV cache warmth. | |
| pub const DEFAULT_DAEMON_WAIT_SECS: u64 = 250; | |
| fn default_daemon_wait_seconds() -> u64 { | |
| DEFAULT_DAEMON_WAIT_SECS | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac37c83e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn resolve_daemon_wait_timeout(cd: Option<&str>) -> u64 { | ||
| let project_root = crate::pipeline::determine_project_root(cd).ok(); | ||
| if let Some(ref root) = project_root | ||
| && let Ok(Some(config)) = csa_config::ProjectConfig::load(root) |
There was a problem hiding this comment.
Report config load failures when resolving wait timeout
This branch treats any ProjectConfig::load error the same as “no config” and silently falls back to DEFAULT_DAEMON_WAIT_SECS. In practice, malformed/unreadable config files (or other load errors) make [session].daemon_wait_seconds appear ignored, so csa session wait keeps timing out at 250s without any diagnostic. Please distinguish load failures from missing config and surface an error (or at least a warning) so timeout behavior is reliable.
Useful? React with 👍 / 👎.
| ); | ||
| // Emit structured retry hint for orchestrators / agents. | ||
| eprintln!( | ||
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}\" -->", |
There was a problem hiding this comment.
Preserve working directory in timeout retry command
The emitted retry hint always uses csa session wait --session <id> and drops the original --cd context. When callers run session wait from outside the project root (using --cd), an orchestrator that executes this hint verbatim will retry in the wrong directory and fail to resolve the session. Include --cd (or an absolute project path) in the directive command so the retry is actually executable.
Useful? React with 👍 / 👎.
- Include --cd argument in CSA:SESSION_WAIT_TIMEOUT retry hint - Log warning on config loading error instead of silent fallback - Centralize DEFAULT_DAEMON_WAIT_SECS as public constant in csa-config Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request bumps the version to 0.1.200 and makes the daemon wait timeout configurable via the project configuration, replacing the previously hardcoded 250-second limit. It also introduces a structured retry hint emitted to stderr when a session wait times out. A review comment identified a potential parsing issue in this retry hint where nested double quotes could break XML/HTML attribute parsers, suggesting the use of single quotes for the path argument instead.
| let cd_arg = cd | ||
| .as_ref() | ||
| .map(|path| format!(" --cd \"{}\"", path)) | ||
| .unwrap_or_default(); | ||
| eprintln!( | ||
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}{}\" -->", | ||
| resolved.session_id, elapsed, resolved.session_id, cd_arg, | ||
| ); |
There was a problem hiding this comment.
The structured retry hint contains nested double quotes in the cmd attribute, which will break parsing for any standard XML/HTML attribute parser. Specifically, the cmd attribute value is delimited by double quotes, and cd_arg also introduces double quotes around the path.
Additionally, using single quotes for the path in the command hint is generally safer and avoids conflict with the outer double quotes of the attribute.
| let cd_arg = cd | |
| .as_ref() | |
| .map(|path| format!(" --cd \"{}\"", path)) | |
| .unwrap_or_default(); | |
| eprintln!( | |
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}{}\" -->", | |
| resolved.session_id, elapsed, resolved.session_id, cd_arg, | |
| ); | |
| // Emit structured retry hint for orchestrators / agents. | |
| let cd_arg = cd | |
| .as_ref() | |
| .map(|path| format!(" --cd '{}'", path)) | |
| .unwrap_or_default(); | |
| eprintln!( | |
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}{}\" -->", | |
| resolved.session_id, elapsed, resolved.session_id, cd_arg, | |
| ); |
…quote parsing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the project version to 0.1.200 and makes the session wait timeout configurable through the project's session settings. It replaces the hardcoded 250-second timeout with a value resolved from the configuration, while maintaining the same default. The update also introduces a structured retry hint printed to stderr upon timeout. A review comment correctly identified that the retry hint does not escape shell-sensitive or XML-sensitive characters in the path, which could result in invalid commands or malformed output.
| let cd_arg = cd | ||
| .as_ref() | ||
| .map(|path| format!(" --cd '{}'", path)) | ||
| .unwrap_or_default(); | ||
| eprintln!( | ||
| "<!-- CSA:SESSION_WAIT_TIMEOUT session={} elapsed={}s cmd=\"csa session wait --session {}{}\" -->", | ||
| resolved.session_id, elapsed, resolved.session_id, cd_arg, | ||
| ); |
There was a problem hiding this comment.
The structured retry hint does not escape the cd path or the overall command string. If the path contains single quotes, the suggested shell command will be broken. Furthermore, if the path contains double quotes or other XML-sensitive characters, it will break the XML-like attribute structure of the directive.
It is recommended to escape single quotes for the shell command and XML-sensitive characters for the attribute value, and to quote all attribute values for better machine readability.
let cd_arg = cd
.as_ref()
.map(|path| format!(" --cd '{}'", path.replace('\'', "'\\''")))
.unwrap_or_default();
let cmd = format!("csa session wait --session {}{}", resolved.session_id, cd_arg);
eprintln!(
"<!-- CSA:SESSION_WAIT_TIMEOUT session=\"{}\" elapsed=\"{}s\" cmd=\"{}\" -->",
resolved.session_id,
elapsed,
cmd.replace('&', "&").replace('<', "<").replace('>', ">").replace('"', """)
);
Audit: Dismissing path escaping finding (Round 3)Finding: Bot suggests full XML entity escaping and shell single-quote escaping for the Assessment: Acceptable as-is for the following reasons:
If CSA later needs to parse these directives with a strict XML parser, the escaping can be added at that point. For now, the single-quote approach is sufficient. 🤖 Arbitrated by Claude Code orchestrator |
Summary
daemon_wait_secondsfield to[session]config (default 250s for KV cache warmth)CSA:SESSION_WAIT_TIMEOUTdirective to stderr on timeout (exit 124) with retry commandChanges
csa-config/config_session.rs: Adddaemon_wait_secondswith serde default 250session_cmds_daemon.rs: Read config value, emit retry hint on timeout onlysession_dispatch.rs: Pass configured timeout tohandle_session_wait()Test plan
csa session waitreturns exit 0 with stdout.log on completed session (unchanged)csa session waitreturns exit 124 with retry hint on timeout[session].daemon_wait_secondsin config overrides default 250🤖 Generated with Claude Code