Skip to content

fix(skills): align Mural doctor and auth status on usable sessions - #3179

Merged
Jamie Kim (jkim323) merged 8 commits into
mainfrom
fix/3178-mural-doctor-session-alignment
Oct 15, 2026
Merged

Jamie Kim (jkim323) merged 8 commits into
mainfrom
fix/3178-mural-doctor-session-alignment

Conversation

@WilliamBerryiii

Copy link
Copy Markdown
Member

Pull Request

Description

mural doctor and mural auth status now agree on whether a profile is logged in. A ready verdict from doctor now means Mural commands can actually authenticate. The login overwrite protection added in #3129 is unchanged.

#3129 made login and status share one session check. That check counts a MURAL_REFRESH_TOKEN stored in the keyring or credential file, which is right for blocking an accidental login overwrite. Nothing in the request path ever reads that backend token, though; requests authenticate and refresh only from the token-store record. Reusing the same check in doctor, as #3178 originally proposed, would have reported ready for a state in which every command fails with "run auth login first". This PR therefore separates the two questions.

Session checks

  • New _profile_has_usable_session reads only the token store. It returns true when the active profile's record passes _select_profile validation and has an access or refresh token. It never reads the keyring or credential file and never raises.
  • _profile_has_session, the login overwrite guard, and auth login behavior are unchanged.

mural auth status

  • authenticated now comes from the usable-session check.
  • Every status output gains backend_refresh_token, which reports a backend refresh token separately. An unreadable credential file reports false instead of raising.
  • Exit codes are unchanged. One fix(skills): allow Mural login after auth setup #3129 test now expects authenticated: false with backend_refresh_token: true for a backend-only refresh token.

mural doctor

  • Doctor resolves the profile the same way status and the request path do: --profile, then MURAL_PROFILE, then the profile chosen by mural auth use, then default. Previously it ignored the mural auth use choice.
  • It reads login state and granted scopes for that profile through the usable-session check, so a backend-only refresh token yields needs_login.
  • Doctor still never touches the keyring or the network. An invalid profile name still produces a verdict instead of an exception.

Scope check before each command

  • _require_scope resolves the same active profile when main() calls it. Previously main() checked default while the request used the active profile.
  • Commands without required scopes still never load the token store at dispatch.
  • An invalid --profile on a scoped command now exits 1 with a validation error instead of 77 as a scope denial. A test pins this.

Cleanup

  • Removed the unused _op_auth_status, which hard-coded authenticated: True, along with its package re-export and eight imports that became unused. The handler parity test lists auth_status as CLI-only.

Documentation

  • Mural SECURITY.md B3 describes the overwrite guard and the usable-session check separately. The B2 elevation-of-privilege bullet names the active-profile precedence.
  • The docs/security/security-model.md registry row is updated.
  • SKILL.md documents the new status field and doctor's profile resolution, and corrects the scope-upgrade command to auth login --write --force; without --force, login keeps an existing session.
  • The needs_login and needs_scope_upgrade remediation text in references/bootstrap.md mentions --force. Agents that call doctor match verdict tokens rather than this wording.

Related Issue(s)

Fixes #3178

Related to #3129

Type of Change

Select all that apply:

Code & Documentation:

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Breaking change (fix or feature causing existing functionality to change)
  • Documentation update

Infrastructure & Configuration:

  • GitHub Actions workflow
  • Linting configuration (markdown, PowerShell, etc.)
  • Security configuration
  • DevContainer configuration
  • Dependency update

