fix(auth): expand port hint with git credential fill command and docs URL#1608
Conversation
… URL
The [i] hint emitted by AuthResolver.build_error_context when dep_ref.port
is set named the right problem but not the remedy. Users had to translate
'stores per-port entries' into a concrete debugging step themselves.
New three-line hint:
[i] Host 'host:port' -- this helper may key by host only.
Verify with: printf 'protocol=https\nhost=host:port\n\n' | git credential fill
See: https://microsoft.github.io/apm/getting-started/authentication/#custom-port-hosts-and-per-port-credentials
Rationale:
- git credential fill is the helper-agnostic entry point every credential
helper implements; it reproduces exactly what APM sends internally.
- The docs URL points to the per-helper compatibility table already in tree.
- Matches the wire-format precedent set by the cross-protocol warning
(also emits a 'See: <docs URL>' line).
Docs: authentication.md section updated with the same verification
command; apm-guide skill mirror updated in parallel.
Closes #799
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the actionability of the custom-port credential-helper hint emitted by AuthResolver.build_error_context by adding (1) a concrete git credential fill verification command and (2) a direct link to the relevant authentication docs section, and mirrors that guidance in the documentation corpus.
Changes:
- Updated
AuthResolver.build_error_contextto emit a 3-line[i]hint whenhost_info.portis set (warning + verification command + docs URL). - Added unit tests to assert presence of the
git credential fillcommand and the docs anchor in the emitted hint. - Updated both the public docs and the in-repo APM usage skill docs to include the same verification command.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/core/auth.py |
Expands the port-specific hint to include a git credential fill reproduction command and a docs link. |
tests/unit/test_auth.py |
Adds assertions covering the new command and docs link in the hint output. |
docs/src/content/docs/getting-started/authentication.md |
Documents the helper-agnostic verification command in the custom-port credentials section. |
packages/apm-guide/.apm/skills/apm-usage/authentication.md |
Mirrors the same verification command in the APM usage skill docs. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
| def test_port_hint_includes_docs_url(self): | ||
| with patch.dict(os.environ, {}, clear=True): | ||
| with patch.object(GitHubTokenManager, "resolve_credential_from_git", return_value=None): | ||
| resolver = AuthResolver() | ||
| msg = resolver.build_error_context("bitbucket.corp.com", "clone", port=7999) | ||
| assert "custom-port-hosts-and-per-port-credentials" in msg, ( | ||
| f"Expected docs URL anchor in hint, got:\n{msg}" | ||
| ) |
…nstant - Replace substring URL assertion in test_port_hint_includes_docs_url with urllib.parse.urlparse checks per tests.instructions.md (CodeQL py/incomplete-url-substring-sanitization) - Promote _docs_url local variable to module-level _PORT_CREDENTIAL_DOCS_URL constant, matching _PROTOCOL_FALLBACK_DOCS_URL pattern in clone_engine.py - Normalize 'Docs:' prefix in port hint (was 'See:') for consistency with the five other build_error_context URL lines in auth.py - Add CHANGELOG entry for the user-visible credential error improvement Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
apm-review-panel recommendationHeadline: Ship after fixing the substring URL assertion that will fail CodeQL CI; all other findings are low-friction folds. CEO arbitration: The panel is in strong agreement: this is a well-scoped, user-facing improvement to credential error ergonomics. The single blocking issue -- a substring URL assertion in Deferred (not blocking): Findings applied in commit
|
| # | Persona | Finding | Disposition |
|---|---|---|---|
| 1 | test-coverage-expert + python-architect | test_port_hint_includes_docs_url uses substring URL assertion (CodeQL py/incomplete-url-substring-sanitization); violates tests.instructions.md |
FOLD (BLOCKING) -- fixed: urlparse with .hostname + .fragment assertions |
| 2 | python-architect | _docs_url local variable should be module-level constant |
FOLD -- promoted to _PORT_CREDENTIAL_DOCS_URL |
| 3 | devx-ux-expert | " See:" prefix inconsistent with all other build_error_context URL lines (lines 594, 603, 622, 636, 651 all use " Docs:") |
FOLD -- normalized to " Docs:" |
| 4 | oss-growth-hacker | CHANGELOG entry for user-visible auth improvement | FOLD -- added to [Unreleased] > Fixed |
| 5 | doc-writer | Skill mirror prose diverges from canonical getting-started wording | DEFER -- prose is acceptably close; a precision pass would be a separate docs-only PR |
| 6 | cli-logging-expert | [i] prefix unique among build_error_context entries (others are bare text) |
DEFER -- touches established format pattern beyond this PR's scope |
| 7 | supply-chain-security-expert | display interpolated in suggested shell command without quoting |
DEFER -- display is already inside single-quotes in the resulting shell string; low real-world risk |
| 8 | auth-expert | Hint omits path= line for useHttpPath setups |
DEFER -- additional hint enhancement; separate scope |
Ship recommendation: ship_with_followups -- folds applied, lint green (ruff + pylint 10.00/10 + auth-signals clean), all 5 port-hint tests pass. Ready for maintainer merge once CI clears.
TL;DR
The
[i]hint emitted byAuthResolver.build_error_contextwhen a custom port is detected now includes a concrete verification command and a docs link, raising actionability from ~70% to ~100%.Problem (WHY)
Follow-up from PR #788 review panel (devx-ux-expert finding #4). The previous hint:
Named the right problem but not the next action. Users had to translate "stores per-port entries" into a concrete debugging step for their specific helper (GCM, osxkeychain, libsecret, gh-auth) themselves.
Approach (WHAT)
Rewrite the hint to three lines:
git credential fillis the helper-agnostic entry point every credential helper implements; it reproduces exactly what APM sends internally.See: <docs URL>pattern established by the cross-protocol warning (PR fix(install): warn when --allow-protocol-fallback reuses a custom port across schemes (#786) #789).Implementation (HOW)
flowchart LR A[build_error_context] --> B{host_info.port != None?} B -->|Yes| C[Emit 3-line hint:\n1. host-only key warning\n2. git credential fill cmd\n3. docs URL] B -->|No| D[Skip hint]Files changed:
src/apm_cli/core/auth.py-- rewrites the port hint string (lines 710-714)tests/unit/test_auth.py-- adds two new assertions:git credential fillsubstring + docs URL anchor; existingper-portassertion continues to pass via the URLdocs/src/content/docs/getting-started/authentication.md-- adds the verification command to the "Custom-port hosts" sectionpackages/apm-guide/.apm/skills/apm-usage/authentication.md-- mirrors the same updateTrade-offs:
Validation evidence
uv run --extra dev pytest tests/unit/test_auth.py -q: 105 passeduv run --extra dev ruff check src/ tests/: All checks passeduv run --extra dev ruff format --check src/ tests/: 1165 files already formatteduv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/: 10.00/10bash scripts/lint-auth-signals.sh: auth-signal lint cleanHow to test
Closes #799