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

[akazlow]

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:

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

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.35%. Comparing base (3620737) to head (ce19da0).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2135      +/-   ##
==========================================
+ Coverage   81.28%   81.35%   +0.06%     
==========================================
  Files         128      118      -10     
  Lines       18925    18838      -87     
  Branches       12        0      -12     
==========================================
- Hits        15384    15325      -59     
+ Misses       3538     3513      -25     
+ Partials        3        0       -3     

Flag	Coverage Δ
docusaurus	?
pester	86.07% <ø> (-0.02%)	⬇️

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	79.31% <ø> (-0.15%)	⬇️
...s/experimental/mural/scripts/mural/_credentials.py	88.10% <ø> (-0.51%)	⬇️

... and 11 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]

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]

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]

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.

[akazlow]

fixed!

[katriendg]

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]

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.

[akazlow]

fixed

[katriendg]

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.

[WilliamBerryiii]

Thanks for this fix — the fallback itself is solid. It only fires when keyring holds zero credentials, and it enforces the same 0600/owner contract (enforce_file_perms=True) as explicit file mode, so nothing secure is shadowed.

One follow-on I'd like to fold in while we're in this code, because I don't want us to settle for living on the plaintext file when a working keyring is right there: when keyring is available-but-empty and the file is populated, let's promote the credentials into the keyring so the operator actually ends up on the secure store.

Important on where: please don't put the promotion inside resolve_backend — that's the read path (auth status, list, etc.), and a silent keyring write there can trigger an OS unlock prompt or fail on a locked vault during what should be a passive read. Instead, do the promotion in the auth/write flows (auth login / auth bootstrap) that already expect to touch credentials: detect "keyring available + empty + file populated," write the creds into keyring, read them back to confirm the write succeeded, and only then clear the file. If the keyring write fails, keep the file and fall back exactly like today. The empty-keyring fallback in resolve_backend can stay as-is for the immediate read, plus a one-line hint that promotion will happen on next login.

Net effect: this PR''s fix keeps auth working immediately, and the next real auth touch moves people off plaintext and onto the keyring — without making passive reads do surprising writes.

To make this easy to pick up, here''s a task-researcher prompt you can run to scope the change:

Task-researcher prompt

Research and produce an implementation plan for adding "file -> keyring credential promotion"
to the Mural CLI credential subsystem.

Context / goal:
When MURAL_CREDENTIAL_BACKEND=auto and the OS keyring backend is available but holds no Mural
credentials, while the per-user credential file IS populated, the CLI should promote (copy) the
file credentials into the keyring so the operator ends up on the more secure store instead of
living on the plaintext file indefinitely. PR #2135 already fixed the immediate bug (empty keyring
no longer shadows usable file creds); this task is the follow-on promotion behavior.

Hard constraint on placement:
Do NOT perform the keyring write inside resolve_backend(). That function is the read path called by
passive commands (auth status, list, etc.); a silent keyring write there can trigger an OS unlock
prompt or fail on a locked vault. The promotion MUST live in the auth write flows
(_cmd_auth_login and/or _cmd_auth_bootstrap) where touching credentials is already expected.

Files to investigate (all under .github/skills/experimental/mural/scripts/mural/):
- _backends.py: resolve_backend(), KeyringBackend, FileBackend, _emit, the _KeyringUnavailable
  sentinel, and the one-shot warn state via _state.seen_fallback_warn().
- _credentials.py: _backend_has_credentials(), _check_credential_file_perms(), _service_name_for(),
  _resolve_credential_file(), and the _KNOWN_CREDENTIAL_KEYS / _PROFILE_REQUIRED_KEYS constants.
- _cli_auth.py: _cmd_auth_bootstrap() and the auth login command — these are the intended homes for
  promotion. Note bootstrap already persists via resolve_backend(profile).
- _constants.py: credential key names and env var constants.
- tests/test_credential_storage.py: existing backend-resolution and fallback tests to mirror.

Required behavior to design and validate:
1. Promotion trigger: auto mode, keyring backend constructs successfully, keyring has none of
   _KNOWN_CREDENTIAL_KEYS for the profile service, and the file backend is populated (reuse
   _backend_has_credentials with enforce_file_perms=True so the 0600/owner contract still gates it).
2. Promotion action: write each populated credential key from the file into the KeyringBackend for
   _service_name_for(profile).
3. Verify-before-delete: after writing, read the values back from keyring and confirm they match the
   file values. Only clear/remove the credential file AFTER a verified successful write. If the file
   is left, document why.
4. Failure handling: if the keyring write or read-back fails (locked vault, _KeyringUnavailable, any
   keyring error), keep the file untouched and fall back to file exactly like today. Never lose
   credentials.
5. Non-interactive safety: respect ENV_NONINTERACTIVE / CI — do not trigger OS prompts in
   non-interactive contexts; decide whether promotion is skipped or attempted silently there.
6. Idempotency + one-shot logging: promotion runs once per profile per process; emit a clear WARNING/
   INFO describing the promotion and the file cleanup, deduped via the existing _state seen-set pattern.
7. Windows: _check_credential_file_perms is a POSIX-only no-op; confirm promotion still behaves
   correctly on Windows and note the residual ACL gap.

Deliverables:
- A short evidence-backed findings doc (current resolution/bootstrap flow, where promotion fits).
- A concrete implementation plan listing exact functions to add/modify and their call sites.
- A test plan extending test_credential_storage.py: keyring-empty+file-populated promotion success,
  keyring-write-failure preserves file + falls back, verify-before-delete, non-interactive skip,
  one-shot dedupe, and a Windows-path case.
Do not write production code in this pass — produce research + plan only.

Happy to pair on the command surface if useful.