Skip to content

fix(cli): report an unreadable install state as invalid in doctor - #438

Merged
barbatdev merged 2 commits into
mainfrom
fix/437-doctor-unreadable-state
Nov 12, 2026
Merged

fix(cli): report an unreadable install state as invalid in doctor#438
barbatdev merged 2 commits into
mainfrom
fix/437-doctor-unreadable-state

Conversation

@barbatdev

@barbatdev barbatdev commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Pull Request

Linked Issue

Fixes #437

PR Type

  • type:feature Feature
  • type:bug Bug fix
  • type:docs Documentation
  • type:refactor Refactoring
  • type:chore Maintenance
  • type:breaking-change Breaking change

Summary

doctor --target-dir reported ownership=unowned when .cortex/install-state.json existed but could not be read, so an unreadable ownership record looked like no record at all. Only a missing file now means unowned; any other read error reports the existing invalid value.

Changes

  • internal/cli/doctor.go: doctorTargetOwnership returns unowned only for fs.ErrNotExist, and invalid for any other read error.
  • internal/cli/doctor_directory_test.go: TestDoctorDirectoryTargetUnreadableStateIsInvalid, through the public doctor --target-dir path, for an undecodable state and a mode-0000 state (skipped as root).
  • docs/operating.md: states which state-file cases give unowned and which give invalid.

Focused Tests

Claim Test command Observed result Limit State Observed by
An unreadable state was reported as unowned go test ./internal/cli -run TestDoctorDirectoryTarget -count=1 before the fix ownership=unowned where ownership=invalid was expected Not reproducible as root Verified Agent that made the change
It is reported as invalid after the fix Same command 4 of 4 pass; exact stdout, empty stderr, exit 0 Verified Agent that made the change
Missing and owned states are unchanged TestDoctorDirectoryTargetStates unowned and owned, exit 0 Verified Agent that made the change
Full suites go vet ./...; go test ./... -count=1; python3 -m unittest discover -s tests Clean; all packages ok; OK Verified Agent that made the change

Side effects: doctorTargetOwnership has one caller (doctor --target-dir); update and uninstall do not use it.

Failed or not done: none.

Warnings: docs/operating.md says an unreadable prior state exits with the conflict code install would hit, but ownership=invalid exits 0, before and after this change. Exit 3 applies only to presence=unreadable or unwritable. This PR keeps the existing exit code; whether invalid should exit non-zero is a separate maintainer decision.

Runtime Harness

N/A: CLI diagnostic change covered by CLI tests.

Rollback

Revert the single commit.

Chain Dependencies / Out of Scope

Checklist

  • Linked issue is open and status:approved.
  • Exactly one matching type:* label is applied.
  • Focused tests and runtime harness evidence are recorded.
  • Rollback and out-of-scope boundaries are documented.

@barbatdev barbatdev added the type:bug Bug fix label Nov 12, 2026
@barbatdev
barbatdev merged commit 703732c into main Nov 12, 2026
2 checks passed
@barbatdev
barbatdev deleted the fix/437-doctor-unreadable-state branch November 12, 2026 21:09
@barbatdev

barbatdev commented Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

Post-merge QA (glm)

The merge does what issue #437 asked: an existing-but-unreadable install state is now reported as ownership=invalid instead of unowned, only a genuinely missing file yields unowned, and a CLI test pins both cases through the public doctor path. The change is minimal, has a single caller, and rollback is a single revert. One non-blocking docs point: the sentence the PR edited still claims an unreadable prior state exits with the conflict code install would hit, but ownership=invalid exits 0 (before and after this change); the PR body itself flags this as a separate maintainer decision.

Suggestion (1)

  • docs still claim an unreadable prior state exits with the conflict code, but ownership=invalid exits 0 (docs/operating.md:102): The PR edited this exact paragraph and preserved the sentence 'An unwritable directory or an unreadable prior state exits with the same conflict code install would hit.' After the merge, an existing-but-unreadable state file is classified ownership=invalid and doctorTargetOwnership's caller returns exit 0, so that sentence is false for the unreadable case (exit 3 applies only to presence=unreadable or unwritable). The PR body itself warns about this and scoped the exit-code decision out. Resolving the sentence (or the exit code) in a follow-up would keep the docs from promising a conflict exit that never happens.

Verdict: clean

@barbatdev

barbatdev commented Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

Post-merge QA (deepseek)

Lens: evidence + test design. The fix is small and correctly targeted: doctorTargetOwnership now returns unowned only for fs.ErrNotExist (via errors.Is(err, os.ErrNotExist), and errors is imported in the merged doctor.go) and invalid for any other read error, which matches the expected behaviour in #437. Evidence is proportionate: the new test goes through the public doctor --target-dir path, exercises both an undecodable state (existing invalid path) and a mode-0000 state, asserts exact stdout, empty stderr and exit 0, and skips as root with a reason; the missing-state case stays covered by the existing TestDoctorDirectoryTargetStates. One documentation contradiction is left in the same paragraph this PR rewrote, disclosed in the PR body as out of scope. No blocking defect: the behaviour matches the issue and I found no input that produces a wrong classification after the merge.

Suggestion (1)

  • Documented exit code contradicts the classification this PR introduces (docs/operating.md:99): The rewritten paragraph now says "a state file that exists but cannot be read or decoded is invalid" and then keeps "An unwritable directory or an unreadable prior state exits with the same conflict code install would hit". In the merged code ownership=invalid exits 0 (the new test asserts code != exitOK as a failure), so the second sentence describes exit 3 for exactly the input the first sentence just classified as invalid. The PR body already flags the mismatch; it should be scoped to presence=unreadable/writability=unwritable here so the doc does not promise an exit code the command does not return.

Verdict: clean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

type:bug Bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: doctor reports an unreadable install state as unowned

1 participant

@barbatdev