fix(cli): doctor image check reads compose file and fix API docs URL#625
fix(cli): doctor image check reads compose file and fix API docs URL#625
Conversation
Doctor's image check used tag-based lookup (repo:tag) but images pulled by digest have no local tag (<none>). Now reads the actual compose.yml to get the digest-pinned image refs and checks those directly. Also fixes the API docs link: /api -> /docs/api (Scalar UI path). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR updates CLI diagnostics and doctor output. The doctor "API docs" link now points to /docs/api. collect.go was revised: checkComposeFile returns the resolved compose path, checkImages accepts that path and prefers image references parsed from the compose file (including digest-pinned refs) when inspecting images, falling back to tag-based names only if absent. A parseComposeImageRefs helper and unit tests for compose parsing were added. go.mod was adjusted to make yaml v3 a direct dependency. 🚥 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. 📝 Coding Plan
Comment |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where synthorg doctor couldn't find digest-pinned images by now reading the compose.yml file. It also fixes a broken link to the API documentation. The changes are logical and include relevant tests. I have one suggestion to improve the robustness of the compose file parsing.
| // parseComposeImageRefs extracts image references from a compose file | ||
| // by matching "image: ghcr.io/aureliolo/synthorg-<service>..." lines. | ||
| // Returns a map of service name (backend, web, sandbox) to full image ref. | ||
| func parseComposeImageRefs(composePath string) map[string]string { | ||
| refs := make(map[string]string) | ||
| if composePath == "" { | ||
| return refs | ||
| } | ||
| data, err := os.ReadFile(composePath) | ||
| if err != nil { | ||
| return refs | ||
| } | ||
| for _, line := range strings.Split(string(data), "\n") { | ||
| line = strings.TrimSpace(line) | ||
| if !strings.HasPrefix(line, "image:") { | ||
| continue | ||
| } | ||
| ref := strings.TrimSpace(strings.TrimPrefix(line, "image:")) | ||
| if !strings.HasPrefix(ref, imagePrefix) { | ||
| continue | ||
| } | ||
| // Extract service name: "ghcr.io/aureliolo/synthorg-backend@sha256:..." -> "backend" | ||
| suffix := strings.TrimPrefix(ref, imagePrefix) | ||
| var svc string | ||
| if i := strings.IndexAny(suffix, ":@"); i > 0 { | ||
| svc = suffix[:i] | ||
| } else { | ||
| svc = suffix | ||
| } | ||
| refs[svc] = ref | ||
| } | ||
| return refs | ||
| } |
There was a problem hiding this comment.
Parsing the compose.yml file using string manipulation is fragile and may fail with valid but slightly different YAML formatting (e.g., comments, quotes, or different spacing). Using a proper YAML parser would make this function more robust and maintainable.
This suggestion uses gopkg.in/yaml.v3 to reliably parse the file. You will need to add "gopkg.in/yaml.v3" to your imports.
// parseComposeImageRefs extracts image references from a compose file by parsing it as YAML.
// Returns a map of service name (e.g. backend, web) to full image ref.
func parseComposeImageRefs(composePath string) map[string]string {
type composeFile struct {
Services map[string]struct {
Image string `yaml:"image"`
} `yaml:"services"`
}
refs := make(map[string]string)
if composePath == "" {
return refs
}
data, err := os.ReadFile(composePath)
if err != nil {
return refs // Silently fail on read error to preserve original behavior.
}
var cf composeFile
if err := yaml.Unmarshal(data, &cf); err == nil {
for name, service := range cf.Services {
if service.Image != "" {
refs[name] = service.Image
}
}
}
// Silently fail on parse error to preserve original behavior.
return refs
}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/internal/diagnostics/collect.go`:
- Around line 343-361: The line-based loop that scans strings.Split(data, "\n")
for image: lines is fragile (it misses quoted values, commented lines,
multi-line/anchors); replace this ad-hoc parsing with a YAML-aware approach:
create a parseComposeImageRefs function (or replace the existing loop) that
reads the compose file, unmarshals it into a struct like composeFile { Services
map[string]struct{ Image string `yaml:"image"` } `yaml:"services"` } using
gopkg.in/yaml.v3, then iterate cf.Services and add entries to refs only when
def.Image has the imagePrefix; keep the existing refs map and imagePrefix checks
and return an empty map on read/unmarshal errors.
- Around line 334-342: parseComposeImageRefs currently reads composePath
directly, which triggers a CodeQL path traversal warning; before calling
os.ReadFile validate and canonicalize the input by calling filepath.Clean on
composePath, reject or return empty if the cleaned path contains path traversal
segments (e.g. "..") or otherwise looks suspicious, and optionally verify the
file exists/readable with os.Stat to fail early; update parseComposeImageRefs to
perform these checks on the composePath parameter (use filepath.Clean,
strings.Contains for "..", and os.Stat) before calling os.ReadFile to satisfy
the scanner and add defense-in-depth.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 725c3e44-fcb7-4626-9266-1e2c42747221
📒 Files selected for processing (3)
cli/cmd/doctor.gocli/internal/diagnostics/collect.gocli/internal/diagnostics/collect_test.go
…tion Replace fragile line-based string scanning with proper YAML unmarshaling (go.yaml.in/yaml/v3) for extracting image refs from compose files. Add filepath.Clean + path traversal rejection before os.ReadFile. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/internal/diagnostics/collect.go (1)
257-276:⚠️ Potential issue | 🟠 MajorReorder
composeFileNamesto match Docker Compose's official file precedence.The current
composeFileNamesarray preferscompose.ymlovercompose.yaml, but Docker Compose's documented preference is the opposite. This creates a bug: when both files exist in the same directory,docker compose config --quietvalidates Docker's preferredcompose.yaml, but the function returns the path tocompose.yml. Downstream parsing then operates on the unvalidated file, potentially missing validation errors or analyzing a different configuration than what Docker Compose would use.Fix: Swap the order to match Docker Compose
var composeFileNames = []string{ - "compose.yml", "compose.yaml", + "compose.yaml", "compose.yml", "docker-compose.yml", "docker-compose.yaml", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/diagnostics/collect.go` around lines 257 - 276, The composeFileNames ordering is wrong so checkComposeFile may return compose.yml while Docker Compose prefers compose.yaml; update the composeFileNames slice (used by checkComposeFile) to list "compose.yaml" before "compose.yml" so the loop in checkComposeFile validates and returns the same filename Docker Compose would pick, ensuring docker.ComposeExec validation applies to the returned path and downstream parsing uses the validated file.
🤖 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/internal/diagnostics/collect.go`:
- Around line 368-380: The loop that builds composeRefs discards the original
Compose YAML key and reconstructs a key from the image repo suffix, causing
lookups like checkImages to miss aliased services; change the iteration to
capture the service key (e.g., for svcName, svc := range cf.Services) and use
that svcName as the map key (refs[svcName] = svc.Image) when the image matches
imagePrefix so the original Compose service name (aliases) is preserved for
lookup in functions like checkImages.
---
Outside diff comments:
In `@cli/internal/diagnostics/collect.go`:
- Around line 257-276: The composeFileNames ordering is wrong so
checkComposeFile may return compose.yml while Docker Compose prefers
compose.yaml; update the composeFileNames slice (used by checkComposeFile) to
list "compose.yaml" before "compose.yml" so the loop in checkComposeFile
validates and returns the same filename Docker Compose would pick, ensuring
docker.ComposeExec validation applies to the returned path and downstream
parsing uses the validated file.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 995b7aa1-04f1-423b-9b65-42f89f5e5f2c
📒 Files selected for processing (3)
cli/go.modcli/internal/diagnostics/collect.gocli/internal/diagnostics/collect_test.go
Use the YAML service name as the map key in parseComposeImageRefs instead of reconstructing it from the image repo suffix. Simpler and preserves aliased service names. Reorder composeFileNames to match Docker Compose's documented preference: .yaml before .yml. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 I have created a release *beep* *boop* --- ## [0.3.10](v0.3.9...v0.3.10) (2026-03-20) ### Bug Fixes * **ci:** generate required secrets in DAST workflow ([#623](#623)) ([6ae297f](6ae297f)) * **cli:** doctor image check reads compose file and fix API docs URL ([#625](#625)) ([5202e53](5202e53)) * **engine:** sanitize error messages in checkpoint reconciliation and compaction summaries ([#632](#632)) ([5394ed7](5394ed7)) * mitigate TOCTOU DNS rebinding gap in git clone SSRF prevention ([#633](#633)) ([1846f6e](1846f6e)) * resolve post-startup log loss, add provider model discovery, and improve setup wizard UX ([#634](#634)) ([2df8d11](2df8d11)) ### Maintenance * bump https://github.com/astral-sh/ruff-pre-commit from v0.15.6 to 0.15.7 ([#628](#628)) ([c641d2c](c641d2c)) * bump python from `584e89d` to `fb83750` in /docker/backend ([#627](#627)) ([1a36eca](1a36eca)) * bump python from `584e89d` to `fb83750` in /docker/sandbox ([#629](#629)) ([fd3e69a](fd3e69a)) * bump the minor-and-patch group across 2 directories with 3 updates ([#630](#630)) ([67d14c4](67d14c4)) * bump the minor-and-patch group with 2 updates ([#631](#631)) ([2e51b60](2e51b60)) * **ci:** add timeout-minutes, harden fuzz script, extend CVE audit ([#626](#626)) ([25420e2](25420e2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
synthorg doctorshowed images as "not found locally" because it checked by tag (repo:0.3.9) but images are pulled by digest (no local tag)compose.ymlto get digest-pinned image refs and checks those directly/api->/docs/api(correct Scalar UI path)Test plan
synthorg doctorshows images as "available" when pulled by digestTestParseComposeImageRefs*tests pass (4 cases: digest, tag, empty path, missing file)go vet+go test+golangci-lintall pass🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores