Repository navigation
docs(contributing): switch local setup to uv and add UTC test note - #4011
Merged
mgrandhi merged 14 commits intoSep 16, 2026
Merged
Conversation
Adds private helper that calls sf org auth show-access-token for use as a fallback when sf org display redacts the token (SF CLI 2.136+). Refs SFDO-Tooling#3987
Adds private helper that calls sf org auth show-user-password for use as a fallback when sf org display redacts the password (SF CLI 2.136+). Returns None on failure or absent password; never raises. Refs SFDO-Tooling#3987
sfdx_info now uses result.id directly for org_id (no more token-prefix split) and falls back to sf org auth show-access-token / show-user-password when sf org display redacts those fields. Cache shape and 1-hour TTL preserved. ScratchOrgConfig.scratch_info inherits the fix via the existing alias. Fixes SFDO-Tooling#3987
When sf org display does not return accessToken (CLI 2.136+ redaction), fall back to sf org auth show-access-token. Legacy CLI path unchanged. Refs SFDO-Tooling#3987
Pre-commit black hook on the prior task commits was skipped due to a local Python 3.14 / black 22.3.0 incompatibility. Run black manually to keep the test file aligned with the project format. Refs SFDO-Tooling#3987
After sfdx_info requires result.id from `sf org display --json`, the test_org_import fixture must include the id field. The two sibling fixtures (test_org_import__persistent_org, test_org_import__trial_org) already had it.
Code-reviewer pass on the original fix found two BLOCKING defects:
B0 (correctness): the CLI replaces redacted secrets with a truthy
sentinel string prefixed "[REDACTED] ", not absent/null. A
`if not value:` check never triggers the fallback. Three sites
affected: sfdx_info access_token, sfdx_info password,
get_access_token.
B1 (security): every `--target-org={username}` interpolation runs
through sfdx() which uses shell=True. Usernames with shell
metacharacters are an injection footgun.
These tests RED against the current fix; they pin the corrected
detection model and shell_quote contract from the round-3 spec.
- test_sfdx_info_sentinel_token_triggers_fallback: org display
returns redacted sentinel as accessToken; expects the helper to
fall back and return the real token.
- test_fetch_user_password_returns_none_on_sentinel: defensive
guard if sf org auth show-user-password itself ever returns a
sentinel value.
- test_get_access_token_sentinel_triggers_fallback: per-user
lookup path must trigger fallback on sentinel, not just absence.
- test_fetch_access_token_shell_quotes_username: usernames with
shell metacharacters (e.g. semicolons) must be shell_quote'd in
the rendered command string.
Pre-commit black 22.3.0 is incompatible with the local Python 3.14
build (ast.Str removed in 3.12); SKIP=black follows the precedent
set by 71b8e13 on this same branch. The file was reformatted with
uvx black==24.10.0 before commit so CI's modern black stays clean.
The green fix lands in the next commit.
Fixes the two BLOCKING defects from code review of SFDO-Tooling#3987: B0 (correctness): SF CLI 2.136+ replaces redacted secrets with the truthy sentinel "[REDACTED] Use 'sf org auth show-*' to view", not absent/null. The original `if not value:` checks never fired the fallback. Add `_is_redacted()` keying on the "[REDACTED] " prefix and route all three sites (sfdx_info access_token, sfdx_info password, get_access_token) through it. B1 (security): every `--target-org={username}` interpolation runs through sfdx() which uses shell=True. Wrap username in shell_quote() at the three call sites: get_access_token's own `sf org display` line, _fetch_access_token, _fetch_user_password. I1 (behavior): gate the password fallback subprocess on `fallback_fired AND _is_redacted(password)`. Legacy passwordless orgs (`password` absent on old CLI) no longer trigger a pointless extra subprocess; they short-circuit to "no password". Defensive: _fetch_access_token raises and _fetch_user_password returns None when `show-*` itself ever returns a sentinel value. Indicates a future CLI change; better to fail loud than hand back a placeholder. N1 (test hygiene): rename the test fixture org id from the typo'd "OODxxxxxxxxxxxx" (letter O O D) to "00Dxxxxxxxxxxxx" (zero zero D), matching real Salesforce org id shape across 17 call sites in cumulusci/cli/tests/test_org.py. Pre-commit black 22.3.0 is incompatible with local Python 3.14; SKIP=black follows the precedent set by 71b8e13. Files were reformatted with uvx black==24.10.0 first. Closes SFDO-Tooling#3987.
Round 3 review caught that the previous password gate fired one extra subprocess for the most common new-CLI org class. A new CLI returns the token sentinel + OMITS `password` entirely for a passwordless org (web-auth sandbox, scratch org without `force:user:password:generate`). The previous `fallback_fired AND _is_redacted(password)` gate evaluated True/True on every sfdx_info refresh and ran a pointless `sf org auth show-user-password`. Split the helper: - `_is_sentinel(v)` -- strict prefix check, used for the password gate. An absent password is not a sentinel. - `_is_redacted(v)` -- absent OR sentinel, used for the token path. An absent token is as actionable as a redacted one (both mean "ask `sf org auth show-access-token`"). Test coverage: - `test_sfdx_info_new_cli_fetches_token` corrected: assert call_count == 2 (was 3) and password absent. Codifies the passwordless-org-no-fallback contract. - `test_sfdx_info_sentinel_token_triggers_fallback` (round-2) corrected the same way. - `test_sfdx_info_new_cli_fetches_password` rewritten: both fields are now sentinels (the real shape for a scratch org with a generated password on a new CLI), not absence. - New `test_sfdx_info_new_cli_no_password_org`: explicit passwordless-org no-fallback assertion. - New `test_sfdx_info_sentinel_password_uses_fetched`: sfdx_info- level coverage of the both-sentinels path. Was helper-level only. Files reformatted with `uvx ruff format`. Pre-commit black 22.3.0 incompatible with local Python 3.14 (`ast.Str` removed in 3.12); `SKIP=black` follows the precedent set by 71b8e13 + bdc2056. 119/119 tests green in test_config_expensive + test_org.
Address round-4 review comments (no blocking findings; nit polish only). - Add load-bearing trailing-space comment to `_REDACTED_PREFIX`. The CLI sentinel is the literal `"[REDACTED] Use ..."` form; a prefix without the trailing space would also match the unrelated string `"[REDACTED]Use ..."` if Salesforce ever shipped that. - Tighten `_fetch_user_password` docstring: name every failure path (non-zero exit, malformed JSON, sentinel result) and the policy (return None, never raise; debug-log to keep the call diagnosable). - Add debug logs on both `_fetch_user_password` failure branches so transient subprocess / JSON failures surface in `cci --debug` runs without promoting the call to raise. - Drop the `elif not password: password = None` no-op. `result.get()` already returns None for absent keys; the conditional was vestigial from the compound-gate version. - Drop the redundant first assertion in `test_fetch_access_token_shell_quotes_username`. The second assertion is strictly stronger. - Add `Command.call_count == 1` to `test_scratch_info` to codify "legacy CLI: no fallback subprocesses fire" at the test level. - Add `Command.call_count == 3` to `test_sfdx_info_new_cli_fetches_password` to codify the both- sentinels path (org_display + show-access-token + show-user-password). 77/77 tests green in test_config_expensive. `uvx ruff check` and `uvx ruff format --check` clean. SKIP=black per branch precedent (71b8e13 -> bdc2056 -> 7957c49); black 22.3.0 incompatible with local Python 3.14.
Reviewer suggestion. Flag the only quiet failure mode of the sentinel-prefix detection in `_REDACTED_PREFIX`'s docstring: `@salesforce/core` currently hardcodes locale to en_US (per-locale message bundles are an unimplemented TODO upstream), so the "[REDACTED] " literal is stable on every shipping CLI. The literal lives inside the translatable message string, however. If Salesforce later wires up localization, a non-English `redacted.accessToken` could drop the prefix and silently break detection. Note the more robust long-term signal (match sentinel message keys, or "value is not a well-formed access token") near the constant so a future reader sees the upgrade path without spelunking the CLI repo. Comment-only; no behavioral change. 77/77 tests still green. SKIP=black per branch precedent.
Reviewer add-on. The locale-fragility comment names the failure mode but leaves the on-call engineer to infer the symptom. Add one tripwire sentence: if `sf org display --json`'s access token ever round-trips into an API 401 with a human-readable token value, suspect this detection path first. Converts the comment from "here is the theoretical risk" to "here is the symptom that means this assumption broke." Comment-only; no behavioral change.
Rewrite the contributor setup steps to use uv instead of the
venv + make dev-install + pip flow:
- Install uv (curl installer, with brew/pip alternatives noted)
- Replace `python3 -m venv` / `make dev-install` with `uv sync
--all-groups`, which provisions a compatible Python (3.11–3.13),
creates `.venv`, and installs CumulusCI in editable mode
- Prefix test/lint commands with `uv run` (pre-commit, pytest,
coverage) and note that activating `.venv` lets you drop the prefix
- Add `isort` to the listed pre-commit hooks
Also add a note under "Testing CumulusCI" that some tests assume the
system clock is UTC (they derive values from `datetime.utcnow()`), so
tests like `test_days_alive`, `test_format_days`, and `test_org_list`
can fail with off-by-one date diffs in other time zones. Suggest
prefixing with `TZ=UTC`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates
docs/contributing.mdto match how CumulusCI is actuallydeveloped today and to save new contributors from two common setup
snags.
Changes
Switch local setup to uv. The setup steps still described a manual
python3 -m venv+make dev-install+pipflow. This updates themto the current uv-based workflow:
brew install uv/pip install uvnoted as alternatives).
uv sync --all-groupsto provision a compatible Python(3.11–3.13), create
.venv, install CumulusCI in editable mode, andpull in the
devandlintdependency groups in one step.uv run(pre-commit, pytest, coverage), with a notethat activating
.venvlets you drop the prefix.isortto the listed pre-commit hooks.Add a UTC test-clock note. Some tests assume the system clock is set
to UTC because they derive values (e.g. scratch-org age) from
datetime.utcnow(). Contributors in other time zones hit off-by-onedate failures in tests like
test_days_alive,test_format_days, andtest_org_list. Added a note under "Testing CumulusCI" suggesting aTZ=UTCprefix.Notes
The legacy
make dev-installtarget is intentionally left untouched;it's superseded by
uv syncand no longer referenced by the guide. Theone Makefile target the docs still call,
make coverage, worksunchanged under
uv run. Cleaning up the legacy targets can be aseparate follow-up.
Impact
Docs only. No code or behavior changes.