Feat: validate .opencode structure and colorize status tags#17
Feat: validate .opencode structure and colorize status tags#17
Conversation
📝 WalkthroughWalkthroughRemoved ten Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Doctor as CLI Doctor
participant FS as Filesystem (.opencode)
participant Reporter as Reporter/Formatter
participant Stdout as Output
User->>CLI_Doctor: run doctor command
CLI_Doctor->>FS: inspect repository root & .opencode/{agent,command,skills}
FS-->>CLI_Doctor: presence / missing responses
CLI_Doctor->>Reporter: build TaggedLine entries (StatusTag + message)
Reporter-->>CLI_Doctor: formatted lines (color-aware / NO_COLOR)
CLI_Doctor->>Stdout: print status-tagged report lines
Stdout-->>User: visible report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@cli/src/services/doctor.rs`:
- Around line 1465-1470: format_tagged_lines currently prefixes only the first
physical line of each TaggedLine, so embedded newlines in DoctorProblem.summary
or DoctorFixResultRecord.detail lose their status tag; update
format_tagged_lines to split each TaggedLine.text on newline boundaries (or
sanitize embedded newlines) and emit a prefixed line for every physical line
using status_tag_prefix(line.tag) for each split piece, then join with "\n";
also add a regression test exercising a TaggedLine built from a multiline
DoctorProblem.summary/DoctorFixResultRecord.detail to ensure every physical line
is tagged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e313baa5-df2d-44fc-b532-ee89cd1eae41
⛔ Files ignored due to path filters (3)
context/plans/doctor-opencode-structure-status-tags.mdis excluded by!context/**/*.mdcontext/plans/doctor-status-colors.mdis excluded by!context/**/*.mdcontext/sce/agent-trace-hook-doctor.mdis excluded by!context/**/*.md
📒 Files selected for processing (11)
.opencode/skills/sce-atomic-commit/tile.json.opencode/skills/sce-bootstrap-context/tile.json.opencode/skills/sce-context-sync/tile.json.opencode/skills/sce-drift-analyzer/tile.json.opencode/skills/sce-drift-fixer/tile.json.opencode/skills/sce-handover-writer/tile.json.opencode/skills/sce-plan-authoring/tile.json.opencode/skills/sce-plan-review/tile.json.opencode/skills/sce-task-execution/tile.json.opencode/skills/sce-validation/tile.jsoncli/src/services/doctor.rs
💤 Files with no reviewable changes (10)
- .opencode/skills/sce-drift-fixer/tile.json
- .opencode/skills/sce-validation/tile.json
- .opencode/skills/sce-plan-authoring/tile.json
- .opencode/skills/sce-context-sync/tile.json
- .opencode/skills/sce-drift-analyzer/tile.json
- .opencode/skills/sce-task-execution/tile.json
- .opencode/skills/sce-plan-review/tile.json
- .opencode/skills/sce-handover-writer/tile.json
- .opencode/skills/sce-bootstrap-context/tile.json
- .opencode/skills/sce-atomic-commit/tile.json
…unique color for each status Co-authored-by: SCE <sce@crocoder.dev>
Co-authored-by: SCE <sce@crocoder.dev>
ivke995
force-pushed
the
feat/opencode-content-doctor-check
branch
from
388184d to
057982e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cli/src/services/doctor.rs (2)
1485-1490:⚠️ Potential issue | 🟡 MinorPrefix every physical line.
DoctorProblem.summaryandDoctorFixResultRecord.detailare free-form strings. If either contains\n, only the first physical line keeps a status tag and the continuation lines break the new tagged-output contract. Add one regression case with a multiline summary/detail too.Suggested fix
fn format_tagged_lines(lines: Vec<TaggedLine>) -> String { lines .into_iter() - .map(|line| format!("{} {}", status_tag_prefix(line.tag), line.text)) + .flat_map(|line| { + let prefix = status_tag_prefix(line.tag); + line.text + .lines() + .map(move |text| format!("{prefix} {text}")) + .collect::<Vec<_>>() + }) .collect::<Vec<_>>() .join("\n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/services/doctor.rs` around lines 1485 - 1490, format_tagged_lines currently prefixes only the first physical line of each TaggedLine, so multiline values from DoctorProblem.summary or DoctorFixResultRecord.detail lose their status tags; update format_tagged_lines to split each line.text on '\n', prefix each physical line with status_tag_prefix(line.tag), then rejoin them preserving original newlines, and add a regression test that constructs a DoctorProblem and a DoctorFixResultRecord with multiline summary/detail to assert every physical line is tagged.
847-905:⚠️ Potential issue | 🟠 MajorCheck directory listability, not just metadata.
fs::metadata()can succeed for an existing directory that the process still cannot list, so.opencode/{agent,command,skills}can be reported as healthy even when the doctor cannot actually inspect them. Afteris_dir(), probefs::read_dir()and surfacePermissionDeniedthrough the sameRepoAssetspath.Does `std::fs::metadata` on a directory prove the directory is readable/listable, or is `std::fs::read_dir` required to detect an existing but unreadable directory in Rust?Suggested fix
match fs::metadata(&required_path) { Ok(metadata) => { if !metadata.is_dir() { problems.push(DoctorProblem { category: ProblemCategory::RepoAssets, severity: ProblemSeverity::Error, fixability: ProblemFixability::ManualOnly, summary: format!( "OpenCode required directory '{}' is not a directory.", required_path.display() ), remediation: format!( "Reinstall OpenCode assets so '{}' includes the required '{}' directory, then rerun 'sce doctor'.", opencode_root.display(), directory ), next_action: "manual_steps", }); + } else if let Err(error) = fs::read_dir(&required_path) { + problems.push(DoctorProblem { + category: ProblemCategory::RepoAssets, + severity: ProblemSeverity::Error, + fixability: ProblemFixability::ManualOnly, + summary: format!( + "OpenCode required directory '{}' is not readable: {error}", + required_path.display() + ), + remediation: format!( + "Verify that '{}' is readable and rerun 'sce doctor'.", + required_path.display() + ), + next_action: "manual_steps", + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/services/doctor.rs` around lines 847 - 905, inspect_opencode_required_directories currently uses fs::metadata and only checks is_dir(), which misses directories that exist but cannot be listed; after confirming metadata.is_dir() (for entries in OPENCODE_REQUIRED_DIRECTORIES) call fs::read_dir(&required_path) and handle errors: if read_dir returns Err(e) with e.kind() == std::io::ErrorKind::PermissionDenied, push a DoctorProblem (same category ProblemCategory::RepoAssets, severity ProblemSeverity::Error, fixability ProblemFixability::ManualOnly) with a summary/remediation like the NotFound/inspect error cases but indicating the directory is not readable/listable, and otherwise proceed as before when read_dir is Ok.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/services/doctor.rs`:
- Around line 2377-2386: The two test fixtures constructing DoctorDependencies
(the literal at lines shown and the second one around the later block) are
missing the required resolve_global_config_path field; add
resolve_global_config_path: &|| Ok(some_path_buf) (i.e., a closure that returns
Result<PathBuf>) to each literal so the struct matches the definition; locate
the DoctorDependencies instances near
run_git_command/resolve_state_root/resolve_agent_trace_local_db_path and insert
resolve_global_config_path returning an appropriate PathBuf (or a test temp
path) in both fixtures.
---
Duplicate comments:
In `@cli/src/services/doctor.rs`:
- Around line 1485-1490: format_tagged_lines currently prefixes only the first
physical line of each TaggedLine, so multiline values from DoctorProblem.summary
or DoctorFixResultRecord.detail lose their status tags; update
format_tagged_lines to split each line.text on '\n', prefix each physical line
with status_tag_prefix(line.tag), then rejoin them preserving original newlines,
and add a regression test that constructs a DoctorProblem and a
DoctorFixResultRecord with multiline summary/detail to assert every physical
line is tagged.
- Around line 847-905: inspect_opencode_required_directories currently uses
fs::metadata and only checks is_dir(), which misses directories that exist but
cannot be listed; after confirming metadata.is_dir() (for entries in
OPENCODE_REQUIRED_DIRECTORIES) call fs::read_dir(&required_path) and handle
errors: if read_dir returns Err(e) with e.kind() ==
std::io::ErrorKind::PermissionDenied, push a DoctorProblem (same category
ProblemCategory::RepoAssets, severity ProblemSeverity::Error, fixability
ProblemFixability::ManualOnly) with a summary/remediation like the
NotFound/inspect error cases but indicating the directory is not
readable/listable, and otherwise proceed as before when read_dir is Ok.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96828b67-9ec7-472a-a4cc-2b00c892c976
⛔ Files ignored due to path filters (4)
context/plans/doctor-opencode-structure-status-tags.mdis excluded by!context/**/*.mdcontext/plans/doctor-status-color-no-color.mdis excluded by!context/**/*.mdcontext/plans/doctor-status-colors.mdis excluded by!context/**/*.mdcontext/sce/agent-trace-hook-doctor.mdis excluded by!context/**/*.md
📒 Files selected for processing (12)
.opencode/skills/sce-atomic-commit/tile.json.opencode/skills/sce-bootstrap-context/tile.json.opencode/skills/sce-context-sync/tile.json.opencode/skills/sce-drift-analyzer/tile.json.opencode/skills/sce-drift-fixer/tile.json.opencode/skills/sce-handover-writer/tile.json.opencode/skills/sce-plan-authoring/tile.json.opencode/skills/sce-plan-review/tile.json.opencode/skills/sce-task-execution/tile.json.opencode/skills/sce-validation/tile.jsoncli/src/services/doctor.rscli/src/services/style.rs
💤 Files with no reviewable changes (10)
- .opencode/skills/sce-atomic-commit/tile.json
- .opencode/skills/sce-plan-authoring/tile.json
- .opencode/skills/sce-validation/tile.json
- .opencode/skills/sce-drift-analyzer/tile.json
- .opencode/skills/sce-bootstrap-context/tile.json
- .opencode/skills/sce-task-execution/tile.json
- .opencode/skills/sce-drift-fixer/tile.json
- .opencode/skills/sce-context-sync/tile.json
- .opencode/skills/sce-plan-review/tile.json
- .opencode/skills/sce-handover-writer/tile.json
…ctable tests Co-authored-by: SCE <sce@crocoder.dev>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cli/src/services/doctor.rs (2)
1485-1490:⚠️ Potential issue | 🟡 MinorTag every physical line, not every
TaggedLine.A multiline
DoctorProblem.summaryorDoctorFixResultRecord.detailloses its prefix on continuation lines here, which breaks the new tagged-output contract.Suggested fix
fn format_tagged_lines(lines: Vec<TaggedLine>) -> String { lines .into_iter() - .map(|line| format!("{} {}", status_tag_prefix(line.tag), line.text)) + .flat_map(|line| { + let prefix = status_tag_prefix(line.tag); + line.text + .lines() + .map(move |text| format!("{prefix} {text}")) + .collect::<Vec<_>>() + }) .collect::<Vec<_>>() .join("\n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/services/doctor.rs` around lines 1485 - 1490, format_tagged_lines currently prefixes only the first physical line of each TaggedLine, so multiline fields like DoctorProblem.summary and DoctorFixResultRecord.detail lose their tags on continuation lines; modify format_tagged_lines to split each TaggedLine.text on newline and emit a separate prefixed physical line for each segment (use status_tag_prefix(line.tag) for every physical line), then join all resulting lines with "\n" so every physical line is tagged consistently.
847-905:⚠️ Potential issue | 🟠 MajorProbe directory listability after
metadata().
fs::metadata()only proves existence/type. A permission-locked.opencode/agent,.opencode/command, or.opencode/skillsdirectory still passes this branch, so doctor can report the structure as healthy even though the directory cannot be read. Afteris_dir(), callfs::read_dir(&required_path)and mapPermissionDeniedthrough the sameRepoAssetsproblem path; add one unreadable-directory regression test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/services/doctor.rs` around lines 847 - 905, inspect_opencode_required_directories currently uses fs::metadata to check existence/type but doesn’t verify listability, so permission-locked directories (e.g., entries in OPENCODE_REQUIRED_DIRECTORIES such as ".opencode/agent", ".opencode/command", ".opencode/skills") appear healthy; after the metadata.is_dir() check in inspect_opencode_required_directories, call fs::read_dir(&required_path) and handle Err(e) where e.kind() == std::io::ErrorKind::PermissionDenied by pushing the same DoctorProblem (ProblemCategory::RepoAssets, severity Error, fixability ManualOnly, appropriate summary/remediation/next_action) used for missing/unreadable directories, and add a regression test that creates an unreadable required directory and asserts the doctor reports the unreadable-directory problem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/services/doctor.rs`:
- Around line 1309-1318: The hook classification currently matches on the
rendered string causing executable but uninspectable hooks to be treated as
"ok"; update the logic in tag_for_hook()/hook_state() to match on the underlying
enum/fields (e.g., inspect_hook_content_state() / HookContentState) instead of
the formatted text, and change hook_state() so the HookContentState::Unknown
variant explicitly returns "unknown" (not "ok"); ensure tag_for_hook() uses that
machine-level value to emit [FAIL]/[PASS]/[UNKNOWN] accordingly so unreadable
hooks are classified as failures/unknowns rather than passing.
- Around line 2482-2508: The assertions hardcode forward slashes (e.g.
".opencode/agent") which fails on Windows because the summary uses
required_path.display() (backslashes on Windows); update the tests that call
problem_matches to compare paths in a platform-agnostic way—either construct the
expected fragment from the same required_path (e.g. use
required_path.display()/to_string_lossy()) or split/compare path components
(Path/PathBuf ends_with or iterating components) instead of string matching with
"/" so the checks for ".opencode/agent", ".opencode/command", and
".opencode/skills" succeed on all OSes.
- Around line 2191-2207: The test
doctor_text_output_disables_prefix_colors_when_no_color_set is vacuous and racy
because it manipulates NO_COLOR and inspects formatted output (via
format_execution) while stdout() isn't a terminal; instead remove this
high-level test and add focused unit tests for supports_color() and
style_if_enabled() in style/tests.rs (or add a deterministic seam/mocking for
is_terminal()) to exercise the NO_COLOR branch and color enabling logic
directly; update or delete the
doctor_text_output_disables_prefix_colors_when_no_color_set test and create new
tests that call supports_color() and style_if_enabled() (or their equivalents)
so the behavior is verified deterministically without changing global env in
concurrent test runs.
---
Duplicate comments:
In `@cli/src/services/doctor.rs`:
- Around line 1485-1490: format_tagged_lines currently prefixes only the first
physical line of each TaggedLine, so multiline fields like DoctorProblem.summary
and DoctorFixResultRecord.detail lose their tags on continuation lines; modify
format_tagged_lines to split each TaggedLine.text on newline and emit a separate
prefixed physical line for each segment (use status_tag_prefix(line.tag) for
every physical line), then join all resulting lines with "\n" so every physical
line is tagged consistently.
- Around line 847-905: inspect_opencode_required_directories currently uses
fs::metadata to check existence/type but doesn’t verify listability, so
permission-locked directories (e.g., entries in OPENCODE_REQUIRED_DIRECTORIES
such as ".opencode/agent", ".opencode/command", ".opencode/skills") appear
healthy; after the metadata.is_dir() check in
inspect_opencode_required_directories, call fs::read_dir(&required_path) and
handle Err(e) where e.kind() == std::io::ErrorKind::PermissionDenied by pushing
the same DoctorProblem (ProblemCategory::RepoAssets, severity Error, fixability
ManualOnly, appropriate summary/remediation/next_action) used for
missing/unreadable directories, and add a regression test that creates an
unreadable required directory and asserts the doctor reports the
unreadable-directory problem.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: acc92906-761d-45f3-a592-caad0713b55c
📒 Files selected for processing (1)
cli/src/services/doctor.rs
| lines.push(TaggedLine { | ||
| tag: tag_for_hook(hook), | ||
| text: format!( | ||
| " {}: {} (content={}, executable={}) {}", | ||
| value(hook.name), | ||
| value(hook_state(hook)), | ||
| value(hook_content_state(hook.content_state)), | ||
| value(if hook.executable { "yes" } else { "no" }), | ||
| value(&hook.path.display().to_string()) | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 114
🏁 Script executed:
# First, let's see the current implementation around the specified lines
sed -n '1300,1325p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 844
🏁 Script executed:
# Let's also check lines 1534-1539
sed -n '1530,1545p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 470
🏁 Script executed:
# Find the tag_for_hook function definition
rg -n "fn tag_for_hook" cli/src/services/doctor.rs -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 397
🏁 Script executed:
# Find the hook_state function definition
rg -n "fn hook_state" cli/src/services/doctor.rs -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 402
🏁 Script executed:
# Find the HookContentState enum definition
rg -n "enum HookContentState" cli/src/services/doctor.rs -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 314
🏁 Script executed:
# Find HookFileHealth struct definition
rg -n "struct HookFileHealth" cli/src/services/doctor.rs -A 15Repository: crocoder-dev/shared-context-engineering
Length of output: 415
🏁 Script executed:
# Find where HookContentState::Unknown is assigned
rg -n "HookContentState::Unknown" cli/src/services/doctor.rs -B 2 -A 2Repository: crocoder-dev/shared-context-engineering
Length of output: 570
🏁 Script executed:
# Find inspect_hook_content_state function
rg -n "fn inspect_hook_content_state" cli/src/services/doctor.rs -A 15Repository: crocoder-dev/shared-context-engineering
Length of output: 557
🏁 Script executed:
# Check if hook_state is called elsewhere and how it's used with tag_for_hook
rg -n "hook_state(" cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 155
🏁 Script executed:
# Verify the match pattern suggestion covers all enum variants
ast-grep --pattern 'enum HookContentState { $$$ }'Repository: crocoder-dev/shared-context-engineering
Length of output: 344
Classify unreadable hooks as failures.
The current implementation has a bug: when inspect_hook_content_state() returns HookContentState::Unknown (e.g., due to permission errors) and a hook exists and is executable, hook_state() falls through to returning "ok", causing tag_for_hook() to emit [PASS] for an uninspectable hook. Match on the underlying fields instead of the rendered string, and update hook_state() to explicitly surface "unknown" for the Unknown variant.
Suggested fix
fn tag_for_hook(hook: &HookFileHealth) -> StatusTag {
- if hook_state(hook) == "ok" {
- StatusTag::Pass
- } else {
- StatusTag::Fail
- }
+ match (hook.exists, hook.content_state, hook.executable) {
+ (true, HookContentState::Current, true) => StatusTag::Pass,
+ _ => StatusTag::Fail,
+ }
}Also update hook_state() to add:
} else if !hook.executable {
"not_executable"
+} else if hook.content_state == HookContentState::Unknown {
+ "unknown"
} else {
"ok"
}As per coding guidelines, "Separate machine classification from rendered messages when the CLI contract cares about exit codes in Rust."
Also applies to: 1534-1539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/doctor.rs` around lines 1309 - 1318, The hook classification
currently matches on the rendered string causing executable but uninspectable
hooks to be treated as "ok"; update the logic in tag_for_hook()/hook_state() to
match on the underlying enum/fields (e.g., inspect_hook_content_state() /
HookContentState) instead of the formatted text, and change hook_state() so the
HookContentState::Unknown variant explicitly returns "unknown" (not "ok");
ensure tag_for_hook() uses that machine-level value to emit
[FAIL]/[PASS]/[UNKNOWN] accordingly so unreadable hooks are classified as
failures/unknowns rather than passing.
| fn doctor_text_output_disables_prefix_colors_when_no_color_set() { | ||
| let previous = std::env::var("NO_COLOR").ok(); | ||
| std::env::set_var("NO_COLOR", "1"); | ||
| let execution = DoctorExecution { | ||
| report: base_report(DoctorMode::Diagnose, Readiness::Ready), | ||
| fix_results: Vec::new(), | ||
| }; | ||
| let output = super::format_execution(&execution); | ||
|
|
||
| match previous { | ||
| Some(value) => std::env::set_var("NO_COLOR", value), | ||
| None => std::env::remove_var("NO_COLOR"), | ||
| } | ||
|
|
||
| assert_all_lines_tagged(&output); | ||
| assert!(output.contains("[PASS] ")); | ||
| assert!(!output.contains("\u{1b}[")); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test in question
sed -n '2191,2207p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 762
🏁 Script executed:
# Find the supports_color function
rg "supports_color" cli/src/services/doctor.rs -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Search for stdout().is_terminal() usage
rg "is_terminal" cli/src/services/doctor.rs -B 3 -A 3Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Look for any synchronization mechanisms for env vars
rg "static.*Mutex|MUTEX" cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Find the base_report function to understand test setup
rg "fn base_report" cli/src/services/doctor.rs -A 5Repository: crocoder-dev/shared-context-engineering
Length of output: 344
🏁 Script executed:
# Find format_execution function
rg "fn format_execution" cli/src/services/doctor.rs -A 15Repository: crocoder-dev/shared-context-engineering
Length of output: 743
🏁 Script executed:
# Search for color-related functions in the file
rg "color|Color|supports" cli/src/services/doctor.rs | head -20Repository: crocoder-dev/shared-context-engineering
Length of output: 153
🏁 Script executed:
# Check what assert_all_lines_tagged does
rg "fn assert_all_lines_tagged" cli/src/ -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 836
🏁 Script executed:
# Look at the entire test module and surrounding code to understand context
wc -l cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 114
🏁 Script executed:
# Look for how TaggedLine is converted to string with color codes
rg "impl.*Display|fn.*render" cli/src/services/doctor.rs -B 2 -A 8Repository: crocoder-dev/shared-context-engineering
Length of output: 2435
🏁 Script executed:
# Find where NO_COLOR is checked
rg "NO_COLOR" cli/src/services/doctor.rs -B 3 -A 3Repository: crocoder-dev/shared-context-engineering
Length of output: 676
🏁 Script executed:
# Look for strip_ansi_codes function
rg "fn strip_ansi_codes" cli/src/services/doctor.rs -A 8Repository: crocoder-dev/shared-context-engineering
Length of output: 497
🏁 Script executed:
# Find how the output with color codes is generated - look for ANSI escape sequences
rg '1b\[|\\033|ansi|Color|color' cli/src/services/doctor.rs | head -30Repository: crocoder-dev/shared-context-engineering
Length of output: 411
🏁 Script executed:
# Search the entire cli/src directory for supports_color definition
rg "fn supports_color" cli/src/ -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 1707
🏁 Script executed:
# Look at style_if_enabled and how it's used
rg "style_if_enabled" cli/src/services/style.rs -B 2 -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 2377
🏁 Script executed:
# See the full style.rs context around color support
head -100 cli/src/services/style.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 2088
🏁 Script executed:
# Find how TaggedLine is rendered to string with or without colors
rg "TaggedLine" cli/src/services/doctor.rs -B 2 -A 5 | head -50Repository: crocoder-dev/shared-context-engineering
Length of output: 1185
🏁 Script executed:
# Look for where color codes are actually added to output
rg "format_line|render.*line|impl.*fmt" cli/src/services/doctor.rs -B 2 -A 8Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check the with_no_color helper in style tests to see how they handle synchronization
rg "fn with_no_color|with_no_color" cli/src/services/style/ -B 2 -A 15Repository: crocoder-dev/shared-context-engineering
Length of output: 9287
🏁 Script executed:
# Find how TaggedLine vector is converted to the final output string
rg "TaggedLine" cli/src/services/doctor.rs -A 3 | grep -E "format_execution|output|String|join|lines"Repository: crocoder-dev/shared-context-engineering
Length of output: 1032
🏁 Script executed:
# Look for the actual rendering of TaggedLine to output
sed -n '2100,2150p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 2002
🏁 Script executed:
# Find where lines are joined into output
rg "iter\(\).*join\|format_execution.*String" cli/src/services/doctor.rs -B 3 -A 8Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check the complete format_execution function
rg "fn format_execution" cli/src/services/doctor.rs -A 30Repository: crocoder-dev/shared-context-engineering
Length of output: 1149
🏁 Script executed:
# Get the full format_tagged_lines function
rg "fn format_tagged_lines" cli/src/services/doctor.rs -A 20Repository: crocoder-dev/shared-context-engineering
Length of output: 750
🏁 Script executed:
# Also look for StatusTag to understand how it renders
rg "enum StatusTag\|impl.*StatusTag" cli/src/services/doctor.rs -A 10Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check if there's any test configuration for parallel execution
rg "test.*parallel\|--test-threads\|Cargo.toml" cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Look at the entire test to understand what assert_all_lines_tagged does
sed -n '2185,2210p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 951
🏁 Script executed:
# Check if there are other color-related tests that might race
rg "NO_COLOR\|set_var\|env.*var" cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check the style tests to see how they handle env mutation
cat cli/src/services/style/tests.rs | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 2425
🏁 Script executed:
# Check if there's any mocking or test infrastructure for is_terminal
rg "is_terminal\|mock\|Mock" cli/src/services/ --type rustRepository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check Cargo.toml to see test configuration
find . -name "Cargo.toml" -type f -exec grep -l "test\|parallel" {} \;Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Look at how other doctor tests are structured
rg "#\[test\]" cli/src/services/doctor.rs | head -20Repository: crocoder-dev/shared-context-engineering
Length of output: 286
🏁 Script executed:
# Let me understand the exact issue - check if there are other tests that manipulate NO_COLOR
rg "set_var.*NO_COLOR\|env::var" cli/src/services/doctor.rs -B 2 -A 5Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Check if there's any synchronization primitive being used anywhere
rg "static.*Mutex\|MUTEX\|Arc\|RwLock" cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Get full context of the test assertions
sed -n '2191,2220p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 1320
Remove or replace this test with direct unit tests of the style helpers.
The test is both vacuous and subject to race conditions:
-
Vacuous:
supports_color()checksNO_COLORfirst, but in test contextstdout().is_terminal()returnsfalse, so the test passes regardless of whether theNO_COLORbranch works. Breaking theNO_COLORlogic wouldn't fail this test. -
Racey: Setting/restoring process-global environment state without synchronization means parallel tests can interfere with each other. Test
style_if_enabled_returns_plain_text_when_no_color()instyle/tests.rsexercises the same concern and should also be protected.
Instead, test style_if_enabled() and supports_color() directly in style/tests.rs, or use a deterministic seam (e.g., mock is_terminal()) rather than relying on captured output in a higher-level test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/doctor.rs` around lines 2191 - 2207, The test
doctor_text_output_disables_prefix_colors_when_no_color_set is vacuous and racy
because it manipulates NO_COLOR and inspects formatted output (via
format_execution) while stdout() isn't a terminal; instead remove this
high-level test and add focused unit tests for supports_color() and
style_if_enabled() in style/tests.rs (or add a deterministic seam/mocking for
is_terminal()) to exercise the NO_COLOR branch and color enabling logic
directly; update or delete the
doctor_text_output_disables_prefix_colors_when_no_color_set test and create new
tests that call supports_color() and style_if_enabled() (or their equivalents)
so the behavior is verified deterministically without changing global env in
concurrent test runs.
| assert!(problems.iter().any(|problem| { | ||
| problem_matches( | ||
| problem, | ||
| "repo_assets", | ||
| "error", | ||
| "manual_only", | ||
| ".opencode/agent", | ||
| ) | ||
| })); | ||
| assert!(problems.iter().any(|problem| { | ||
| problem_matches( | ||
| problem, | ||
| "repo_assets", | ||
| "error", | ||
| "manual_only", | ||
| ".opencode/command", | ||
| ) | ||
| })); | ||
| assert!(problems.iter().any(|problem| { | ||
| problem_matches( | ||
| problem, | ||
| "repo_assets", | ||
| "error", | ||
| "manual_only", | ||
| ".opencode/skills", | ||
| ) | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific lines mentioned in the review
sed -n '2482,2508p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 838
🏁 Script executed:
# Let's also look at more context around those lines to understand the test
sed -n '2450,2530p' cli/src/services/doctor.rsRepository: crocoder-dev/shared-context-engineering
Length of output: 3134
🏁 Script executed:
# Find the problem_matches function to understand what it does
rg -n "fn problem_matches" cli/src/services/doctor.rs -A 15Repository: crocoder-dev/shared-context-engineering
Length of output: 643
🏁 Script executed:
# Search for where problem.summary is set to understand its structure
rg -n "problem\.summary\s*=" cli/src/services/doctor.rs -B 3 -A 3Repository: crocoder-dev/shared-context-engineering
Length of output: 406
🏁 Script executed:
# Look for the Problem struct definition to understand its fields
rg -n "struct Problem|impl.*Problem" cli/src/services/doctor.rs -A 10 | head -50Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Get the complete problem_matches function
rg -n "fn problem_matches" cli/src/services/doctor.rs -A 25Repository: crocoder-dev/shared-context-engineering
Length of output: 1048
🏁 Script executed:
# Search for where problems are created with summary containing paths
rg -n "summary.*\.opencode\|\"\.opencode" cli/src/services/doctor.rs -B 5 -A 5Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Look for Problem struct and how summary is constructed
rg -n "Problem\s*{|summary:" cli/src/services/doctor.rs -B 2 -A 2 | head -80Repository: crocoder-dev/shared-context-engineering
Length of output: 196
🏁 Script executed:
# Search for Path::display or similar that might create the path strings
rg -n "Path.*display\|display.*Path\|\.to_string\(\)" cli/src/services/doctor.rs -B 3 -A 3 | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 65
🏁 Script executed:
# Search for where the "repo_assets" problem category is created
rg -n "repo_assets" cli/src/services/doctor.rs -B 5 -A 5 | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 2966
🏁 Script executed:
# Search for where the .opencode paths appear in problem generation
rg -n "\.opencode" cli/src/services/doctor.rs -B 3 -A 3Repository: crocoder-dev/shared-context-engineering
Length of output: 2616
🏁 Script executed:
# Look for the DoctorReport struct and how problems are built
rg -n "struct.*Report|impl.*Report" cli/src/services/doctor.rs -A 15 | head -80Repository: crocoder-dev/shared-context-engineering
Length of output: 678
🏁 Script executed:
# Search for Problem creation with summary field
rg -n 'Problem\s*\{|"summary":|summary:\s*' cli/src/services/doctor.rs -B 2 -A 2 | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 5320
🏁 Script executed:
# Search for where missing opencode directories are detected as errors
rg -n "missing|not found|does not exist" cli/src/services/doctor.rs | grep -i opencodeRepository: crocoder-dev/shared-context-engineering
Length of output: 938
🏁 Script executed:
# Search for "agent\|command\|skills" pattern in error generation
rg -n "agent|command|skills" cli/src/services/doctor.rs | head -50Repository: crocoder-dev/shared-context-engineering
Length of output: 3445
🏁 Script executed:
# Look for check_opencode or similar function that validates opencode structure
rg -n "fn.*opencode|opencode.*fn" cli/src/services/doctor.rs -i -A 20 | head -100Repository: crocoder-dev/shared-context-engineering
Length of output: 4326
🏁 Script executed:
# Search for where RepoAssets problems are created
rg -n "RepoAssets" cli/src/services/doctor.rs -B 5 -A 5Repository: crocoder-dev/shared-context-engineering
Length of output: 4848
Hardcoded / in path assertions will fail on Windows.
Lines 2482-2508 assert that the problem summary contains fragments like .opencode/agent, but the actual summary is built using required_path.display() (line 876), which produces backslashes on Windows (\.opencode\agent). This test will fail on Windows runners even when the behavior is correct. Normalize path separators in the summary or assert on path components instead of slash-based string fragments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/services/doctor.rs` around lines 2482 - 2508, The assertions hardcode
forward slashes (e.g. ".opencode/agent") which fails on Windows because the
summary uses required_path.display() (backslashes on Windows); update the tests
that call problem_matches to compare paths in a platform-agnostic way—either
construct the expected fragment from the same required_path (e.g. use
required_path.display()/to_string_lossy()) or split/compare path components
(Path/PathBuf ends_with or iterating components) instead of string matching with
"/" so the checks for ".opencode/agent", ".opencode/command", and
".opencode/skills" succeed on all OSes.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores