Skip to content

Stop repairing stale code-review usage metadata #134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dkropachev wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Stop repairing stale code-review usage metadata #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 1 addition & 98 deletions codex-rs/cli/src/workflow_cmd/compat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@ fn apply_compatibility_repairs(target: &RepairWorkflowTarget) -> anyhow::Result<
if repaired != contents {
fs::write(&workflow_yaml, repaired)?;
repairs.push(format!(
"Updated {} with code-review autocomplete metadata",
"Updated {} with code-review workflow metadata",
workflow_yaml.display()
));
}
Expand Down Expand Up @@ -719,80 +719,9 @@ fn repair_code_review_workflow_metadata(contents: &str) -> String {
"userDescription",
"Run a code review and repro workflow on the current branch.",
);
if !has_workflow_usage_options(&repaired) {
if !repaired.ends_with('\n') {
repaired.push('\n');
}
repaired.push_str(CODE_REVIEW_USAGE_OPTIONS_YAML);
}
repaired
}

const CODE_REVIEW_USAGE_OPTIONS_YAML: &str = r#"usage:
options:
- flag: --action
valueHint: <review|read-report|list-reports|incremental|resume>
description: Run mode: review, read-report, list-reports, incremental, or resume.
- flag: --working-directory
valueHint: <string>
description: Repository path or report-list directory filter.
- flag: --target-ref
valueHint: <string>
description: Branch or commit to review.
- flag: --base-ref
valueHint: <string>
description: Upstream/base reference for branch comparison.
- flag: --scope
valueHint: <branch|repo>
description: Review branch changes or the whole repository.
- flag: --review-id
valueHint: <string>
description: Existing review ID for read-report, incremental, or resume.
- flag: --review-model
valueHint: <string>
description: Model override for initial review agents.
- flag: --repro-model
valueHint: <string>
description: Model override for reproduction agents.
- flag: --limit
valueHint: <integer>
description: Maximum kept findings sent to reproduction.
- flag: --chunk-size-bytes
valueHint: <integer>
description: Maximum chunk size in bytes.
- flag: --module-depth
valueHint: <integer>
description: Directory depth used for chunk grouping.
- flag: --severity-threshold
valueHint: <number>
description: Minimum severity from 0 to 10.
- flag: --confidence-threshold
valueHint: <number>
description: Minimum confidence from 0 to 100.
- flag: --include-preexisting
description: Keep preexisting findings in branch scope.
- flag: --include-skipped-by-limit
description: Incremental mode also replays findings skipped by the previous limit.
- flag: --allowed-areas
valueHint: <Test|Code|Docs|Comment|Else>
description: Allowed finding areas.
- flag: --database-path
valueHint: <string>
description: SQLite audit store path.
- flag: --artifacts-dir
valueHint: <string>
description: Artifact root directory.
- flag: --output
valueHint: <json|md>
description: Return json or markdown wrapper output.
- flag: --report-type
valueHint: <default|github-review>
description: Render default markdown or a GitHub review draft.
- flag: --findings
valueHint: <confirmed|filtered|both>
description: Return confirmed, filtered, or both finding sets.
"#;

fn default_command_from_id(id: &str) -> String {
slugify(id.rsplit('/').next().unwrap_or(id))
}
Expand Down Expand Up @@ -1195,32 +1124,6 @@ fn strip_inline_comment(value: &str) -> &str {
value
}

fn has_workflow_usage_options(contents: &str) -> bool {
let mut in_usage = false;
for line in contents.lines() {
let trimmed = line.trim();
if trimmed.is_empty() || trimmed.starts_with('#') {
continue;
}
let indent = line.len().saturating_sub(line.trim_start().len());
if indent == 0 {
in_usage = trimmed
.split_once(':')
.is_some_and(|(line_key, _)| line_key.trim() == "usage");
continue;
}
if in_usage
&& indent == 2
&& trimmed
.split_once(':')
.is_some_and(|(line_key, _)| line_key.trim() == "options")
{
return true;
}
}
false
}

