feat(hooks): deterministic pipeline enforcement via CSA:NEXT_STEP directives#538
feat(hooks): deterministic pipeline enforcement via CSA:NEXT_STEP directives#538RyderFreeman4Logos merged 3 commits intomainfrom
Conversation
…haining - Emit CSA:NEXT_STEP directive after csa review and weave step completion - Add NextStepDirective struct in csa-hooks for structured emission - Review complete → emits next step pointing to pr-bot - Weave plan steps → emit next step pointing to subsequent step - Update dev2merge PATTERN.md to document pr-bot as mandatory step Partially addresses #524 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism for pipeline chaining by introducing CSA:NEXT_STEP directives. It includes a new robust parser and formatter for these directives in the csa-hooks crate and integrates them into the plan execution and review commands within cli-sub-agent. Feedback focuses on improving the reliability of the directive format by escaping double quotes in command strings and suggests using constants for duplicated command strings to enhance maintainability.
| "cmd" => { | ||
| let v = value.trim(); | ||
| if !v.is_empty() { | ||
| cmd = Some(v.to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
To complement the escaping of double quotes in format_next_step_directive, the parser needs to unescape them. When parsing the cmd attribute, you should replace " back to " to reconstruct the original command string.
| "cmd" => { | |
| let v = value.trim(); | |
| if !v.is_empty() { | |
| cmd = Some(v.to_string()); | |
| } | |
| } | |
| "cmd" => { | |
| let v = value.trim().replace(""", "\""); | |
| if !v.is_empty() { | |
| cmd = Some(v.to_string()); | |
| } | |
| } |
| pub fn format_next_step_directive(cmd: &str, required: bool) -> String { | ||
| format!( | ||
| "<!-- CSA:NEXT_STEP cmd=\"{}\" required={} -->", | ||
| cmd, required | ||
| ) | ||
| } |
There was a problem hiding this comment.
The cmd string is directly embedded in the formatted string. If cmd contains a double quote ("), it will break the directive's format, as the parser will interpret it as the end of the cmd attribute's value. This can lead to incorrect parsing and pipeline failures.
To fix this, you should escape double quotes within the cmd string. Using the HTML entity " is a good choice given the HTML-comment-like syntax of the directive. Note that the parser will also need to be updated to handle this unescaping.
| pub fn format_next_step_directive(cmd: &str, required: bool) -> String { | |
| format!( | |
| "<!-- CSA:NEXT_STEP cmd=\"{}\" required={} -->", | |
| cmd, required | |
| ) | |
| } | |
| pub fn format_next_step_directive(cmd: &str, required: bool) -> String { | |
| format!( | |
| "<!-- CSA:NEXT_STEP cmd=\"{}\" required={} -->", | |
| cmd.replace('"', """), required | |
| ) | |
| } |
| eprintln!( | ||
| "{}", | ||
| csa_hooks::format_next_step_directive( | ||
| "csa plan run patterns/dev2merge/workflow.toml --step pr-bot", |
There was a problem hiding this comment.
This command string is hardcoded and duplicated later in the file (line 451). To improve maintainability and avoid potential inconsistencies, it's best to define it as a constant at the top of the module or file. For example: const NEXT_STEP_PR_BOT_CMD: &str = "csa plan run patterns/dev2merge/workflow.toml --step pr-bot";
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0232e3a28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| eprintln!( | ||
| "{}", | ||
| csa_hooks::format_next_step_directive( | ||
| "csa plan run patterns/dev2merge/workflow.toml --step pr-bot", |
There was a problem hiding this comment.
Use supported
csa plan run syntax in NEXT_STEP command
This directive emits csa plan run ... --step ..., but PlanCommands::Run (see crates/cli-sub-agent/src/cli.rs) does not define a --step option—only file/pattern/var/tool/dry_run/cd—so the command will be rejected as an unknown flag. In the CLEAN path this prevents orchestrators from mechanically chaining review into the required pr-bot workflow.
Useful? React with 👍 / 👎.
| && let Some(next_step) = find_next_step(step, &plan.steps) | ||
| { | ||
| let cmd = format!( | ||
| "csa plan run --step {} \"{}\"", |
There was a problem hiding this comment.
Escape nested quotes before formatting NEXT_STEP directives
The plan path builds cmd with an embedded quoted title ("{}"), then format_next_step_directive wraps that whole command in cmd="..." again. Because the parser stops quoted values at the first " in parse_next_step_directive, emitted directives get truncated/malformed whenever a title is present, so downstream consumers cannot reliably parse the intended next command (and may miss required=true).
Useful? React with 👍 / 👎.
…plicate pr-bot command Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism for pipeline chaining by emitting and parsing CSA:NEXT_STEP directives. Key changes include an enhanced directive parser in the csa-hooks crate that supports shell commands and requirement flags, and integration into the cli-sub-agent to suggest follow-up steps after plan execution or code reviews. Feedback focuses on improving the robustness of the directive parser and formatter regarding backslash escaping, ensuring the next-step logic is position-based rather than ID-dependent, and maintaining consistency by using defined constants.
| fn find_next_step<'a>(current: &PlanStep, steps: &'a [PlanStep]) -> Option<&'a PlanStep> { | ||
| steps.iter().find(|s| s.id > current.id) | ||
| } |
There was a problem hiding this comment.
The current implementation of find_next_step relies on comparing step IDs, which assumes that IDs are strictly increasing and reflect the execution order. However, execute_plan_with_journal iterates through the steps in their list order. If the plan contains non-sequential IDs or is reordered, this logic may fail to identify the correct successor or miss it entirely. It is safer and more efficient to find the next step based on its position in the steps slice.
| fn find_next_step<'a>(current: &PlanStep, steps: &'a [PlanStep]) -> Option<&'a PlanStep> { | |
| steps.iter().find(|s| s.id > current.id) | |
| } | |
| fn find_next_step<'a>(current: &PlanStep, steps: &'a [PlanStep]) -> Option<&'a PlanStep> { | |
| let idx = steps.iter().position(|s| std::ptr::eq(s, current))?; | |
| steps.get(idx + 1) | |
| } |
| } | ||
| end += 1; | ||
| } | ||
| let raw = &remaining[..end]; | ||
| remaining = if end < remaining.len() { | ||
| &remaining[end + 1..] |
There was a problem hiding this comment.
The logic for finding the closing quote of a directive attribute is susceptible to a parsing error when a literal backslash precedes the quote (e.g., cmd="C:\\"). In such cases, the parser incorrectly treats the closing quote as escaped and continues parsing into subsequent attributes. A more robust approach should track the escape state to correctly distinguish between escaped quotes and literal backslashes.
let mut escaped = false;
while end < bytes.len() {
if bytes[end] == b'"' && !escaped {
break;
}
escaped = bytes[end] == b'\\' && !escaped;
end += 1;
}| pub fn format_next_step_directive(cmd: &str, required: bool) -> String { | ||
| let escaped = cmd.replace('"', r#"\""#); | ||
| format!( | ||
| "<!-- CSA:NEXT_STEP cmd=\"{}\" required={} -->", | ||
| escaped, required | ||
| ) | ||
| } |
There was a problem hiding this comment.
The format_next_step_directive function only escapes double quotes, but it should also escape backslashes to ensure that commands containing literal backslashes (like Windows paths) are correctly serialized and can be reliably parsed back.
| pub fn format_next_step_directive(cmd: &str, required: bool) -> String { | |
| let escaped = cmd.replace('"', r#"\""#); | |
| format!( | |
| "<!-- CSA:NEXT_STEP cmd=\"{}\" required={} -->", | |
| escaped, required | |
| ) | |
| } | |
| pub fn format_next_step_directive(cmd: &str, required: bool) -> String { | |
| let escaped = cmd.replace('\\', "\\\\").replace('"', "\\\""); | |
| format!( | |
| "<!-- CSA:NEXT_STEP cmd=\"{}\" required={} -->", | |
| escaped, required | |
| ) | |
| } |
| eprintln!( | ||
| "{}", | ||
| csa_hooks::format_next_step_directive( | ||
| "csa plan run patterns/dev2merge/workflow.toml --step pr-bot", |
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Audit: Bot Findings (Round 2)Finding 1:
|
Summary
NextStepDirectivestruct incsa-hooksfor structured next-step emissioncsa reviewcompletes successfully, emitCSA:NEXT_STEPdirective pointing to pr-botCSA:NEXT_STEPpointing to next stepPATTERN.mdto document pr-bot as mandatory stepPartially addresses #524
Test plan
csa review --branch mainemitsCSA:NEXT_STEPon successcsa plan runemitsCSA:NEXT_STEPbetween stepsweave compilevalidates workflow.toml🤖 Generated with Claude Code