fix(mural): fallback to file backend when keyring is empty by akazlow · Pull Request #2135 · microsoft/hve-core

akazlow · 2026-06-22T16:19:00Z

Pull Request

Description

Fixes backend resolution when auto mode selects keyring but keyring contains no usable credentials.

In that case, when the credential file is populated, resolution now falls back to file backend with a one-shot warning so authentication can proceed.

Related Issue(s)

Fixes #1942

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:

AI Artifacts:

Reviewed contribution with prompt-builder agent and addressed all feedback
Copilot instructions (.github/instructions/*.instructions.md)
Copilot prompt (.github/prompts/*.prompt.md)
Copilot agent (.github/agents/*.agent.md)
Copilot skill (.github/skills/*/SKILL.md)

Other:

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

Testing

Added/updated backend-resolution tests in .github/skills/experimental/mural/tests/test_credential_storage.py to cover keyring-available-but-empty fallback behavior.
Verified fallback warning semantics remain one-shot and profile-scoped.
Local pytest execution is currently blocked in this shell due to missing pytest in the active .venv; CI remains the source of truth for full test execution.

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 /prompt-analyze to review contribution
Addressed all feedback from prompt-builder review
Verified contribution follows common standards and type-specific requirements

Required Automated Checks

The following validation commands must pass before merging:

Markdown linting: npm run lint:md
Spell checking: npm run spell-check
Frontmatter validation: npm run lint:frontmatter
Skill structure validation: npm run validate:skills
Link validation: npm run lint:md-links
PowerShell analysis: npm run lint:ps
Plugin freshness: npm run plugin:generate
Docusaurus tests: npm run docs:test

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

Additional Notes

This update keeps auto-mode behavior unchanged when keyring contains credentials.
Review follow-ups on docstring accuracy and fallback-path tests are being addressed in branch updates.

codecov-commenter · 2026-06-22T16:22:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.22%. Comparing base (d44ad1e) to head (c049d17).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2135      +/-   ##
==========================================
- Coverage   80.52%   78.22%   -2.31%     
==========================================
  Files         128       87      -41     
  Lines       19274    13932    -5342     
  Branches       12        0      -12     
==========================================
- Hits        15521    10898    -4623     
+ Misses       3750     3034     -716     
+ Partials        3        0       -3

Flag	Coverage Δ
docusaurus	`?`
pester	`84.23% <ø> (-0.02%)`	⬇️
pytest	`69.11% <100.00%> (-8.66%)`	⬇️

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

Files with missing lines	Coverage Δ
...ills/experimental/mural/scripts/mural/_backends.py	`81.01% <100.00%> (+1.56%)`	⬆️
...s/experimental/mural/scripts/mural/_credentials.py	`88.79% <100.00%> (+0.18%)`	⬆️

... and 42 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.

akazlow · 2026-06-22T16:23:33Z

Review Summary

This fix addresses credential backend resolution when the keyring backend is selected but lacks usable secrets.

Key Changes

_backends.py: Added fallback logic to use file backend when keyring is unavailable/empty
Prevents auth failures when credential selection doesn't match available backend state

Impact

Mural skill can now gracefully degrade to file-based auth instead of failing entirely
Improves user experience in devcontainer/CI scenarios where keyring may not be initialized

Verification Checklist

Backend resolution scope limited to single file
Fallback preserves original credential file path
No breaking changes to existing auth workflows

katriendg

