Add developer experience features and dependency fixes#6
Merged
Conversation
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds optional profile inheritance via an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "puzzlectl (run)"
participant DBus as "D-Bus Daemon"
participant Service as "puzzled Service"
participant Policy as "Policy Engine"
User->>CLI: puzzlectl run --profile=restricted -- python3 agent.py
CLI->>DBus: create_branch(profile, base)
DBus->>Service: create branch
Service->>Service: resolve inheritance & validate profile
Service-->>DBus: branch created
DBus-->>CLI: branch id
CLI->>DBus: activate_branch(branch_id)
DBus->>Service: activate & monitor branch
Service-->>DBus: branch active
DBus-->>CLI: acknowledged
CLI->>CLI: poll branch state (deadline)
CLI->>DBus: diff_branch(branch_id)
DBus->>Service: fetch diff
Service-->>DBus: diff result
DBus-->>CLI: diff
alt auto-commit
CLI->>DBus: commit_branch(branch_id)
else auto-rollback
CLI->>DBus: rollback_branch(branch_id)
else interactive
CLI->>User: prompt Approve?
User-->>CLI: Yes/No
alt approve
CLI->>DBus: commit_branch(branch_id)
DBus->>Policy: evaluate_commit()
Policy-->>DBus: policy_result
DBus-->>CLI: commit response
else reject
CLI->>DBus: rollback_branch(branch_id)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/technical-design.md (1)
615-691:⚠️ Potential issue | 🟡 MinorThe new
extendsdocs are attached to an obsolete profile schema.These sections still describe the old shape (
profile,process.exec_allow, lowercasegated/fail_closed), while the current loader and fixtures usename,exec_allowlist, PascalCase network modes, andFailClosed. Adding inheritance guidance here will send readers toward YAML that does not deserialize.Also applies to: 1675-1715
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/technical-design.md` around lines 615 - 691, The docs for the `extends` inheritance section describe an obsolete schema (fields like `profile`, `process.exec_allow`, lowercase `gated`/`fail_closed`) and must be updated to match the current loader/fixtures which use `name`, `exec_allowlist`, PascalCase network modes and enum values like `FailClosed`; update all occurrences in this section (and the other noted block around lines 1675-1715) to use the current field names and casing, describe vector inheritance semantics for `exec_allowlist`/`read_allow`/`write_deny` exactly as implemented (empty list inherits, non-empty replaces), limit inheritance depth to 3 and mention circular detection, and ensure examples use the current enum/token names (e.g., `Gated` → `Gated` PascalCase and `FailClosed`), plus adjust references to `process.exec_allow` to `exec_allowlist` to avoid deserialization mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/puzzlectl/src/main.rs`:
- Around line 2072-2210: The generator in cmd_profile_init currently accepts
arbitrary network_mode values, writes exec_allowlist/exec_denylist as empty
literal lists, and interpolates raw prompt strings into YAML (causing validator
failures in load_one); fix by validating net_mode against the allowed set
{"Blocked","Gated","Monitored","Unrestricted"} and rejecting/normalizing unknown
inputs, ensure exec_allowlist and exec_denylist are populated with sensible
defaults (or at least one placeholder entry) so load_one won't reject the
profile, and stop hand-concatenating YAML by constructing the profile struct and
serializing it with serde_yaml (or otherwise escape/quote user inputs) before
writing; refer to cmd_profile_init, the network.mode field,
exec_allowlist/exec_denylist, and crates/puzzled/src/profile.rs::load_one when
making the changes.
- Around line 2252-2340: The Rego generation interpolates unescaped inputs
(deny_path, deny_extension items, and message) which can break semantics or
produce invalid Rego; update the code that builds rules (the blocks referencing
deny_path, deny_extension loop, and max_* message interpolations) to: 1) add a
helper to escape user input for regex (escape regex metacharacters, then convert
glob '*' to '.*' and anchor with ^ and $) and use it instead of
pattern.replace('*', ".*"); 2) escape extensions for regex safely (escape '.'
and other regex metacharacters before appending "$") rather than
ext.replace('.', "\\."); and 3) escape message text for embedding in Rego string
literals (escape backslashes and double quotes, and any percent/format sequences
as needed) before passing into format! so sprintf(...) and the surrounding
double-quoted Rego strings remain valid; apply these helpers where rule_name is
computed and where msg/regex/ext are inserted.
- Around line 1711-1760: output_run_result_with_governance currently only prints
policy violations but never signals failure; change it to return an exit status
(e.g., Result<(), i32> or an i32 exit code) and return a non-zero value when the
parsed policy_result contains "Rejected" (after printing violations). Update
callers (notably cmd_run which calls commit_branch() and then
output_run_result_with_governance) to check that return value and propagate a
non-zero exit (or Err) back to the process so the shell sees failure; apply the
same change for the other identical block referenced around lines 1844-1858. Use
the function name output_run_result_with_governance and cmd_run to locate and
modify the logic.
- Around line 1656-1675: poll_branch_until_done currently treats any
non-in-progress state as "ready", which lets cmd_run (and --auto-commit) proceed
on terminal states like "terminated", "failed", or "degraded" and also masks
backend errors from diff_branch; update poll_branch_until_done to explicitly
detect terminal/error states ("terminated", "failed", "degraded", etc.) and
return an enum or distinct string that cmd_run can check, and modify cmd_run to
refuse auto-commit/diff/approval when the branch is in any terminal or degraded
state; additionally, stop swallowing errors from diff_branch/diff_branch_async
(do not convert failures into empty/no_changes) and instead propagate errors up
so callers can decide (log/abort) rather than assume a successful no-op.
In `@crates/puzzled/src/profile.rs`:
- Around line 494-498: load_all() currently assigns self.profiles = new_profiles
before calling resolve_inheritance(), so failures discard the previous working
cache; change the flow to resolve on a temporary profiles container and only
swap on success: keep new_profiles in a local variable, call a resolver that can
operate on that temp (either add a new function resolve_inheritance_for(&mut
profiles) or refactor resolve_inheritance() to accept &mut profiles), run merged
validation/inheritance on the temp, and only if that returns Ok assign
self.profiles = temp_profiles; this preserves the last known-good set if
resolution fails.
- Around line 442-446: The merge_profile code unconditionally copies
child.fail_mode, child.enforcement, child.seccomp_mode, child.allow_symlinks,
and child.allow_exec_overlay which lets serde defaults in omitted child profiles
weaken parent security; update the profile loading/merge so these scalar
security fields preserve presence—either change the deserialized child type to
Option-backed fields (e.g., fail_mode: Option<...>, enforcement: Option<bool>,
seccomp_mode: Option<...>, allow_symlinks: Option<bool>, allow_exec_overlay:
Option<bool>) and merge only when Some(...), or use a presence-preserving raw
deserialize type before calling merge_profile, and add tests exercising scalar
inheritance behavior in merge_profile to ensure omitted child fields do not
override stricter parent settings.
In `@docs/profile-authoring-guide.md`:
- Around line 73-87: The YAML example under name: my-custom-agent that extends:
standard is incomplete — add either a clear “partial example” annotation or make
it a fully valid profile by including the required keys you omitted: add network
and behavioral sections and include all required resource_limits keys (not just
memory_bytes and max_pids) so the profile will load; update the
filesystem/write_allowlist comment to state whether read_allowlist/denylist are
inherited or intentionally omitted, and label the snippet as “partial (inherits
required fields from standard)” if you choose not to list them explicitly.
In `@man/puzzlectl.1`:
- Around line 168-192: The man page for the "puzzlectl policy add-rule" command
is missing documentation for the implemented flags --message and --policy-file;
update the man-page entry by adding two TP/BI sections matching the style of the
other flags that document --message <text> (custom violation message template)
and --policy-file <path> (path to the policy file to append/write the generated
Rego), referencing the implemented behavior in PolicyAction::AddRule in
crates/puzzlectl/src/main.rs so users can see both flags described alongside
--deny-path, --max-file-size, etc.
---
Outside diff comments:
In `@docs/technical-design.md`:
- Around line 615-691: The docs for the `extends` inheritance section describe
an obsolete schema (fields like `profile`, `process.exec_allow`, lowercase
`gated`/`fail_closed`) and must be updated to match the current loader/fixtures
which use `name`, `exec_allowlist`, PascalCase network modes and enum values
like `FailClosed`; update all occurrences in this section (and the other noted
block around lines 1675-1715) to use the current field names and casing,
describe vector inheritance semantics for
`exec_allowlist`/`read_allow`/`write_deny` exactly as implemented (empty list
inherits, non-empty replaces), limit inheritance depth to 3 and mention circular
detection, and ensure examples use the current enum/token names (e.g., `Gated` →
`Gated` PascalCase and `FailClosed`), plus adjust references to
`process.exec_allow` to `exec_allowlist` to avoid deserialization mismatch.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b723f735-d51e-4722-8066-9c56379bb2a0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
AGENTS.mdREADME.mdcrates/puzzle-phase2-demo/src/main.rscrates/puzzlectl/Cargo.tomlcrates/puzzlectl/src/main.rscrates/puzzled-types/src/lib.rscrates/puzzled/src/profile.rscrates/puzzled/src/sandbox/landlock.rscrates/puzzled/src/sandbox/mod.rscrates/puzzled/src/sandbox/seccomp/mod.rscrates/puzzled/src/seccomp_handler.rscrates/puzzled/src/test_helpers.rscrates/puzzled/tests/podman_native_integration.rscrates/puzzled/tests/profile_validation.rscrates/puzzled/tests/seccomp_notif_handler.rscrates/puzzled/tests/seccomp_validation.rsdocs/PRD.mddocs/admin-guide.mddocs/demo-guide.mddocs/developer-guide.mddocs/profile-authoring-guide.mddocs/technical-design.mdman/puzzlectl.1man/puzzlepod-profile.5policies/profiles/test-inherit.yaml
Comment on lines
+442
to
+446
| fail_mode: child.fail_mode, | ||
| enforcement: child.enforcement.clone(), | ||
| seccomp_mode: child.seccomp_mode, | ||
| allow_symlinks: child.allow_symlinks, | ||
| allow_exec_overlay: child.allow_exec_overlay, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that these fields are defaulted during deserialization and then
# always sourced from `child` during merge.
sed -n '335,373p' crates/puzzled-types/src/lib.rs
printf '\n---\n'
sed -n '442,446p' crates/puzzled/src/profile.rsRepository: LobsterTrap/puzzlepod
Length of output: 2015
🏁 Script executed:
# Get the complete merge_profile function
sed -n '400,500p' crates/puzzled/src/profile.rsRepository: LobsterTrap/puzzlepod
Length of output: 3644
🏁 Script executed:
# Check the default values for these types
rg -A 3 'impl.*Default.*for.*FailMode|impl.*Default.*for.*SeccompMode|impl.*Default.*for.*EnforcementRequirements' crates/puzzled-types/src/lib.rsRepository: LobsterTrap/puzzlepod
Length of output: 47
🏁 Script executed:
# Look for how profiles are actually merged when extends is present
rg -B 5 -A 10 'merge_profile|extends' crates/puzzled/src/profile.rs | head -80Repository: LobsterTrap/puzzlepod
Length of output: 3170
🏁 Script executed:
# Find definitions of FailMode, EnforcementRequirements, SeccompMode
rg -A 10 'enum FailMode|enum SeccompMode|struct EnforcementRequirements' crates/puzzled-types/src/lib.rsRepository: LobsterTrap/puzzlepod
Length of output: 1318
🏁 Script executed:
# Check if there are any tests that demonstrate the expected inheritance behavior
rg -B 5 -A 15 'test.*inherit|test.*merge' crates/puzzled/src/profile.rsRepository: LobsterTrap/puzzlepod
Length of output: 3877
Child profiles silently weaken parent hardening when omitting security fields.
Lines 442–446 unconditionally inherit fail_mode, enforcement, seccomp_mode, allow_symlinks, and allow_exec_overlay from the child, but these are #[serde(default)] fields. When a child profile omits them, deserialization fills them with defaults (e.g., SeccompMode::Permissive, enforce_bpf_lsm: false), which then override stricter parent values.
For example: parent sets seccomp_mode: Strict, child omits it, child gets SeccompMode::Permissive as default, and inheritance produces the weaker mode. Similarly, missing enforcement fields revert to false (least restrictive).
The behavior is intentional (documented in the merge_profile comment at lines 427–430), but it has security implications that are not acknowledged. Additionally, there are no tests validating scalar field inheritance—only Vec fields are tested.
Recommend: Use Option-backed fields or a presence-preserving raw type before merge, or document that child profiles must explicitly repeat parent security settings (do not omit them).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/puzzled/src/profile.rs` around lines 442 - 446, The merge_profile code
unconditionally copies child.fail_mode, child.enforcement, child.seccomp_mode,
child.allow_symlinks, and child.allow_exec_overlay which lets serde defaults in
omitted child profiles weaken parent security; update the profile loading/merge
so these scalar security fields preserve presence—either change the deserialized
child type to Option-backed fields (e.g., fail_mode: Option<...>, enforcement:
Option<bool>, seccomp_mode: Option<...>, allow_symlinks: Option<bool>,
allow_exec_overlay: Option<bool>) and merge only when Some(...), or use a
presence-preserving raw deserialize type before calling merge_profile, and add
tests exercising scalar inheritance behavior in merge_profile to ensure omitted
child fields do not override stricter parent settings.
…add-rule, profile inheritance Implement 6 features to reduce CLI friction while preserving PuzzlePod's governance model: 1. CLI help reorganization — display_order and after_help quick start 2. Better error messages — actionable hints for D-Bus, branch ID, and profile errors 3. Profile inheritance — extends field on AgentProfile with merge logic, cycle detection, max depth 3, and 7 unit tests 4. puzzlectl profile init — interactive/non-interactive YAML generator 5. puzzlectl policy add-rule — template-based Rego rule generation 6. puzzlectl run — single-command governed execution combining branch create, activate, poll, diff, and commit/rollback All features pass cargo build, test (918 pass, 8 pre-existing failures), clippy -D warnings, and fmt --check. Documentation updated across PRD, technical design, developer guide, admin guide, profile authoring guide, demo guide, README, AGENTS.md, and man pages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adam Miller <admiller@redhat.com>
…file Upgrades ratatui 0.29→0.30 to resolve RUSTSEC-2026-0002 (lru soundness issue) and fixes yanked unicode-segmentation. Adds non-empty exec_allowlist and exec_denylist to test-inherit.yaml to satisfy profile validation tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adam Miller <admiller@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (7)
crates/puzzlectl/src/main.rs (7)
1711-1762:⚠️ Potential issue | 🟠 MajorDC: Exit code is zero even when governance rejects the commit.
When
commit_branchreturns aRejectedpolicy result,output_run_result_with_governanceprints violations butcmd_runreturnsOk(()). Shell scripts checking$?will see success (exit 0) despite rollback.🛠️ Suggested approach
fn output_run_result_with_governance( format: OutputFormat, branch_id: &str, commit_json: &str, changes: usize, -) { +) -> bool { // returns true if approved, false if rejected + let mut approved = true; match format { OutputFormat::Json => { /* ... */ } OutputFormat::Text => { if let Ok(v) = serde_json::from_str::<serde_json::Value>(commit_json) { if let Some(result) = v.get("policy_result") { if result.get("Approved").is_some() { eprintln!("[run] committed ({changes} file(s), policy: approved)"); } else if let Some(violations) = result.get("Rejected") { + approved = false; // ... print violations ... } } } } } + approved }Then in
cmd_run:- output_run_result_with_governance(output, &branch_id, &result, change_count); + if !output_run_result_with_governance(output, &branch_id, &result, change_count) { + std::process::exit(1); + }Also applies to: 1843-1845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 1711 - 1762, output_run_result_with_governance currently only prints policy rejections but always yields success; change it to signal failure to the caller (e.g., return Result<(), anyhow::Error> or a bool) when the parsed commit_json contains a "policy_result" with "Rejected" so callers can exit nonzero. Update output_run_result_with_governance to detect v.get("policy_result").get("Rejected") and return Err(anyhow!("commit rejected by policy")) (or false) after printing violations, and then modify cmd_run to propagate that error (return Err / exit non-zero) instead of always returning Ok(()), ensuring shell callers see a nonzero exit on policy rejection.
1656-1676:⚠️ Potential issue | 🟠 MajorDC: Terminal/failure states not distinguished before diff/commit.
poll_branch_until_donereturns any non-in-progress state, butcmd_runproceeds todiff_branchand commit regardless. States liketerminated,failed, ordegradedshould block progression to commit/diff.Consider returning an enum or checking the state in
cmd_run:🛠️ Suggested approach
+const TERMINAL_ERROR_STATES: &[&str] = &["terminated", "failed", "degraded"]; + async fn poll_branch_until_done( client: &client::PuzzledClient, branch_id: &str, poll_ms: u64, -) -> Result<String> { +) -> Result<(String, bool)> { // (state, is_success) let interval = std::time::Duration::from_millis(poll_ms.clamp(100, 10_000)); let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_secs(86400); loop { // ... match state.as_str() { "active" | "creating" | "ready" | "frozen" | "committing" => continue, - _ => return Ok(state), + _ => { + let is_success = !TERMINAL_ERROR_STATES.contains(&state.as_str()); + return Ok((state, is_success)); + } } } }Then in
cmd_run, checkis_successand skip commit if false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 1656 - 1676, poll_branch_until_done currently treats any non-progress state as success; change it to return a typed result (e.g., a BranchState enum) instead of raw String so terminal failure states are distinguished: map the parsed state strings from client.inspect_branch / parse_json_field into enum variants (Active/Creating/Ready/Frozen/Committing/Terminated/Failed/Degraded/Unknown) and include an is_success() method or similar. Update cmd_run to call the new poll_branch_until_done and check the returned BranchState.is_success(); if false, skip diff_branch and commit (and surface an error/exit early). Ensure all call sites using poll_branch_until_done are updated to handle the enum/early-return logic.
2167-2199:⚠️ Potential issue | 🟠 MajorM: Hand-built YAML is vulnerable to injection and validation failures.
YAML injection: User input (
description,name,extends) is interpolated without escaping. A description liketest\nmalicious: injectedwill corrupt the YAML structure.Empty exec lists: Line 2181 hardcodes
exec_allowlist: []\nexec_denylist: []\n, but the PR description mentions adding non-empty lists totest-inherit.yamlto satisfy validation—suggestingload_one()may reject empty lists.🛠️ Suggested approach: use serde_yaml
- let mut y = String::new(); - y += &format!("name: {name}\n"); - y += &format!("description: \"{description}\"\n"); - // ... manual string building ... + // Build a proper AgentProfile struct and serialize it + let profile = AgentProfile { + name: name.clone(), + description: description.clone(), + extends: extends_val.clone(), + filesystem: puzzled_types::FilesystemLimits { + read_allowlist: vec!["/usr/bin".into(), /* ... */], + write_allowlist: write_dirs.iter().map(|s| PathBuf::from(s)).collect(), + denylist: vec!["/etc/shadow".into(), /* ... */], + }, + exec_allowlist: vec!["*".to_string()], // or sensible default + exec_denylist: vec![], + // ... other fields ... + }; + let y = serde_yaml::to_string(&profile)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 2167 - 2199, The current code builds YAML by concatenating into the String y (variable y in main.rs) which allows YAML injection for user fields (name, description, extends) and emits literal "exec_allowlist: []" which may fail validation; replace the hand-built concatenation with serializing a Rust config struct via serde_yaml::to_string (define a Config struct matching the YAML shape and populate fields like name, description, extends, filesystem.write_allowlist from write_dirs, network.allowed_domains from allowed_domains, resource limits from memory_mb/storage_mb/max_pids, and behavioral fields), so serde_yaml will properly escape inputs and produce valid YAML; also populate exec_allowlist and exec_denylist from the program’s actual lists (use Vecs and default to empty Vec::new() if none) instead of hardcoding "[]"; finally use the serialized string in place of y (and ensure any downstream loader such as load_one() receives the serialized YAML).
2137-2145:⚠️ Potential issue | 🟠 MajorM:
network_modenot validated against allowed values.The
net_modevalue is accepted without validation. Invalid values like"InvalidMode"will produce a YAML file that fails when loaded by the daemon.🛠️ Suggested validation
let net_mode = cli_network_mode.map(String::from).map_or_else( || prompt("Network mode (Blocked/Gated/Monitored/Unrestricted)", "Blocked"), Ok, )?; + const VALID_NETWORK_MODES: &[&str] = &["Blocked", "Gated", "Monitored", "Unrestricted"]; + if !VALID_NETWORK_MODES.contains(&net_mode.as_str()) { + anyhow::bail!( + "invalid network mode '{}': must be one of: {}", + net_mode, + VALID_NETWORK_MODES.join(", ") + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 2137 - 2145, The net_mode obtained from cli_network_mode/prompt is not validated and can contain invalid strings; after producing net_mode (the variable named net_mode from cli_network_mode.map(String::from).map_or_else(...)), validate it against the allowed set {"Blocked","Gated","Monitored","Unrestricted"} and reject/reprompt on invalid values (or map into a strong type/enum like NetworkMode) before writing YAML; implement this by either converting net_mode into a NetworkMode enum via a fallible parse/match and returning an error or looping the prompt until a valid value is entered, ensuring any downstream usage expects the validated enum/value.
2252-2272:⚠️ Potential issue | 🟠 MajorM: Incomplete regex escaping for
--deny-pathpatterns.Line 2267 only converts
*to.*without escaping other regex metacharacters. A pattern likefile[1].txtortest(prod).yamlwill produce invalid or unintended regex.🛠️ Suggested helper
+fn glob_to_regex(pattern: &str) -> String { + let mut result = String::with_capacity(pattern.len() * 2); + result.push('^'); + for c in pattern.chars() { + match c { + '*' => result.push_str(".*"), + '?' => result.push('.'), + '.' | '[' | ']' | '(' | ')' | '{' | '}' | '+' | '^' | '$' | '|' | '\\' => { + result.push('\\'); + result.push(c); + } + _ => result.push(c), + } + } + result.push('$'); + result +} + if let Some(pattern) = deny_path { let rule_name = sanitize_rule_name(pattern); let msg = message.unwrap_or("path matches denied pattern"); + let regex = glob_to_regex(pattern); rules += &format!( // ... - regex = pattern.replace('*', ".*"), + regex = regex,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 2252 - 2272, The deny-path pattern is being turned into a regex by only replacing '*' with '.*', which leaves other regex metacharacters unescaped and can create invalid/unintended patterns; update the logic that builds the rule (the block using deny_path, sanitize_rule_name, and the rules += format! call) to first escape the incoming pattern using a proper regex-escaping helper (e.g., regex::escape or an equivalent function) and then re-introduce wildcard behaviour by replacing the escaped '\*' with '.*' (or by handling '*' specially before escaping), so the final regex used in the re_match call is safe and behaves as expected for patterns like "file[1].txt" or "test(prod).yaml".
1813-1820:⚠️ Potential issue | 🟠 MajorM: Backend errors from
diff_branchare silently converted to "no changes".Line 1817 converts any
diff_brancherror to"[]", which then triggers the "no changes" path and rollback. D-Bus failures, timeouts, or daemon crashes will be hidden from the user.🛠️ Suggested fix
// Step 4: Get diff - let diff_json = client - .diff_branch(&branch_id) - .await - .unwrap_or_else(|_| "[]".into()); + let diff_json = client.diff_branch(&branch_id).await.with_context(|| { + format!("failed to get diff for branch {}", branch_id) + })?;Or if you want to handle errors gracefully without failing:
+ let diff_json = match client.diff_branch(&branch_id).await { + Ok(json) => json, + Err(e) => { + eprintln!("[run] error getting diff: {}", e); + client.rollback_branch(&branch_id, "diff failed").await?; + anyhow::bail!("diff_branch failed: {}", e); + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 1813 - 1820, The current code calls client.diff_branch(&branch_id).await.unwrap_or_else(|_| "[]".into()) which hides all backend errors by treating them as an empty diff; change this to surface or log the actual error instead of unconditionally returning "[]". Replace the unwrap_or_else with proper error handling (e.g., use match or the ? operator on client.diff_branch(&branch_id).await) so that on Err you either return/propagate the error to the caller (preserving original error information) or log the error with context including branch_id and then handle it explicitly; ensure diff_json and change_count are computed only from a successful diff_branch response and do not conflate transport/backend failures with “no changes.”
2254-2271:⚠️ Potential issue | 🟠 MajorM: User-provided
--messagenot escaped for Rego string literals.The
msgvalue is interpolated directly into Rego at line 2263. A message containing"or\will produce invalid Rego syntax.🛠️ Suggested helper
+fn escape_rego_string(s: &str) -> String { + s.replace('\\', "\\\\").replace('"', "\\\"") +} + if let Some(pattern) = deny_path { let rule_name = sanitize_rule_name(pattern); - let msg = message.unwrap_or("path matches denied pattern"); + let msg = escape_rego_string(message.unwrap_or("path matches denied pattern"));Apply the same escaping to all
msgusages in the other rule templates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 2254 - 2271, The current string interpolation builds Rego rules by inserting user-provided msg directly into the multi-line literal (see the msg variable and the rules += format! block in main.rs), which breaks on quotes/backslashes; add a helper (e.g., escape_rego_string) that escapes backslashes and double-quotes for Rego string literals (convert "\" -> "\\\"", "\" -> "\\\\" or equivalent order to avoid double-escaping) and use that helper when interpolating msg (and any other message usages across rule templates) so the generated Rego remains syntactically valid.
🧹 Nitpick comments (2)
docs/developer-guide.md (1)
397-404: Add language specifier to fenced code block.The code block is missing a language specifier, which causes markdownlint MD040 warnings and may affect syntax highlighting.
📝 Proposed fix
-``` +```bash # Interactive mode (prompts for each field) puzzlectl profile init --out /path/to/my-agent.yaml # Non-interactive with inheritance puzzlectl profile init --non-interactive --name my-agent --extends standard \ --network-mode Gated --out /path/to/my-agent.yaml</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/developer-guide.mdaround lines 397 - 404, The fenced code block
containing the puzzlectl commands lacks a language specifier (causing MD040 and
missing syntax highlighting); update the opening fence fromtobash so
the block starting with the puzzlectl profile init commands (the two example
invocations using puzzlectl) is labeled as bash/terminal and will
render/highlight correctly.</details> </blockquote></details> <details> <summary>crates/puzzlectl/src/main.rs (1)</summary><blockquote> `1645-1646`: **L: New functions lack prefixed comment tags.** As per coding guidelines, source code should use prefixed comment tags (H, M, SC, PM, DC, L, A/B/C). The existing code uses tags like `// H62:`, `// J42:`, `// M-ctl2:`, but the new `cmd_run` helpers and `cmd_profile_init`/`cmd_policy_add_rule` functions don't have design-decision or lifecycle comments. Consider adding tags like `// DC: run workflow...` or `// PM: profile init...` to document design choices. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/puzzlectl/src/main.rs` around lines 1645 - 1646, Add the required prefixed comment tags above the new helper functions to match project guidelines: insert a design-decision (DC:) comment describing the run workflow above cmd_run and any helper functions it uses, add a project-management or lifecycle tag (PM:) explaining initialization steps above cmd_profile_init, and add a design or lifecycle tag (DC:/L:) describing rule addition behavior above cmd_policy_add_rule; ensure the short tag lines mirror existing conventions (e.g., // DC: ..., // PM: ...) and briefly explain the rationale or lifecycle stage for each function. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@crates/puzzlectl/src/main.rs:
- Around line 1711-1762: output_run_result_with_governance currently only prints
policy rejections but always yields success; change it to signal failure to the
caller (e.g., return Result<(), anyhow::Error> or a bool) when the parsed
commit_json contains a "policy_result" with "Rejected" so callers can exit
nonzero. Update output_run_result_with_governance to detect
v.get("policy_result").get("Rejected") and return Err(anyhow!("commit rejected
by policy")) (or false) after printing violations, and then modify cmd_run to
propagate that error (return Err / exit non-zero) instead of always returning
Ok(()), ensuring shell callers see a nonzero exit on policy rejection.- Around line 1656-1676: poll_branch_until_done currently treats any
non-progress state as success; change it to return a typed result (e.g., a
BranchState enum) instead of raw String so terminal failure states are
distinguished: map the parsed state strings from client.inspect_branch /
parse_json_field into enum variants
(Active/Creating/Ready/Frozen/Committing/Terminated/Failed/Degraded/Unknown) and
include an is_success() method or similar. Update cmd_run to call the new
poll_branch_until_done and check the returned BranchState.is_success(); if
false, skip diff_branch and commit (and surface an error/exit early). Ensure all
call sites using poll_branch_until_done are updated to handle the
enum/early-return logic.- Around line 2167-2199: The current code builds YAML by concatenating into the
String y (variable y in main.rs) which allows YAML injection for user fields
(name, description, extends) and emits literal "exec_allowlist: []" which may
fail validation; replace the hand-built concatenation with serializing a Rust
config struct via serde_yaml::to_string (define a Config struct matching the
YAML shape and populate fields like name, description, extends,
filesystem.write_allowlist from write_dirs, network.allowed_domains from
allowed_domains, resource limits from memory_mb/storage_mb/max_pids, and
behavioral fields), so serde_yaml will properly escape inputs and produce valid
YAML; also populate exec_allowlist and exec_denylist from the program’s actual
lists (use Vecs and default to empty Vec::new() if none) instead of hardcoding
"[]"; finally use the serialized string in place of y (and ensure any downstream
loader such as load_one() receives the serialized YAML).- Around line 2137-2145: The net_mode obtained from cli_network_mode/prompt is
not validated and can contain invalid strings; after producing net_mode (the
variable named net_mode from
cli_network_mode.map(String::from).map_or_else(...)), validate it against the
allowed set {"Blocked","Gated","Monitored","Unrestricted"} and reject/reprompt
on invalid values (or map into a strong type/enum like NetworkMode) before
writing YAML; implement this by either converting net_mode into a NetworkMode
enum via a fallible parse/match and returning an error or looping the prompt
until a valid value is entered, ensuring any downstream usage expects the
validated enum/value.- Around line 2252-2272: The deny-path pattern is being turned into a regex by
only replacing '' with '.', which leaves other regex metacharacters unescaped
and can create invalid/unintended patterns; update the logic that builds the
rule (the block using deny_path, sanitize_rule_name, and the rules += format!
call) to first escape the incoming pattern using a proper regex-escaping helper
(e.g., regex::escape or an equivalent function) and then re-introduce wildcard
behaviour by replacing the escaped '*' with '.' (or by handling '' specially
before escaping), so the final regex used in the re_match call is safe and
behaves as expected for patterns like "file[1].txt" or "test(prod).yaml".- Around line 1813-1820: The current code calls
client.diff_branch(&branch_id).await.unwrap_or_else(|_| "[]".into()) which hides
all backend errors by treating them as an empty diff; change this to surface or
log the actual error instead of unconditionally returning "[]". Replace the
unwrap_or_else with proper error handling (e.g., use match or the ? operator on
client.diff_branch(&branch_id).await) so that on Err you either return/propagate
the error to the caller (preserving original error information) or log the error
with context including branch_id and then handle it explicitly; ensure diff_json
and change_count are computed only from a successful diff_branch response and do
not conflate transport/backend failures with “no changes.”- Around line 2254-2271: The current string interpolation builds Rego rules by
inserting user-provided msg directly into the multi-line literal (see the msg
variable and the rules += format! block in main.rs), which breaks on
quotes/backslashes; add a helper (e.g., escape_rego_string) that escapes
backslashes and double-quotes for Rego string literals (convert "" -> "\"",
"" -> "\\" or equivalent order to avoid double-escaping) and use that helper
when interpolating msg (and any other message usages across rule templates) so
the generated Rego remains syntactically valid.
Nitpick comments:
In@crates/puzzlectl/src/main.rs:
- Around line 1645-1646: Add the required prefixed comment tags above the new
helper functions to match project guidelines: insert a design-decision (DC:)
comment describing the run workflow above cmd_run and any helper functions it
uses, add a project-management or lifecycle tag (PM:) explaining initialization
steps above cmd_profile_init, and add a design or lifecycle tag (DC:/L:)
describing rule addition behavior above cmd_policy_add_rule; ensure the short
tag lines mirror existing conventions (e.g., // DC: ..., // PM: ...) and briefly
explain the rationale or lifecycle stage for each function.In
@docs/developer-guide.md:
- Around line 397-404: The fenced code block containing the puzzlectl commands
lacks a language specifier (causing MD040 and missing syntax highlighting);
update the opening fence fromtobash so the block starting with the
puzzlectl profile init commands (the two example invocations using puzzlectl) is
labeled as bash/terminal and will render/highlight correctly.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `a7485c2b-0773-4a17-a68e-c1d6ab107205` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cd512c0ff02d2fccc51e1d9df19d03e73f703d6a and 308a22d559cca0ce34e1ecfb9fdfa9fa20347a68. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `Cargo.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (25)</summary> * `AGENTS.md` * `README.md` * `crates/puzzle-phase2-demo/src/main.rs` * `crates/puzzlectl/Cargo.toml` * `crates/puzzlectl/src/main.rs` * `crates/puzzled-types/src/lib.rs` * `crates/puzzled/src/profile.rs` * `crates/puzzled/src/sandbox/landlock.rs` * `crates/puzzled/src/sandbox/mod.rs` * `crates/puzzled/src/sandbox/seccomp/mod.rs` * `crates/puzzled/src/seccomp_handler.rs` * `crates/puzzled/src/test_helpers.rs` * `crates/puzzled/tests/podman_native_integration.rs` * `crates/puzzled/tests/profile_validation.rs` * `crates/puzzled/tests/seccomp_notif_handler.rs` * `crates/puzzled/tests/seccomp_validation.rs` * `docs/PRD.md` * `docs/admin-guide.md` * `docs/demo-guide.md` * `docs/developer-guide.md` * `docs/profile-authoring-guide.md` * `docs/technical-design.md` * `man/puzzlectl.1` * `man/puzzlepod-profile.5` * `policies/profiles/test-inherit.yaml` </details> <details> <summary>✅ Files skipped from review due to trivial changes (10)</summary> * crates/puzzlectl/Cargo.toml * crates/puzzled/tests/seccomp_notif_handler.rs * crates/puzzled/src/sandbox/landlock.rs * crates/puzzled/src/seccomp_handler.rs * crates/puzzled/src/sandbox/seccomp/mod.rs * crates/puzzled/src/sandbox/mod.rs * docs/demo-guide.md * crates/puzzled/tests/podman_native_integration.rs * policies/profiles/test-inherit.yaml * AGENTS.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (9)</summary> * crates/puzzled/tests/seccomp_validation.rs * crates/puzzle-phase2-demo/src/main.rs * README.md * crates/puzzled/tests/profile_validation.rs * crates/puzzled/src/test_helpers.rs * man/puzzlepod-profile.5 * crates/puzzled-types/src/lib.rs * docs/technical-design.md * docs/profile-authoring-guide.md </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/profile-authoring-guide.md (1)
73-87:⚠️ Potential issue | 🟠 MajorInheritance example is still not copy-paste valid.
Line 83 onward defines only partial
resource_limitsand omits requirednetwork/behavioral; with current deserialization this example won’t load as-is.✅ Suggested fix (make snippet fully valid)
name: my-custom-agent description: Custom agent extending standard extends: standard filesystem: write_allowlist: - /workspace # read_allowlist, denylist inherited from standard (empty lists inherit) resource_limits: memory_bytes: 1073741824 + cpu_shares: 100 + io_weight: 100 max_pids: 128 - # resource_limits always use child's values + storage_quota_mb: 1024 + inode_quota: 10000 + # resource_limits always use child's values (all required keys must be present) + +network: + mode: Gated + allowed_domains: + - github.com + +behavioral: + max_deletions: 50 + max_reads_per_minute: 500 + credential_access_alert: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/profile-authoring-guide.md` around lines 73 - 87, The YAML example under name/extends shows an incomplete resource_limits block (resource_limits: memory_bytes/max_pids) and omits required sections like network and behavioral, so it won’t deserialize; update the snippet for my-custom-agent to include a complete resource_limits with all required child keys (e.g., network and behavioral entries), and ensure any inherited lists (filesystem.read_allowlist/denylist) are explicitly shown or commented to be inherited, so the example is copy-paste valid and loads as-is; refer to the resource_limits, network, behavioral, filesystem.write_allowlist, name, and extends fields when making the changes.
🧹 Nitpick comments (1)
crates/puzzled-types/src/lib.rs (1)
369-370: Prefix new Rust source comments with a project tag.These two newly added source comments are missing the required prefix tag style used across this repo.
💡 Suggested edit
- /// Optional parent profile to inherit defaults from. - /// Child fields override parent fields. Max inheritance depth: 3. + /// DC: Optional parent profile to inherit defaults from. + /// DC: Child fields override parent fields. Max inheritance depth: 3.As per coding guidelines:
**/*.{rs,c,h}must “Use prefixed comment tags in source code: H, M, SC, PM, DC, L, A/B/C”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/puzzled-types/src/lib.rs` around lines 369 - 370, The two newly added Rust doc comments ("Optional parent profile to inherit defaults from." and "Child fields override parent fields. Max inheritance depth: 3.") must use the repo's prefixed comment tag convention; update those lines in crates/puzzled-types/src/lib.rs to include the appropriate tag (for example, use "M:" or the tag used by nearby similar comments) so they read like "/// M: Optional parent profile..." and "/// M: Child fields override parent fields..." ensuring you use one of the allowed tags (H, M, SC, PM, DC, L, A/B/C) and match surrounding comment style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/admin-guide.md`:
- Line 589: The sentence describing `puzzlectl run` is awkwardly phrased as a
compound modifier; reword it for clarity by splitting into two sentences: state
that the `puzzlectl run` command provides single-command governed execution, and
then list what it does (branch creation, activation, diff display, and
commit/rollback). Update the sentence that begins with `puzzlectl run` so it
reads clearly (e.g., one sentence introducing the command followed by a second
sentence enumerating the combined actions).
In `@docs/PRD.md`:
- Around line 592-593: Update the FR-PROF-004 requirement text to explicitly
define depth semantics: state that "bounded at 3 levels" means a maximum
inheritance chain length of 3 profiles total (the child profile plus up to 2
ancestor profiles), and clarify that this is the enforced limit to avoid
off-by-one interpretation; reference FR-PROF-004 in the edit so readers and
implementers know this exact meaning.
---
Duplicate comments:
In `@docs/profile-authoring-guide.md`:
- Around line 73-87: The YAML example under name/extends shows an incomplete
resource_limits block (resource_limits: memory_bytes/max_pids) and omits
required sections like network and behavioral, so it won’t deserialize; update
the snippet for my-custom-agent to include a complete resource_limits with all
required child keys (e.g., network and behavioral entries), and ensure any
inherited lists (filesystem.read_allowlist/denylist) are explicitly shown or
commented to be inherited, so the example is copy-paste valid and loads as-is;
refer to the resource_limits, network, behavioral, filesystem.write_allowlist,
name, and extends fields when making the changes.
---
Nitpick comments:
In `@crates/puzzled-types/src/lib.rs`:
- Around line 369-370: The two newly added Rust doc comments ("Optional parent
profile to inherit defaults from." and "Child fields override parent fields. Max
inheritance depth: 3.") must use the repo's prefixed comment tag convention;
update those lines in crates/puzzled-types/src/lib.rs to include the appropriate
tag (for example, use "M:" or the tag used by nearby similar comments) so they
read like "/// M: Optional parent profile..." and "/// M: Child fields override
parent fields..." ensuring you use one of the allowed tags (H, M, SC, PM, DC, L,
A/B/C) and match surrounding comment style.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a67d25b-af60-4d8a-889d-75b4bd7e926e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
AGENTS.mdREADME.mdcrates/puzzle-phase2-demo/src/main.rscrates/puzzlectl/Cargo.tomlcrates/puzzlectl/src/main.rscrates/puzzled-types/src/lib.rscrates/puzzled/src/profile.rscrates/puzzled/src/sandbox/landlock.rscrates/puzzled/src/sandbox/mod.rscrates/puzzled/src/sandbox/seccomp/mod.rscrates/puzzled/src/seccomp_handler.rscrates/puzzled/src/test_helpers.rscrates/puzzled/tests/podman_native_integration.rscrates/puzzled/tests/profile_validation.rscrates/puzzled/tests/seccomp_notif_handler.rscrates/puzzled/tests/seccomp_validation.rsdocs/PRD.mddocs/admin-guide.mddocs/demo-guide.mddocs/developer-guide.mddocs/profile-authoring-guide.mddocs/technical-design.mdman/puzzlectl.1man/puzzlepod-profile.5policies/profiles/test-inherit.yaml
✅ Files skipped from review due to trivial changes (13)
- crates/puzzlectl/Cargo.toml
- AGENTS.md
- crates/puzzled/tests/seccomp_notif_handler.rs
- crates/puzzled/src/sandbox/seccomp/mod.rs
- crates/puzzled/src/sandbox/mod.rs
- crates/puzzled/src/seccomp_handler.rs
- README.md
- docs/demo-guide.md
- crates/puzzled/tests/podman_native_integration.rs
- policies/profiles/test-inherit.yaml
- crates/puzzled/src/profile.rs
- man/puzzlectl.1
- crates/puzzlectl/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/puzzled/tests/seccomp_validation.rs
- crates/puzzle-phase2-demo/src/main.rs
- docs/developer-guide.md
- docs/technical-design.md
|
|
||
| ### Running Governed Agents | ||
|
|
||
| `puzzlectl run` provides single-command governed execution, combining branch creation, activation, diff display, and commit/rollback: |
There was a problem hiding this comment.
Tighten wording in the puzzlectl run sentence.
Line 589 reads awkwardly as a compound modifier; rephrase for clarity.
✏️ Suggested edit
-`puzzlectl run` provides single-command governed execution, combining branch creation, activation, diff display, and commit/rollback:
+`puzzlectl run` provides a single-command workflow for governed execution, combining branch creation, activation, diff display, and commit/rollback:📝 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
| `puzzlectl run` provides single-command governed execution, combining branch creation, activation, diff display, and commit/rollback: | |
| `puzzlectl run` provides a single-command workflow for governed execution, combining branch creation, activation, diff display, and commit/rollback: |
🧰 Tools
🪛 LanguageTool
[grammar] ~589-~589: Use a hyphen to join words.
Context: ... puzzlectl run provides single-command governed execution, combining branch cre...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/admin-guide.md` at line 589, The sentence describing `puzzlectl run` is
awkwardly phrased as a compound modifier; reword it for clarity by splitting
into two sentences: state that the `puzzlectl run` command provides
single-command governed execution, and then list what it does (branch creation,
activation, diff display, and commit/rollback). Update the sentence that begins
with `puzzlectl run` so it reads clearly (e.g., one sentence introducing the
command followed by a second sentence enumerating the combined actions).
Comment on lines
+592
to
+593
| **FR-PROF-004:** Profiles SHALL support inheritance via an optional `extends` field. A child profile inherits all fields from the named parent; child fields override parent fields. Vec fields (exec_allowlist, filesystem lists, etc.) inherit the parent's values when the child's list is empty. Inheritance depth SHALL be bounded at 3 levels. Circular inheritance SHALL be detected and rejected. | ||
|
|
There was a problem hiding this comment.
Clarify depth semantics to avoid off-by-one interpretation.
At Line 592, “bounded at 3 levels” is ambiguous. The current implementation enforces a maximum chain length of 3 profiles total (child + up to 2 ancestors). Please state that explicitly to prevent incorrect profile author expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/PRD.md` around lines 592 - 593, Update the FR-PROF-004 requirement text
to explicitly define depth semantics: state that "bounded at 3 levels" means a
maximum inheritance chain length of 3 profiles total (the child profile plus up
to 2 ancestor profiles), and clarify that this is the enforced limit to avoid
off-by-one interpretation; reference FR-PROF-004 in the edit so readers and
implementers know this exact meaning.
- Handle terminal branch states (failed/terminated/degraded) explicitly in cmd_run instead of proceeding to diff/commit - Return non-zero exit code when governance policy rejects changes - Stop swallowing diff_branch errors as empty changeset - Generate valid profiles from profile init: validate network mode, populate exec_allowlist/exec_denylist, use serde_yaml serialization instead of hand-concatenated YAML strings - Escape user inputs in Rego rule generation (regex metacharacters, Rego string literals) to prevent injection - Fix load_all atomicity: resolve inheritance on temporary loader before swapping to preserve last known-good profile set on failure - Document scalar inheritance security caveat (serde defaults can weaken parent hardening when child omits fields) - Add --message and --policy-file flags to man page - Update obsolete profile schema in technical-design.md to match current field names and PascalCase enum values Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adam Miller <admiller@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/technical-design.md`:
- Line 968: The command reference for "puzzlectl policy add-rule" is missing the
--message and --policy-file options; update the docs entry for the puzzlectl
policy add-rule command (the command synopsis shown) to include both --message
and --policy-file flags, matching the PR objectives and other documentation, and
ensure any short descriptions or examples elsewhere referencing policy add-rule
are updated to document these two options as well.
- Around line 1679-1695: The JSON schema for agent profiles is too permissive:
tighten the "filesystem" object by adding required keys (e.g., require at least
one of "read_allowlist" or "write_allowlist" or
"denylist"/"read_denylist"/"write_denylist") and add non-empty constraints
(minItems: 1) on list properties used by the loader such as "read_allowlist",
"write_allowlist", "read_denylist", "write_denylist", and on top-level execution
lists "exec_allowlist" and "exec_denylist"; update the schema entries for those
properties (the "filesystem" object and the "exec_allowlist"/"exec_denylist"
definitions) to include "required" where appropriate and "minItems": 1 so
profiles that pass JSON Schema also meet loader/profile validation expectations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e426d9d2-60c9-4031-a8b0-0a2981811621
📒 Files selected for processing (5)
crates/puzzlectl/src/main.rscrates/puzzled/src/profile.rsdocs/profile-authoring-guide.mddocs/technical-design.mdman/puzzlectl.1
✅ Files skipped from review due to trivial changes (2)
- docs/profile-authoring-guide.md
- man/puzzlectl.1
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/puzzled/src/profile.rs
- crates/puzzlectl/src/main.rs
| POLICY MANAGEMENT: | ||
| puzzlectl policy reload | ||
| puzzlectl policy test <policy_file> --input=<json_file> | ||
| puzzlectl policy add-rule [--deny-path <glob>] [--max-file-size <bytes>] [--deny-extension <exts>] [--max-files <n>] [--severity <level>] [--dry-run] |
There was a problem hiding this comment.
policy add-rule options appear incomplete in this command reference.
Line 968 omits --message and --policy-file, which are called out in the PR objectives/docs updates. Please keep this command surface synchronized to avoid operator confusion.
🛠️ Suggested doc update
- puzzlectl policy add-rule [--deny-path <glob>] [--max-file-size <bytes>] [--deny-extension <exts>] [--max-files <n>] [--severity <level>] [--dry-run]
+ puzzlectl policy add-rule [--deny-path <glob>] [--max-file-size <bytes>] [--deny-extension <exts>] [--max-files <n>] [--severity <level>] [--message <text>] [--policy-file <file>] [--dry-run]📝 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
| puzzlectl policy add-rule [--deny-path <glob>] [--max-file-size <bytes>] [--deny-extension <exts>] [--max-files <n>] [--severity <level>] [--dry-run] | |
| puzzlectl policy add-rule [--deny-path <glob>] [--max-file-size <bytes>] [--deny-extension <exts>] [--max-files <n>] [--severity <level>] [--message <text>] [--policy-file <file>] [--dry-run] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/technical-design.md` at line 968, The command reference for "puzzlectl
policy add-rule" is missing the --message and --policy-file options; update the
docs entry for the puzzlectl policy add-rule command (the command synopsis
shown) to include both --message and --policy-file flags, matching the PR
objectives and other documentation, and ensure any short descriptions or
examples elsewhere referencing policy add-rule are updated to document these two
options as well.
Comment on lines
+1679
to
+1695
| "required": ["name", "description", "filesystem", "exec_allowlist", "resource_limits", "network", "behavioral"], | ||
| "properties": { | ||
| "profile": { "type": "string", "pattern": "^[a-z][a-z0-9-]{0,62}$" }, | ||
| "version": { "type": "integer", "minimum": 1 }, | ||
| "name": { "type": "string", "pattern": "^[a-zA-Z][a-zA-Z0-9_-]{0,62}$" }, | ||
| "description": { "type": "string" }, | ||
| "extends": { "type": "string", "pattern": "^[a-zA-Z][a-zA-Z0-9_-]{0,62}$", "description": "Parent profile name for inheritance (max depth 3, cycle detection enforced)" }, | ||
| "filesystem": { | ||
| "type": "object", | ||
| "properties": { | ||
| "read_allow": { "type": "array", "items": { "type": "string" } }, | ||
| "read_deny": { "type": "array", "items": { "type": "string" } }, | ||
| "write_allow": { "type": "array", "items": { "type": "string" } }, | ||
| "write_deny": { "type": "array", "items": { "type": "string" } } | ||
| "read_allowlist": { "type": "array", "items": { "type": "string" } }, | ||
| "write_allowlist": { "type": "array", "items": { "type": "string" } }, | ||
| "denylist": { "type": "array", "items": { "type": "string" } }, | ||
| "read_denylist": { "type": "array", "items": { "type": "string" } }, | ||
| "write_denylist": { "type": "array", "items": { "type": "string" } } | ||
| } | ||
| }, | ||
| "process": { | ||
| "exec_allowlist": { "type": "array", "items": { "type": "string" } }, | ||
| "exec_denylist": { "type": "array", "items": { "type": "string" } }, |
There was a problem hiding this comment.
Schema contract is too permissive for profile validation.
At Line 1684-Line 1693, filesystem has no required fields, and at Line 1694-Line 1695 the exec lists have no non-empty constraint. This can allow profiles that pass schema but fail loader/profile validation behavior.
📌 Proposed schema tightening
"filesystem": {
"type": "object",
+ "required": ["read_allowlist", "write_allowlist", "denylist"],
"properties": {
"read_allowlist": { "type": "array", "items": { "type": "string" } },
"write_allowlist": { "type": "array", "items": { "type": "string" } },
"denylist": { "type": "array", "items": { "type": "string" } },
"read_denylist": { "type": "array", "items": { "type": "string" } },
"write_denylist": { "type": "array", "items": { "type": "string" } }
}
},
- "exec_allowlist": { "type": "array", "items": { "type": "string" } },
- "exec_denylist": { "type": "array", "items": { "type": "string" } },
+ "exec_allowlist": { "type": "array", "minItems": 1, "items": { "type": "string" } },
+ "exec_denylist": { "type": "array", "minItems": 1, "items": { "type": "string" } },📝 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
| "required": ["name", "description", "filesystem", "exec_allowlist", "resource_limits", "network", "behavioral"], | |
| "properties": { | |
| "profile": { "type": "string", "pattern": "^[a-z][a-z0-9-]{0,62}$" }, | |
| "version": { "type": "integer", "minimum": 1 }, | |
| "name": { "type": "string", "pattern": "^[a-zA-Z][a-zA-Z0-9_-]{0,62}$" }, | |
| "description": { "type": "string" }, | |
| "extends": { "type": "string", "pattern": "^[a-zA-Z][a-zA-Z0-9_-]{0,62}$", "description": "Parent profile name for inheritance (max depth 3, cycle detection enforced)" }, | |
| "filesystem": { | |
| "type": "object", | |
| "properties": { | |
| "read_allow": { "type": "array", "items": { "type": "string" } }, | |
| "read_deny": { "type": "array", "items": { "type": "string" } }, | |
| "write_allow": { "type": "array", "items": { "type": "string" } }, | |
| "write_deny": { "type": "array", "items": { "type": "string" } } | |
| "read_allowlist": { "type": "array", "items": { "type": "string" } }, | |
| "write_allowlist": { "type": "array", "items": { "type": "string" } }, | |
| "denylist": { "type": "array", "items": { "type": "string" } }, | |
| "read_denylist": { "type": "array", "items": { "type": "string" } }, | |
| "write_denylist": { "type": "array", "items": { "type": "string" } } | |
| } | |
| }, | |
| "process": { | |
| "exec_allowlist": { "type": "array", "items": { "type": "string" } }, | |
| "exec_denylist": { "type": "array", "items": { "type": "string" } }, | |
| "required": ["name", "description", "filesystem", "exec_allowlist", "resource_limits", "network", "behavioral"], | |
| "properties": { | |
| "name": { "type": "string", "pattern": "^[a-zA-Z][a-zA-Z0-9_-]{0,62}$" }, | |
| "description": { "type": "string" }, | |
| "extends": { "type": "string", "pattern": "^[a-zA-Z][a-zA-Z0-9_-]{0,62}$", "description": "Parent profile name for inheritance (max depth 3, cycle detection enforced)" }, | |
| "filesystem": { | |
| "type": "object", | |
| "required": ["read_allowlist", "write_allowlist", "denylist"], | |
| "properties": { | |
| "read_allowlist": { "type": "array", "items": { "type": "string" } }, | |
| "write_allowlist": { "type": "array", "items": { "type": "string" } }, | |
| "denylist": { "type": "array", "items": { "type": "string" } }, | |
| "read_denylist": { "type": "array", "items": { "type": "string" } }, | |
| "write_denylist": { "type": "array", "items": { "type": "string" } } | |
| } | |
| }, | |
| "exec_allowlist": { "type": "array", "minItems": 1, "items": { "type": "string" } }, | |
| "exec_denylist": { "type": "array", "minItems": 1, "items": { "type": "string" } }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/technical-design.md` around lines 1679 - 1695, The JSON schema for agent
profiles is too permissive: tighten the "filesystem" object by adding required
keys (e.g., require at least one of "read_allowlist" or "write_allowlist" or
"denylist"/"read_denylist"/"write_denylist") and add non-empty constraints
(minItems: 1) on list properties used by the loader such as "read_allowlist",
"write_allowlist", "read_denylist", "write_denylist", and on top-level execution
lists "exec_allowlist" and "exec_denylist"; update the schema entries for those
properties (the "filesystem" object and the "exec_allowlist"/"exec_denylist"
definitions) to include "required" where appropriate and "minItems": 1 so
profiles that pass JSON Schema also meet loader/profile validation expectations.
maxamillion
added a commit
that referenced
this pull request
Mar 30, 2026
* Add developer experience features: run command, profile init, policy add-rule, profile inheritance Implement 6 features to reduce CLI friction while preserving PuzzlePod's governance model: 1. CLI help reorganization — display_order and after_help quick start 2. Better error messages — actionable hints for D-Bus, branch ID, and profile errors 3. Profile inheritance — extends field on AgentProfile with merge logic, cycle detection, max depth 3, and 7 unit tests 4. puzzlectl profile init — interactive/non-interactive YAML generator 5. puzzlectl policy add-rule — template-based Rego rule generation 6. puzzlectl run — single-command governed execution combining branch create, activate, poll, diff, and commit/rollback All features pass cargo build, test (918 pass, 8 pre-existing failures), clippy -D warnings, and fmt --check. Documentation updated across PRD, technical design, developer guide, admin guide, profile authoring guide, demo guide, README, AGENTS.md, and man pages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adam Miller <admiller@redhat.com> * fix: bump ratatui to 0.30 and add required fields to test-inherit profile Upgrades ratatui 0.29→0.30 to resolve RUSTSEC-2026-0002 (lru soundness issue) and fixes yanked unicode-segmentation. Adds non-empty exec_allowlist and exec_denylist to test-inherit.yaml to satisfy profile validation tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adam Miller <admiller@redhat.com> * fix: address CodeRabbit review feedback for PR #6 - Handle terminal branch states (failed/terminated/degraded) explicitly in cmd_run instead of proceeding to diff/commit - Return non-zero exit code when governance policy rejects changes - Stop swallowing diff_branch errors as empty changeset - Generate valid profiles from profile init: validate network mode, populate exec_allowlist/exec_denylist, use serde_yaml serialization instead of hand-concatenated YAML strings - Escape user inputs in Rego rule generation (regex metacharacters, Rego string literals) to prevent injection - Fix load_all atomicity: resolve inheritance on temporary loader before swapping to preserve last known-good profile set on failure - Document scalar inheritance security caveat (serde defaults can weaken parent hardening when child omits fields) - Add --message and --policy-file flags to man page - Update obsolete profile schema in technical-design.md to match current field names and PascalCase enum values Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adam Miller <admiller@redhat.com> --------- Signed-off-by: Adam Miller <admiller@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Adam Miller <admiller@redhat.com>
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
puzzlectl runsingle-command workflow,profile initscaffolding,policy add-rulegeneration, and profile inheritance (extendsfield)Test plan
make cipasses (fmt, clippy, 1834 tests, cargo-deny)puzzlectl runworkflow end-to-endpuzzlectl profile initgenerates valid YAMLpuzzlectl policy add-rulegenerates valid Rego🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores