Skip to content

Add doctor CI environment diagnostics#27

Merged
jmcte merged 4 commits intomainfrom
codex/issue-12-doctor-ci-checks
May 7, 2026
Merged

Add doctor CI environment diagnostics#27
jmcte merged 4 commits intomainfrom
codex/issue-12-doctor-ci-checks

Conversation

@jmcte
Copy link
Copy Markdown
Contributor

@jmcte jmcte commented May 1, 2026

Summary

  • add structured ciDiagnostics to apw doctor for Xcode, Cargo, detect-secrets, Developer ID signing identity, and CI runner metadata
  • include OK/WARN/FAIL statuses with remediation hints for each diagnostic
  • document apw --json doctor as the first runner/local environment readiness check

Closes #12

Validation

  • cargo test --manifest-path rust/Cargo.toml doctor_creates_default_credentials_file -- --nocapture
  • cargo fmt --manifest-path rust/Cargo.toml --check
  • bash scripts/ci/run-fast-checks.sh
  • cargo test --manifest-path rust/Cargo.toml (outside sandbox for socket/process-heavy tests)
  • git diff --check

Risks / Notes

  • GitHub does not expose runner labels by default, so doctor reports a WARN in CI unless RUNNER_LABELS or APW_RUNNER_LABELS is provided.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 273d36592a

ℹ️ 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".

Comment thread rust/src/native_app.rs
Copy link
Copy Markdown

@athena-omt athena-omt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the structured ciDiagnostics payload is pointed in the right direction, and the PR is small. I found one substantive blocker before I can approve:

  • rust/src/native_app.rs:233 runs every diagnostic probe through Command::new(...).output() with no timeout. Because apw --json doctor is now documented as the first local/runner readiness check, a misconfigured or hung xcodebuild, cargo, detect-secrets, or security executable can cause doctor itself to hang indefinitely rather than emitting the intended OK/WARN/FAIL result and remediation hint. Please bound these probes with a short timeout and convert timeout into a structured diagnostic result.

Validation: git diff --check origin/main...HEAD passed. I also attempted cargo test --manifest-path rust/Cargo.toml doctor_creates_default_credentials_file -- --nocapture, but this review host is missing OpenSSL development metadata for openssl-sys, so compilation failed before the changed test could run.

Copy link
Copy Markdown

@athena-omt athena-omt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-checked the PR branch and confirm the existing Athena blocker is still present: command_output() still delegates directly to Command::new(command).args(args).output(), and the doctor CI diagnostics call it for xcodebuild, cargo, detect-secrets, and security without any timeout boundary.

Because apw --json doctor is now documented as the first local/runner readiness check, a hung external tool can still hang doctor itself instead of producing the intended structured OK/WARN/FAIL diagnostic with remediation guidance.

I’m not adding a separate duplicate change request beyond the existing one; this is a confirmation that the requested timeout fix is still needed before merge.

@jmcte jmcte merged commit df55ebb into main May 7, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix: apw doctor should surface missing or misconfigured self-hosted runner requirements

2 participants