fn set_top_level_yaml_scalar(contents: &str, key: &str, value: &str) -> String {
let mut replaced = false;
let mut lines = Vec::new();
Expand Down
76 changes: 60 additions & 16 deletions codex-rs/cli/tests/workflows__cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ fn write_workflow_source(workflow_dir: &Path) -> Result<()> {
Ok(())
}

fn assert_code_review_autocomplete_metadata(workflow_dir: &Path) -> Result<()> {
fn assert_code_review_static_metadata_without_legacy_usage(workflow_dir: &Path) -> Result<()> {
let workflow_yaml = fs::read_to_string(workflow_dir.join("workflow.yaml"))?;
assert!(
workflow_yaml.contains("id: code-review") || workflow_yaml.contains("id: \"code-review\"")
Expand All @@ -116,14 +116,23 @@ fn assert_code_review_autocomplete_metadata(workflow_dir: &Path) -> Result<()> {
workflow_yaml.contains("command: code-review")
|| workflow_yaml.contains("command: \"code-review\"")
);
assert!(workflow_yaml.contains("usage:"));
assert!(workflow_yaml.contains("options:"));
assert!(workflow_yaml.contains("flag: --action"));
assert!(
workflow_yaml.contains("valueHint: <review|read-report|list-reports|incremental|resume>")
);
assert!(workflow_yaml.contains("flag: --review-id"));
assert!(workflow_yaml.contains("flag: --include-skipped-by-limit"));
assert!(workflow_yaml.contains("title:"));
assert!(workflow_yaml.contains("userDescription:"));
assert!(!workflow_yaml.contains("usage:\n options:"));
for legacy_fragment in [
"read-report",
"list-reports",
"incremental",
"--action",
"--review-id",
"--target-ref",
"--base-ref",
"--review-model",
"--repro-model",
"--include-skipped-by-limit",
] {
assert!(!workflow_yaml.contains(legacy_fragment));
}
Ok(())
}

Expand Down Expand Up @@ -646,11 +655,46 @@ fn workflow_fix_repairs_workflow_without_running_unsupported_fix_action() -> Res
"Repairing workflow code-review with compatibility mode.",
))
.stdout(contains("Updated "))
.stdout(contains("with code-review autocomplete metadata"))
.stdout(contains("with code-review workflow metadata"))
.stdout(contains("code-review repair check completed."));

assert!(!fake_bun.was_invoked());
assert_code_review_autocomplete_metadata(&workflow_dir)?;
assert_code_review_static_metadata_without_legacy_usage(&workflow_dir)?;

Ok(())
}

#[test]
fn workflow_fix_keeps_valid_code_review_workflow_without_usage_options() -> Result<()> {
let codex_home = TempDir::new()?;
let project = TempDir::new()?;
enable_workflows(codex_home.path())?;
let workflow_yaml = r#"id: code-review
command: code-review
title: "/code-review"
userDescription: "Run a code review and repro workflow on the current branch."
"#;
let workflow_dir = write_workflow(
&codex_home.path().join("workflows"),
"code-review",
workflow_yaml,
)?;
write_workflow_source(&workflow_dir)?;

let mut cmd = codex_command(codex_home.path(), project.path())?;
cmd.args(["workflow", "fix", "code-review"])
.assert()
.success()
.stdout(contains(
"No compatibility repairs were needed for code-review.",
))
.stdout(contains("code-review repair check completed."));

assert_eq!(
fs::read_to_string(workflow_dir.join("workflow.yaml"))?,
workflow_yaml
);
assert_code_review_static_metadata_without_legacy_usage(&workflow_dir)?;

Ok(())
}
Expand Down Expand Up @@ -697,11 +741,11 @@ fn workflow_fix_tolerates_broken_metadata_and_source_without_running_workflow()
"Repairing workflow code-review with compatibility mode.",
))
.stdout(contains("Updated "))
.stdout(contains("with code-review autocomplete metadata"))
.stdout(contains("with code-review workflow metadata"))
.stdout(contains("code-review repair check completed."));

assert!(!fake_bun.was_invoked());
assert_code_review_autocomplete_metadata(&workflow_dir)?;
assert_code_review_static_metadata_without_legacy_usage(&workflow_dir)?;

Ok(())
}
Expand Down Expand Up @@ -730,7 +774,7 @@ fn workflow_fix_scaffolds_missing_workflow_source_for_discovery_fallback() -> Re
.stdout(contains("code-review repair check completed."));

assert!(workflow_dir.join("src").join("workflow.ts").is_file());
assert_code_review_autocomplete_metadata(&workflow_dir)?;
assert_code_review_static_metadata_without_legacy_usage(&workflow_dir)?;

Ok(())
}
Expand Down Expand Up @@ -758,11 +802,11 @@ fn workflow_repair_alias_repairs_workflow_without_running_workflow_runtime() ->
"Repairing workflow code-review with compatibility mode.",
))
.stdout(contains("Updated "))
.stdout(contains("with code-review autocomplete metadata"))
.stdout(contains("with code-review workflow metadata"))
.stdout(contains("code-review repair check completed."));

assert!(!fake_bun.was_invoked());
assert_code_review_autocomplete_metadata(&workflow_dir)?;
assert_code_review_static_metadata_without_legacy_usage(&workflow_dir)?;

Ok(())
}
Expand Down
Loading