AI Artifacts:

  • Reviewed contribution with hve-builder and addressed all actionable findings
  • Copilot instructions (.github/instructions/*.instructions.md)
  • Copilot prompt (.github/prompts/*.prompt.md)
  • Copilot agent (.github/agents/*.agent.md)
  • Copilot skill (.github/skills/*/SKILL.md)
  • Copilot hook (.github/hooks/*/*.json)
  • Eval spec added/updated for changed AI artifacts (evals/)

Note for AI Artifact Contributors:

  • Agents: Research, indexing/referencing other project (using standard VS Code GitHub Copilot/MCP tools), planning, and general implementation agents likely already exist. Review .github/agents/ before creating new ones.
  • Skills: Must include both bash and PowerShell scripts. See Skills.
  • Model Versions: Contributions MUST target models listed in the model catalog (scripts/linting/model-catalog.json) whose provider appears in providerAllowlist and whose status is ga or preview. Run npm run lint:models to validate references.
  • See Agents Not Accepted and Model Version Requirements.

Other:

  • Script/automation (.ps1, .sh, .py)
  • Other (please describe):

Sample Prompts (for AI Artifact Contributions)

User Request:

"Seed the Method 4 brainstorming board in Mural." In the DT Coach, the agent first runs mural doctor --require-scope murals:write.

Execution Flow:

Doctor inspects local state for the active profile without contacting Mural. If the only stored credential is a refresh token in the keyring or credential file, doctor now returns needs_login. The agent stops and relays the bootstrap remediation, which tells the operator to rerun login with --force if login reports existing stored credentials.

Output Artifacts:

{"verdict": "needs_login", "required_scopes": ["murals:write"], "granted_scopes": []}

mural auth status for the same state reports "authenticated": false and "backend_refresh_token": true.

Success Indicators:

Doctor and auth status agree, so ready is followed by commands that authenticate. After mural auth use <profile>, doctor evaluates that profile's login state and scopes.

For detailed contribution requirements, see:

Testing

New tests:

  • A parity matrix runs auth status and doctor over the same eight local states:

    • setup-only
    • a token-store access token
    • a token-store refresh token only
    • a backend-only refresh token
    • client configuration only
    • an active profile switched to an authenticated profile
    • an active profile switched to a setup-only profile
    • a malformed record that still has tokens

    Doctor reports ready exactly when status reports authenticated: true. Doctor runs with the keyring and file backends patched to fail if touched.

  • Usable-session helper cases, plus status cases for a keyring-only refresh token, an unreadable credential file, and a malformed record.

  • Dispatch scope-gate tests:

    • the active profile has write scope: allowed
    • the active profile is read-only: denied
    • --profile overrides the active profile
    • an unscoped command runs with a malformed store
    • an invalid profile name exits 1; this test fails with 77 == 1 when the main() change is reverted

Commands run locally on Windows:

  • Full Mural suite, uv run pytest in .github/skills/experimental/mural at 4d5cbb832: passed, 987 passed and 146 skipped. Clean main at 313693b66 gives 964 passed and 146 skipped.
  • After the test-only commit 5c1efedf5, uv run pytest tests/test_doctor.py tests/test_handler_shape_parity.py tests/test_widget_create_bulk_dispatch.py tests/test_mural_main.py: passed, 47 passed and 6 skipped.
  • uv run ruff check scripts tests and uv run ruff format --check scripts tests: passed.
  • git diff --check over the branch diff: passed.
  • npm run validate:skills: passed, 87 skills with 0 errors and 0 warnings.
  • npm run validate:docs: passed.
  • npm run lint:frontmatter and npm run lint:tables: passed.
  • npx markdownlint-cli2 on the changed Markdown files: passed, 0 issues.
  • npm run docs:generate, npm run docs:generate:check, and npm run lint:asset-docs: passed with no generated diff and 0 errors.
  • npm run spell-check: failed. All 11 issues are graphify in unchanged extension/ files.
  • npm run lint:md-links: failed on two external links in unchanged files: the OWASP AI guide (404) and a Medium article (403). Links in the changed files pass.

npm run validate:local was not run as an aggregate because it stops on a local, gitignored corpus directory. Its owning component checks were run individually as listed.

Checklist

Required Checks

  • Documentation is updated (if applicable)
  • Files follow existing naming conventions
  • Changes are backwards compatible (if applicable)
  • Tests added for new functionality (if applicable)

AI Artifact Contributions

  • Used hve-builder review mode to review contribution
  • Addressed all actionable findings from the hve-builder review
  • Verified contribution follows common standards and type-specific requirements

Required Local Checks

The following local-safe validation commands must pass before merging:

  • Local validation aggregate: npm run validate:local
  • Documentation validation (if docs changed): npm run validate:docs
  • Spell checking: npm run spell-check
  • Link validation: npm run lint:md-links

Security Considerations

  • This PR does not contain any sensitive or NDA information
  • Any new dependencies have been reviewed for security issues
  • Security-related scripts follow the principle of least privilege

No dependency manifests or lockfiles changed.

Additional Notes

  • auth status JSON gains one key, backend_refresh_token. Its authenticated value changes from true to false for a backend-only refresh token, so backwards compatibility is left unchecked for that reason. No other status keys or exit codes changed.
  • An invalid --profile on a scoped command now exits 1 with a validation error instead of 77.
  • The issue suggested reusing _profile_has_session in doctor. This PR intentionally uses a separate usable-session check instead, for the reasons given in the Description.

- add store-only _profile_has_usable_session; login overwrite guard unchanged
- auth status reports backend_refresh_token apart from authenticated
- doctor and dispatch scope gate follow the active profile
- remove unused _op_auth_status and allowlist auth_status as CLI-only

Closes #3178

🔐 - Generated by Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔐 - Generated by Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 - Generated by Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@WilliamBerryiii
Bill Berry (WilliamBerryiii) requested a review from a team as a code owner October 10, 2026 00:52
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Oct 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.90%. Comparing base (b2bc332) to head (fc893e4).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3179      +/-   ##
==========================================
- Coverage   86.61%   81.90%   -4.71%     
==========================================
  Files          87      125      +38     
  Lines       11554    17625    +6071     
  Branches        0       63      +63     
==========================================
+ Hits        10007    14435    +4428     
- Misses       1547     3179    +1632     
- Partials        0       11      +11     
Flag Coverage Δ
docusaurus 90.58% <ø> (?)
pester 86.32% <ø> (-0.01%) ⬇️
pytest 73.18% <100.00%> (-24.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...kills/experimental/mural/scripts/mural/__init__.py 90.12% <100.00%> (ø)
...ills/experimental/mural/scripts/mural/_cli_auth.py 72.37% <100.00%> (ø)
...skills/experimental/mural/scripts/mural/_doctor.py 92.53% <100.00%> (ø)
.../skills/experimental/mural/scripts/mural/_oauth.py 90.87% <100.00%> (ø)
...ls/experimental/mural/scripts/mural/_operations.py 17.36% <ø> (ø)

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Eval Execution

✅ Status: Passed

  • Artifacts evaluated: 1
  • Specs run: 2
  • Assertions passed: 21
  • Assertions failed (blocking): 0
  • Assertions failed (advisory): 0
  • Trial dips tolerated by per-stimulus aggregate: 0
  • Failed specs (merge-blocking): 0
Artifact Kind Status Specs Passed Failed (blocking) Failed (advisory) Trial dips (tolerated) Input tokens / trial Cache-read tokens / trial
mural skill ✅ pass 2 21 0 0 0 70,168 44,548

Legend — ✅ clean · ⚠️ advisory failures only (non-blocking) · ⏭️ skipped · ❌ merge-blocking failure

Only Failed specs (merge-blocking) gates this PR. Advisory assertion failures are signal-quality checks captured during iteration; review them, but they do not block merge and may be acceptable.

Token columns are advisory means per trial reported by the model provider. They vary between trials and never gate this PR.

@jkim323 Jamie Kim (jkim323) left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work splitting "usable session" from the login overwrite guard. The reasoning in the description is clear, the parity matrix is thorough, and moving the scope gate to the active profile is a good security improvement too. Two comments for you to look at and one housekeeping: Plugin Validation failed on a PSGallery Pester install and needs a re-run. Otherwise looks great and approved

Comment thread .github/skills/experimental/mural/references/bootstrap.md Outdated
Comment thread .github/skills/experimental/mural/scripts/mural/_cli_auth.py
🔐 - Generated by Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 - Generated by Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jkim323
Jamie Kim (jkim323) added this pull request to the merge queue Oct 15, 2026
Merged via the queue into main with commit 82b4507 Oct 15, 2026
149 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.

refactor(skills): align Mural doctor and auth-status op with shared session check

4 participants