【WIP】 feat(hooks): add container lifecycle hook system with host-exec support - #1160
Open
SuGuoXiong wants to merge 4 commits into
Open
【WIP】 feat(hooks): add container lifecycle hook system with host-exec support#1160SuGuoXiong wants to merge 4 commits into
SuGuoXiong wants to merge 4 commits into
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
|
|
1 similar comment
|
|
📝 WalkthroughWalkthroughAdds a synchronous container lifecycle hook system with declarative and trait hooks, host and guest execution, context substitution, retries, fire counts, lifecycle integration, CLI configuration, tests, and architecture documentation. ChangesContainer lifecycle hooks
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
🤖 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 `@docs/architecture/container-lifecycle-hooks.md`:
- Around line 5-17: Revise the lifecycle-hooks documentation to describe only
the currently implemented create, start, stop, and exec host-hook phases. Remove
GuestExec, snapshot, and restore from the supported behavior and move those
deferred capabilities into a future-work section.
- Line 1: Update the PR description to include end-to-end call graphs showing
the container lifecycle hook flow before and after the change described by
“Container Lifecycle Hook System.” Do not add a “Fixes #<n>” reference, since
this is a feature change.
In `@src/boxlite/src/hooks/fire_count.rs`:
- Around line 24-28: Update FireCount persistence around load and its increment
path so every (box_id, hook_name) count increment is written to the box-state
storage, and restore persisted counts when the BoxImpl runner is created. Ensure
cache invalidation or runtime reconstruction reloads the stored values instead
of resetting fire_count, while preserving load’s existing key-based behavior.
In `@src/boxlite/src/hooks/host_exec.rs`:
- Around line 47-74: Implement staged timeout termination across the hook
execution flow: in src/boxlite/src/hooks/host_exec.rs lines 47-74, update the
host process setup and timeout handling to terminate the appropriate process
group with SIGTERM, wait 5 seconds, then use SIGKILL if needed after execution
has started; in src/boxlite/src/hooks/runner.rs lines 324-365, retain the
cancellation handle or execution state returned by exec and invoke that
termination path on timeout rather than only returning an error; in
docs/architecture/container-lifecycle-hooks.md lines 232-234, retain the
staged-kill documentation only once implemented; and in
docs/architecture/container-lifecycle-hooks-alpha-tests.md lines 452-459, extend
the timeout test to verify process/group termination and orphan cleanup.
In `@src/boxlite/src/hooks/mod.rs`:
- Around line 157-165: Update hook deserialization around the on_error field and
HookPoint::default_on_error() so an omitted on_error policy remains
distinguishable from an explicitly configured policy. Resolve omitted policies
using the current HookPoint default, preserving explicit HookErrorPolicy values
and the documented Fail defaults for pre-start, pre-exec, pre-snapshot, and
pre-restore.
In `@src/boxlite/src/hooks/runner.rs`:
- Around line 64-102: Update the hook execution loop to resolve metadata through
the `HookOrTrait` value rather than accessing `hook.name` or `hook.on_error`
directly. Add helpers for the display name and effective error policy, use the
display-name helper for spans, context, and logging, and use the hook-point
policy for trait hooks; replace the undefined `item` match with the loop
variable.
- Around line 287-305: Update the GuestExec command-building flow around
BoxCommand::new to apply the same context substitution used by HostExec: run
substitution::substitute_args on args and substitution::substitute_env on env
before passing them to BoxCommand::arg and BoxCommand::env. Preserve the
existing user, working_dir, and ctx.to_env_vars handling.
- Around line 11-13: Separate the declarative Hook type from the in-process hook
trait by renaming the config struct to HookConfig and giving the trait in
runner.rs a distinct name, then update all references and exports accordingly.
Apply this in src/boxlite/src/hooks/runner.rs lines 11-13, 19-25, and 393-451;
update related public usage in src/boxlite/src/lib.rs lines 61-64 and
src/boxlite/src/litebox/box_impl.rs line 187; revise the corresponding
terminology and examples in docs/architecture/container-lifecycle-hooks.md lines
700-741.
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 535-576: Update the PostExec hook context construction inside the
spawned task’s result loop to clone post_exec_box_id, post_exec_container_id,
post_exec_image, and post_exec_command before passing them to
HookContext::for_post_exec. Keep the captured values available for every
iteration of the while loop.
- Around line 310-332: The current PreStart block runs after ensure_booted and
outside the container-start single-flight, allowing repeated execution and
leaving failed starts Running. Move PreStart execution into
ensure_container_started, guarded by the same single-flight that decides whether
init starts, and ensure it runs exactly once before container_start; on hook
failure, clean up the booted VM and restore the retryable pre-start state before
propagating the error.
In `@src/boxlite/src/runtime/options.rs`:
- Around line 435-437: Add hook-name uniqueness validation in the runtime create
path before BoxOptions is persisted, covering SDK and deserialized callers
rather than relying on CLI validation. Use the existing Hook name field and
return the established validation error when duplicate names are detected;
preserve valid hook configurations unchanged.
In `@src/cli/src/cli.rs`:
- Around line 1068-1074: Validate hook actions and conditions against the
selected hook point before persisting BoxOptions: restrict GuestExec to
post-start, pre-stop, and post-restore, and reject HookCondition::ExecResult
unless the hook point is post-exec. Apply the checks in the CLI hook parsing
path covering the GuestExec and condition handling branches, returning a
validation error for incompatible configurations.
- Around line 1198-1208: The retry parsing branch should validate the optional
exhaustion policy instead of defaulting unknown values to OnExhausted::Continue.
Update the retry policy parsing around the retry branch to accept exactly fail
or continue, reject any other policy, and reject inputs containing more than the
allowed three comma-separated fields.
- Around line 987-993: Update the PR description to include both
before-and-after end-to-end call graphs for the CLI-to-runtime container hook
flow, covering hook parsing, modifier association, and runtime execution. Keep
the documentation near the lifecycle hook description consistent with the
implemented symbols and clearly distinguish the existing flow from the changed
flow.
- Around line 1004-1006: Update the CLI argument model and hook-processing logic
around hook_args and the related handling near the hook construction code so
each --hook-arg carries an explicit hook name or otherwise preserves its
association with the intended --hook. Reject arguments that lack an eligible
matching hook, including attempts to modify a final --hook-json hook, instead of
applying all modifiers to the last hook.
- Around line 1110-1121: Update the modifier application logic around
hook_enabled, hook_priority, and hook_timeout to propagate parse errors instead
of falling back to defaults. Validate parsed timeout_secs and return an error
when it is below one second, while preserving valid values and the existing
find_hook_mut lookups.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf61af83-2b99-43fd-8f0b-d103f65a14c8
📒 Files selected for processing (15)
docs/architecture/container-lifecycle-hooks-alpha-tests.mddocs/architecture/container-lifecycle-hooks.mdsrc/boxlite/src/hooks/context.rssrc/boxlite/src/hooks/fire_count.rssrc/boxlite/src/hooks/host_exec.rssrc/boxlite/src/hooks/mod.rssrc/boxlite/src/hooks/runner.rssrc/boxlite/src/hooks/substitution.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/runtime/rt_impl.rssrc/cli/src/cli.rssrc/cli/src/commands/create.rssrc/cli/src/commands/run.rs
| @@ -0,0 +1,1184 @@ | |||
| # Container Lifecycle Hook System | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required call graphs to the PR description.
Add before-and-after end-to-end call graphs to the PR description. The stated change is a feature, so the bug-fix Fixes #<n> requirement does not apply.
As per coding guidelines, every PR description must include before-and-after end-to-end call graphs.
🤖 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 `@docs/architecture/container-lifecycle-hooks.md` at line 1, Update the PR
description to include end-to-end call graphs showing the container lifecycle
hook flow before and after the change described by “Container Lifecycle Hook
System.” Do not add a “Fixes #<n>” reference, since this is a feature change.
Source: Coding guidelines
Comment on lines
+5
to
+17
| Give BoxLite users the ability to inject custom logic at key points in the | ||
| container lifecycle — create, start, stop, exec, snapshot, restore — without | ||
| forking the runtime or wrapping every API call. Hooks run synchronously | ||
| (blocking) with configurable timeouts, ordered by user-defined priority. They | ||
| span the host (commands run on the host OS beside the runtime) and the guest | ||
| (commands run inside the container via exec). | ||
|
|
||
| This is an **embedded library feature** first — the Rust trait, CLI flags, and | ||
| SDK builders are the primary surface. The REST server inherits hooks from box | ||
| options supplied at creation and never invents its own. A hook's execution | ||
| location is determined by its action type: `HostExec` runs on the same machine | ||
| as the boxlite runtime; `GuestExec` runs inside the container via the Execution | ||
| RPC. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit the documented scope to implemented phases.
This section says GuestExec, snapshot, and restore hooks are available. The PR objectives defer these capabilities. Snapshot and restore lifecycle wiring is also not present in the supplied runtime files.
Document only create, start, stop, and exec host-hook support for this phase. Move deferred behavior to a future-work section.
🤖 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 `@docs/architecture/container-lifecycle-hooks.md` around lines 5 - 17, Revise
the lifecycle-hooks documentation to describe only the currently implemented
create, start, stop, and exec host-hook phases. Remove GuestExec, snapshot, and
restore from the supported behavior and move those deferred capabilities into a
future-work section.
Comment on lines
+24
to
+28
| /// Load pre-existing counts from persisted storage (e.g., box state DB). | ||
| pub fn load(&self, box_id: &str, hook_name: &str, persisted_count: u64) { | ||
| let mut counts = self.counts.lock().unwrap(); | ||
| counts.insert((box_id.to_string(), hook_name.to_string()), persisted_count); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist fire counts at the box-state boundary.
load() only copies a value into the in-memory map. No supplied lifecycle path saves increments or reloads them when BoxImpl is reconstructed. As a result, fire_count resets after a runtime restart and can also reset after cache invalidation.
Persist each increment by (box_id, hook_name) and reload the values when the runner is created.
🤖 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 `@src/boxlite/src/hooks/fire_count.rs` around lines 24 - 28, Update FireCount
persistence around load and its increment path so every (box_id, hook_name)
count increment is written to the box-state storage, and restore persisted
counts when the BoxImpl runner is created. Ensure cache invalidation or runtime
reconstruction reloads the stored values instead of resetting fire_count, while
preserving load’s existing key-based behavior.
Comment on lines
+47
to
+74
| let mut child = Command::new(program) | ||
| .args(&args) | ||
| .envs(env.iter().map(|(k, v)| (k.as_str(), v.as_str()))) | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .kill_on_drop(true) | ||
| .spawn() | ||
| .map_err(|e| { | ||
| BoxliteError::Internal(format!( | ||
| "Failed to spawn HostExec hook '{}' program '{program}': {e}", | ||
| hook.name | ||
| )) | ||
| })?; | ||
|
|
||
| // Write context JSON to stdin, then close it | ||
| if let Some(mut stdin) = child.stdin.take() { | ||
| use tokio::io::AsyncWriteExt; | ||
| stdin | ||
| .write_all(ctx_json.as_bytes()) | ||
| .await | ||
| .map_err(|e| BoxliteError::Internal(format!("Failed to write stdin: {e}")))?; | ||
| // stdin is dropped here, closing the pipe | ||
| } | ||
|
|
||
| let timeout_dur = Duration::from_secs(hook.timeout_secs); | ||
|
|
||
| let result = timeout(timeout_dur, child.wait_with_output()).await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n -C 3 'tokio\s*=|name = "tokio"|version = "1\.37' \
Cargo.toml Cargo.lock 2>/dev/null || true
rg -n -C 5 'kill_on_drop|wait_with_output|process_group|start_kill|SIGTERM|SIGKILL' \
src/boxlite/src/hooks/host_exec.rs \
src/boxlite/src/hooks/runner.rsRepository: boxlite-ai/boxlite
Length of output: 6487
🏁 Script executed:
#!/bin/sh
set -eu
sed -n '1,150p' src/boxlite/src/hooks/host_exec.rs
printf '\n--- runner ---\n'
sed -n '280,390p' src/boxlite/src/hooks/runner.rs
printf '\n--- docs ---\n'
sed -n '220,240p' docs/architecture/container-lifecycle-hooks.md
sed -n '445,465p' docs/architecture/container-lifecycle-hooks-alpha-tests.md
printf '\n--- search hook exec APIs ---\n'
rg -n -C 2 'kill_on_drop|process_id|Stdio::from_stdio|child\.' src/boxlite/src/hooks src/box 2>/dev/null || true
printf '\n--- dependency snippets for child/process handling ---\n'
rg -n 'tokio-child|derive_child|process' Cargo.lock Cargo.toml 2>/dev/null || trueRepository: boxlite-ai/boxlite
Length of output: 14464
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- find GuestExec execution interface ---'
rg -n -C 3 'trait .*Execution|impl .*Execution|fn exec\(|exec<|execution\(\)|CancellationToken|start_kill|SIGTERM|SIGKILL' src 2>/dev/null || true
printf '%s\n' '--- BoxCommand definition/usages ---'
fd -i 'box|command' src | sed -n '1,80p'
rg -n -C 4 'struct BoxCommand|impl.*BoxCommand|BoxCommand' src/boxlite/src src 2>/dev/null | sed -n '1,240p'Repository: boxlite-ai/boxlite
Length of output: 50377
Implement forced termination for timed-out hooks.
The timeout paths do not satisfy the documented SIGTERM + grace + SIGKILL behavior.
src/boxlite/src/hooks/host_exec.rs#L47-L114:.kill_on_drop(true)cannot create a process group or implement SIGTERM → 5 s grace → SIGKILL. Ontimeout(...), send the appropriate signal to the child/group if the guest has already started the execution, wait for grace, then escalate.src/boxlite/src/hooks/runner.rs#L312-L360: store the cancellation handle or execution state fromexec(...)and call its SIGTERM/SIGKILL path after timeout instead of only returning an error.docs/architecture/container-lifecycle-hooks.md#L220-L234: keep the staged-kill description only after the behavior is implemented.docs/architecture/container-lifecycle-hooks-alpha-tests.md#L452-L459: make the timeout test validate process/group termination and orphan cleanup, not onlyfire()returning an error.
📍 Affects 4 files
src/boxlite/src/hooks/host_exec.rs#L47-L74(this comment)src/boxlite/src/hooks/runner.rs#L324-L365docs/architecture/container-lifecycle-hooks.md#L232-L234docs/architecture/container-lifecycle-hooks-alpha-tests.md#L452-L459
🤖 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 `@src/boxlite/src/hooks/host_exec.rs` around lines 47 - 74, Implement staged
timeout termination across the hook execution flow: in
src/boxlite/src/hooks/host_exec.rs lines 47-74, update the host process setup
and timeout handling to terminate the appropriate process group with SIGTERM,
wait 5 seconds, then use SIGKILL if needed after execution has started; in
src/boxlite/src/hooks/runner.rs lines 324-365, retain the cancellation handle or
execution state returned by exec and invoke that termination path on timeout
rather than only returning an error; in
docs/architecture/container-lifecycle-hooks.md lines 232-234, retain the
staged-kill documentation only once implemented; and in
docs/architecture/container-lifecycle-hooks-alpha-tests.md lines 452-459, extend
the timeout test to verify process/group termination and orphan cleanup.
Comment on lines
+157
to
+165
| /// Timeout in seconds. The default is 30 s. Must be ≥ 1. | ||
| #[serde(default = "default_timeout")] | ||
| pub timeout_secs: u64, | ||
| /// Only fire when this condition holds. `None` = always fire. | ||
| #[serde(default)] | ||
| pub condition: Option<HookCondition>, | ||
| /// What to do when this hook fails (non-zero exit, timeout, or spawn error). | ||
| #[serde(default)] | ||
| pub on_error: HookErrorPolicy, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply on_error defaults from HookPoint.
#[serde(default)] calls HookErrorPolicy::default(), which is always Continue. A minimal JSON pre-start, pre-exec, pre-snapshot, or pre-restore hook therefore continues after failure instead of using the documented Fail policy.
Store an omitted policy separately, or implement custom deserialization, then resolve it with HookPoint::default_on_error().
🤖 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 `@src/boxlite/src/hooks/mod.rs` around lines 157 - 165, Update hook
deserialization around the on_error field and HookPoint::default_on_error() so
an omitted on_error policy remains distinguishable from an explicitly configured
policy. Resolve omitted policies using the current HookPoint default, preserving
explicit HookErrorPolicy values and the documented Fail defaults for pre-start,
pre-exec, pre-snapshot, and pre-restore.
Comment on lines
+987
to
+993
| /// Container lifecycle hooks. | ||
| /// | ||
| /// Each `--hook` flag defines one hook. The simple syntax is: | ||
| /// `<name>:<point>:<host|guest>:<program>` with args via `--hook-arg`. | ||
| /// | ||
| /// Modifiers like `--hook-timeout` and `--hook-on-error` reference the hook | ||
| /// by name and must appear after the corresponding `--hook` flag. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add end-to-end call graphs to the PR description.
The supplied PR description does not include before-and-after end-to-end call graphs. Add both call graphs for the CLI-to-runtime hook flow.
As per coding guidelines, every PR description must include before-and-after end-to-end call graphs.
🤖 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 `@src/cli/src/cli.rs` around lines 987 - 993, Update the PR description to
include both before-and-after end-to-end call graphs for the CLI-to-runtime
container hook flow, covering hook parsing, modifier association, and runtime
execution. Keep the documentation near the lifecycle hook description consistent
with the implemented symbols and clearly distinguish the existing flow from the
changed flow.
Source: Coding guidelines
Comment on lines
+1004
to
+1006
| /// Add an argument to the most recent --hook. | ||
| #[arg(long = "hook-arg", value_name = "ARG")] | ||
| pub hook_args: Vec<String>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the target hook for each --hook-arg.
Clap collects hooks and hook_args into separate vectors. The original flag order is lost. A command with multiple hooks therefore appends every argument to the final hook. --hook-arg also modifies a final --hook-json hook despite the documented restriction.
Require a hook name in each argument modifier, or use a per-hook syntax that preserves argument association. Reject --hook-arg when no eligible hook exists.
Also applies to: 1097-1107
🤖 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 `@src/cli/src/cli.rs` around lines 1004 - 1006, Update the CLI argument model
and hook-processing logic around hook_args and the related handling near the
hook construction code so each --hook-arg carries an explicit hook name or
otherwise preserves its association with the intended --hook. Reject arguments
that lack an eligible matching hook, including attempts to modify a final
--hook-json hook, instead of applying all modifiers to the last hook.
Comment on lines
+1068
to
+1074
| "guest" => HookAction::GuestExec { | ||
| command: program, | ||
| args: vec![], | ||
| env: vec![], | ||
| user: None, | ||
| working_dir: None, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate hook actions and conditions against the hook point.
The CLI accepts GuestExec at every hook point. The runner skips it when no guest session exists. The hook contract permits guest execution only at post-start, pre-stop, and post-restore. The CLI also accepts HookCondition::ExecResult for non-post-exec hooks even though that condition is PostExec-only.
Reject these incompatible configurations before persisting BoxOptions.
Also applies to: 1126-1130
🤖 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 `@src/cli/src/cli.rs` around lines 1068 - 1074, Validate hook actions and
conditions against the selected hook point before persisting BoxOptions:
restrict GuestExec to post-start, pre-stop, and post-restore, and reject
HookCondition::ExecResult unless the hook point is post-exec. Apply the checks
in the CLI hook parsing path covering the GuestExec and condition handling
branches, returning a validation error for incompatible configurations.
Comment on lines
+1110
to
+1121
| for (name, val) in &self.hook_enabled { | ||
| let hook = find_hook_mut(opts, name)?; | ||
| hook.enabled = val.parse::<bool>().unwrap_or(true); | ||
| } | ||
| for (name, val) in &self.hook_priority { | ||
| let hook = find_hook_mut(opts, name)?; | ||
| hook.priority = val.parse().unwrap_or(0); | ||
| } | ||
| for (name, val) in &self.hook_timeout { | ||
| let hook = find_hook_mut(opts, name)?; | ||
| hook.timeout_secs = val.parse().unwrap_or(30); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid modifier values.
An invalid --hook-enabled value enables the hook. Invalid priority and timeout values silently become 0 and 30. A timeout of 0 also violates the Hook contract and causes an immediate timeout.
Return an error for parse failures and reject timeouts below one second.
Proposed validation
for (name, val) in &self.hook_enabled {
let hook = find_hook_mut(opts, name)?;
- hook.enabled = val.parse::<bool>().unwrap_or(true);
+ hook.enabled = val
+ .parse::<bool>()
+ .map_err(|_| anyhow::anyhow!("Invalid enabled value '{}' for hook '{}'", val, name))?;
}
for (name, val) in &self.hook_priority {
let hook = find_hook_mut(opts, name)?;
- hook.priority = val.parse().unwrap_or(0);
+ hook.priority = val
+ .parse()
+ .map_err(|_| anyhow::anyhow!("Invalid priority '{}' for hook '{}'", val, name))?;
}
for (name, val) in &self.hook_timeout {
let hook = find_hook_mut(opts, name)?;
- hook.timeout_secs = val.parse().unwrap_or(30);
+ let timeout_secs: u64 = val
+ .parse()
+ .map_err(|_| anyhow::anyhow!("Invalid timeout '{}' for hook '{}'", val, name))?;
+ if timeout_secs == 0 {
+ anyhow::bail!("Hook '{}' timeout must be at least one second", name);
+ }
+ hook.timeout_secs = timeout_secs;
}📝 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
| for (name, val) in &self.hook_enabled { | |
| let hook = find_hook_mut(opts, name)?; | |
| hook.enabled = val.parse::<bool>().unwrap_or(true); | |
| } | |
| for (name, val) in &self.hook_priority { | |
| let hook = find_hook_mut(opts, name)?; | |
| hook.priority = val.parse().unwrap_or(0); | |
| } | |
| for (name, val) in &self.hook_timeout { | |
| let hook = find_hook_mut(opts, name)?; | |
| hook.timeout_secs = val.parse().unwrap_or(30); | |
| } | |
| for (name, val) in &self.hook_enabled { | |
| let hook = find_hook_mut(opts, name)?; | |
| hook.enabled = val | |
| .parse::<bool>() | |
| .map_err(|_| anyhow::anyhow!("Invalid enabled value '{}' for hook '{}'", val, name))?; | |
| } | |
| for (name, val) in &self.hook_priority { | |
| let hook = find_hook_mut(opts, name)?; | |
| hook.priority = val | |
| .parse() | |
| .map_err(|_| anyhow::anyhow!("Invalid priority '{}' for hook '{}'", val, name))?; | |
| } | |
| for (name, val) in &self.hook_timeout { | |
| let hook = find_hook_mut(opts, name)?; | |
| let timeout_secs: u64 = val | |
| .parse() | |
| .map_err(|_| anyhow::anyhow!("Invalid timeout '{}' for hook '{}'", val, name))?; | |
| if timeout_secs == 0 { | |
| anyhow::bail!("Hook '{}' timeout must be at least one second", name); | |
| } | |
| hook.timeout_secs = timeout_secs; | |
| } |
🤖 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 `@src/cli/src/cli.rs` around lines 1110 - 1121, Update the modifier application
logic around hook_enabled, hook_priority, and hook_timeout to propagate parse
errors instead of falling back to defaults. Validate parsed timeout_secs and
return an error when it is below one second, while preserving valid values and
the existing find_hook_mut lookups.
Comment on lines
+1198
to
+1208
| _ if s.starts_with("retry:") => { | ||
| let parts: Vec<&str> = s[6..].split(',').collect(); | ||
| if parts.len() < 2 { | ||
| anyhow::bail!("retry policy needs max_retries,backoff_secs: got '{s}'"); | ||
| } | ||
| let max_retries: u32 = parts[0].parse()?; | ||
| let backoff_secs: u64 = parts[1].parse()?; | ||
| let on_exhausted = parts.get(2).map_or(OnExhausted::Continue, |oe| match *oe { | ||
| "fail" => OnExhausted::Fail, | ||
| _ => OnExhausted::Continue, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unknown retry exhaustion policies.
retry:1,2,fali silently becomes OnExhausted::Continue. A typo can therefore continue the lifecycle after a hook failure when the operator intended it to fail. Extra fields are also ignored.
Accept only fail and continue when the optional exhaustion policy is present. Reject other values and extra fields.
🤖 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 `@src/cli/src/cli.rs` around lines 1198 - 1208, The retry parsing branch should
validate the optional exhaustion policy instead of defaulting unknown values to
OnExhausted::Continue. Update the retry policy parsing around the retry branch
to accept exactly fail or continue, reject any other policy, and reject inputs
containing more than the allowed three comma-separated fields.
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.
Overview
Adds a container lifecycle hook system that lets users inject custom logic at
key points in the box lifecycle — create, start, stop, exec — without forking
the runtime or wrapping every API call.
Hook Points
Architecture
What ships
boxlite::hooksHook,HookPoint(7 points),HookActionHookCondition+ExecHookTrigger,HookErrorPolicyHookContext(JSON stdin +$BOXLITE_*env-varHookRunner::fire(),Hooktrait (in-process, notfire()wired intoBoxImpl::{start, stop, exec}andRuntimeImpl::create_box()--hook,--hook-json,--hook-arg+ 8 modifier flags--hook-timeout,--hook-on-error, etc.)Example — auto-snapshot after
pip installDeferred to later phases
Design: [docs/architecture/container-lifecycle-hooks.md](./docs/architecture/c
ontainer-lifecycle-hooks.md)
Test plan: [docs/architecture/container-lifecycle-hooks-alpha-tests.md](./docs
/architecture/container-lifecycle-hooks-alpha-tests.md)
Summary by CodeRabbit