Thanks for filing both the issue (#1942) and this fix, @akazlow — clear, well-scoped report and a tidy, minimal change. I validated it locally: an available-but-empty keyring plus a populated credential file now resolves to FileBackend and emits the one-time fallback warning, matching all five steps of your suggested fix. The existing test_credential_storage.py suite (38 tests) still passes and ruff is clean. 🎉

Two process asks before we go further:

Please update the PR description to follow the repo's pull request template — filling in Description, Related Issue, Type of Change, Testing, and the Checklist keeps reviews consistent. (Fixes #1942 is already there — nice.)
In that checklist, please tick the CI checks that apply to this change and leave the rest as N/A. This diff only touches a Python script in the mural skill, so the markdown / docs / PowerShell checks don't apply; the relevant ones are skill structure validation (npm run validate:skills), Python lint (npm run lint:py), and the skill's own pytest suite, plus Tests added and Documentation updated under Required Checks (see inline comments).

I've left a few inline comments — one test gap and a docstring update I'd like to see before merge, plus a couple of optional / consistency notes. Thanks again for contributing!

katriendg · 2026-06-23T08:33:44Z

+                if value:
+                    keyring_populated = True
+                    break
+            if not keyring_populated:


Regression test (please address): This new fallback path has no test. Could you add one to the TestResolveBackend class in tests/test_credential_storage.py asserting that, in auto mode, an available-but-empty keyring plus a populated credential file resolves to FileBackend and emits exactly one fallback warning (deduped per profile per process, mirroring test_auto_fallback_warn_dedupes_per_profile_per_process)? The existing test_auto_returns_keyring_when_available only covers the both-empty case, so this path is currently untested.

katriendg · 2026-06-23T08:33:44Z

    elif selector == "auto":
        try:
            selected = KeyringBackend()
+            service = _service_name_for(profile)


Docstring update (please address): The resolve_backend docstring above still describes auto fallback as happening only "when _KeyringUnavailable is raised." This PR adds a second trigger (keyring available but empty → file backend when the file is populated). Please extend the docstring to document the new path and its one-shot warning so the contract stays accurate.

katriendg · 2026-06-23T08:33:44Z

+            if not keyring_populated:
+                file_backend = FileBackend(file_path)
+                if file_path.exists():
+                    _check_credential_file_perms(file_path, os.environ)


Perms-check consistency (your call, non-blocking): This path runs _check_credential_file_perms() at resolve time, which can raise MuralError on POSIX for a bad mode/owner. The keyring-unavailable fallback just below constructs FileBackend(file_path) without a resolve-time perms check. On Windows this is a no-op, but on POSIX the empty-keyring fallback is now stricter than the unavailable-keyring one. Worth deciding whether to apply the check to both paths or neither, for symmetry.

katriendg · 2026-06-23T08:33:44Z

+                if file_path.exists():
+                    _check_credential_file_perms(file_path, os.environ)
+                file_entries = file_backend._read_all()
+                file_populated = any(


Optional refactor: These keyring / file "is populated" probes duplicate logic already in _maybe_warn_concurrent_state. A small shared helper (e.g. _backend_has_credentials(backend, service)) would centralize it and reduce drift risk if the key set or probing logic changes later. Purely optional.

katriendg · 2026-06-23T08:33:44Z

+            service = _service_name_for(profile)
+            keyring_populated = False
+            for key in _KNOWN_CREDENTIAL_KEYS:
+                value = selected.get(service, key)


Info, no action needed: Because this .get() runs inside the outer try, a keyring read error after successful construction is caught by except _KeyringUnavailable and reported as "unavailable… falling back" rather than "empty." Reasonable behavior — just flagging that the message can read slightly off in that edge case.

fix(mural): fallback to file backend when keyring is empty

6dfceff

akazlow requested a review from a team as a code owner June 22, 2026 16:19

akazlow self-assigned this Jun 22, 2026

fix(mural): fix linting errors - break long lines for ruff compliance

3899995

akazlow force-pushed the fix/mural-auth-backend-fallback branch from 3042220 to 3899995 Compare June 22, 2026 16:34

katriendg reviewed Jun 23, 2026

View reviewed changes

akazlow added 3 commits June 23, 2026 16:08

fix(mural): address PR review for backend fallback docs and tests

06d0c21

style(mural): apply ruff formatting in credential storage tests

e685831

refactor(mural): unify credential backend probes

f7ff5cd

github-advanced-security AI found potential problems Jun 23, 2026

View reviewed changes

Comment thread .github/skills/experimental/mural/tests/test_credential_storage.py Fixed

akazlow and others added 2 commits June 23, 2026 17:02

test(mural): avoid chmod in permission fallback test

0033c76

Merge branch 'main' into fix/mural-auth-backend-fallback

c049d17

akazlow requested a review from katriendg June 24, 2026 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mural): fallback to file backend when keyring is empty#2135

fix(mural): fallback to file backend when keyring is empty#2135
akazlow wants to merge 7 commits into
microsoft:mainfrom
akazlow:fix/mural-auth-backend-fallback

akazlow commented Jun 22, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Jun 22, 2026 •

edited

Loading

Uh oh!

akazlow commented Jun 22, 2026

Uh oh!

katriendg left a comment

Uh oh!

katriendg Jun 23, 2026

Uh oh!

katriendg Jun 23, 2026

Uh oh!

akazlow Jun 23, 2026

Uh oh!

katriendg Jun 23, 2026

Uh oh!

katriendg Jun 23, 2026

Uh oh!

akazlow Jun 23, 2026

Uh oh!

katriendg Jun 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

akazlow commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Description

Related Issue(s)

Type of Change

Testing

Checklist

Required Checks

AI Artifact Contributions

Required Automated Checks

Security Considerations

Additional Notes

Uh oh!

codecov-commenter commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

akazlow commented Jun 22, 2026

Review Summary

Key Changes

Impact

Verification Checklist

Uh oh!

katriendg left a comment

Choose a reason for hiding this comment

Uh oh!

katriendg Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

katriendg Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

akazlow Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

katriendg Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

katriendg Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

akazlow Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

katriendg Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akazlow commented Jun 22, 2026 •

edited

Loading

codecov-commenter commented Jun 22, 2026 •

edited

Loading