refactor(cli-sub-agent): consolidate plan execution args into PlanRunContext#301
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec244b3797
ℹ️ 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".
crates/cli-sub-agent/src/plan_cmd.rs
Outdated
| if !is_failure { | ||
| completed_steps.insert(step.id); | ||
| } |
There was a problem hiding this comment.
The resume journal currently treats every non-is_failure result as completed, but execute_step returns unsupported loop steps as skipped=true with exit_code=2, so they get persisted in completed_steps even though they are still an error. On the next run, those steps hit the RESUME-SKIP branch and are omitted from results, which can make unsupported_skips drop to zero and let a workflow report success after rerun despite still containing unsupported loop logic. Only genuinely successful steps (exit_code == 0) should be added to completed_steps.
Useful? React with 👍 / 👎.
| if let Some(rest) = trimmed.strip_prefix("ssh://git@github.com/") { | ||
| return Some(rest.to_string()); | ||
| } | ||
| Some(trimmed.to_string()) |
There was a problem hiding this comment.
This fallback returns the raw remote.origin.url string for any non-matched format, and the new run-context log prints that value to stderr; credentialed remotes like https://token@host/org/repo.git will therefore leak secrets into logs. Please strip URL userinfo (or avoid emitting unknown URL formats) before returning/logging effective_repo (the same helper pattern was also added in plan_cmd.rs).
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1e5fbab19
ℹ️ 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".
crates/cli-sub-agent/src/plan_cmd.rs
Outdated
| if !is_failure { | ||
| completed_steps.insert(step.id); |
There was a problem hiding this comment.
Steps are marked completed whenever !is_failure, but is_failure is defined as !result.skipped && result.exit_code != 0, so skipped steps with non-zero exit codes are treated as completed. This lets failures like unsupported loop steps (and other non-zero skipped outcomes) be written to completed_steps, then silently bypassed on the next resumed run, which can turn a previously failing workflow into a false success without re-evaluating that step.
Useful? React with 👍 / 👎.
| if let Some(rest) = trimmed.strip_prefix("ssh://git@github.com/") { | ||
| return Some(rest.to_string()); | ||
| } | ||
| Some(trimmed.to_string()) |
There was a problem hiding this comment.
The fallback path returns remote.origin.url verbatim, and this value is printed in run context logs; if a repo uses credential-bearing HTTPS remotes (for example token/userinfo in the URL), this leaks secrets into stderr/CI logs. The fallback should sanitize or strip auth components before returning/logging the value.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c809690b8f
ℹ️ 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".
| if let Some(rest) = trimmed.strip_prefix("ssh://git@github.com/") { | ||
| return Some(rest.to_string()); | ||
| } | ||
| Some(trimmed.to_string()) |
There was a problem hiding this comment.
handle_plan_run prints the value returned by detect_effective_repo, but this function falls back to Some(trimmed.to_string()) without removing user:token@ credentials from HTTPS remotes. If remote.origin.url is set to a credentialed URL (common in CI), running csa plan run will emit the token to stderr/logs, leaking secrets; this path should sanitize the authority segment the same way run_cmd now does.
Useful? React with 👍 / 👎.
| .join(".csa") | ||
| .join("state") | ||
| .join("plan") | ||
| .join(format!("{safe_name}.journal.json")) |
There was a problem hiding this comment.
The journal filename is derived only from normalized plan name, so different workflow files that share a plan name (or names that normalize to the same slug) collide on the same .journal.json path. Running one workflow can overwrite another workflow’s in-progress journal, which breaks resume correctness and can force rerunning previously completed side-effecting steps.
Useful? React with 👍 / 👎.
RyderFreeman4Logos
force-pushed
the
fix/workflow-polling-delegate-to-csa
branch
from
c809690 to
43247e3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43247e316b
ℹ️ 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".
crates/cli-sub-agent/src/plan_cmd.rs
Outdated
| let raw = String::from_utf8_lossy(&output.stdout).trim().to_string(); | ||
| if raw.is_empty() { | ||
| return None; | ||
| } | ||
| let trimmed = raw.trim_end_matches(".git"); |
There was a problem hiding this comment.
handle_plan_run now prints effective_repo to stderr, but detect_effective_repo in this file returns remote.origin.url almost verbatim (only trimming .git). In repositories that use credentialed remotes (for example https://user:token@github.com/org/repo), this leaks secrets into logs and any wrapper that captures 2>&1. The equivalent helper in run_cmd.rs strips credentials, so this path should apply the same sanitization before returning/logging the repo string.
Useful? React with 👍 / 👎.
| let same_workflow = journal.workflow_name == plan.name | ||
| && journal.workflow_path == normalize_path(workflow_path); | ||
| if !same_workflow || journal.status == "completed" { |
There was a problem hiding this comment.
Resume currently keys only on workflow name/path and status != completed, then reuses completed_steps without checking whether HEAD or the working tree changed since the journal was written. If a run fails and the user edits files before rerunning, earlier completed gates (e.g. fmt/lint/tests) are skipped and later steps run on new, unvalidated code. The journal needs a repository-state guard (at least commit + dirty fingerprint) before trusting prior completed steps.
Useful? React with 👍 / 👎.
1 participant
Consolidate plan execution arguments into PlanRunContext to reduce pass-through parameter diffusion. Also includes a minor test fix in llm_client.rs. All quality gates (fmt, clippy, test) passed. Heterogeneous code review (by codex) passed.