Repository navigation
chore(release): bfabric 1.23.0, bfabric_scripts 1.18.0, bfabric_app_runner 0.9.0 - #638
Merged
Merged
Conversation
Removes excess from bfabric_scripts/cli/api/ ahead of adding JSON input to the api commands, so that feature lands net-negative overall. - parser.py / inspect.py: drop the Google-style Args:/Returns: blocks that AGENTS.md rules out, the comments that restate the line below them, and ConfigDict(arbitrary_types_allowed=True) on two models whose fields are all standard types. Kept every comment that explains *why* (the engine guard, the minOccurs/maxOccurs XSD defaults). - query_repr.py: drop to_dict's three @Overloads and its duplicates="drop" mode, which no production caller used, for one uniform return type. - read.py: drop a dead __all__ re-export shim, and stop routing client.read through eval() -- it built the call as an f-string purely to log the equivalent Python, which the log string still does. That eval was also masking perform_query's return type, annotated list[ApiResponseObjectType] while returning a ResultContainer. - create.py / update.py: fold their identical render_output tail into render_saved() in output_format.py. _confirm_action takes a Mapping[str, object] now: it only logs and pretty- prints the attributes, and an invariant dict annotation forced a needless exact match on the value type.
Adds tests that characterise the current on-disk and cross-process shapes, so the upcoming auth-method refactor can be verified as behaviour-preserving rather than merely green. They pass against unmodified code by design. - test_frozen_contracts.py: the auth-owned key set, that inline secrets are a subset of it, that auth keys stay out of BfabricClientConfig, ConfigData's field order and that `auth` has no default (bfabric_rest_proxy and bfabric_asgi_auth construct it positionally by keyword), that export_config_data covers every field and unwraps SecretStr rather than emitting '**********', the override JSON round trip, and the exact YAML key set each auth command writes -- including that a legacy password environment does not gain an auth_method key, and mode 0600. - test_backward_compat.py: extend the vendored 1.19.0 replica beyond the PAT shape to the oauth and client_credentials shapes. app_runner bind-mounts the host config into containers running published <=1.19.0 clients, so every shape the CLI writes must leave the shared file loadable for them. - test_credential_provider.py: pickle round trips for both grants and the cache path. The provider is pickled as part of a Bfabric client and the refactor will add named constructors around __init__, which is what these guard.
``auth_method`` was a bare string with every method's fields flattened beside it, so nothing tied a method to its own data: no validator related the two, and each consumer re-derived the mapping. Each variant now owns its keys and knows how to produce its own credentials. The on-disk YAML is unchanged. It is frozen -- 1.21.0 shipped it, the docs teach users to hand-write it, and app_runner bind-mounts the file into containers running published <=1.19.0 clients -- so the union is an in-memory shape only, translated in one place. EnvironmentConfig keeps auth/auth_method/client_id/client_secret/scope as properties, so every existing reader is untouched. An unrecognised auth_method no longer fails the whole file. ConfigFile validates every environment eagerly, so one unknown value used to make an unrelated `auth activate` impossible; it now parses to UnknownAuth and raises only when that environment is the one being connected. UnknownAuth keeps the sibling keys verbatim, because re-emitting only auth_method would delete data belonging to a method this version cannot parse. Both "before" validators stop mutating their input. ConfigFile.model_validate was injecting an `environments` key into the caller's dict, which the writers then persisted to YAML -- the defensive deepcopy in config_writer was masking it. Both copies are gone. Also adds OAuthCredentialProvider.for_client_credentials / for_refresh / has_token and oauth_token_url, which the variants build on; wiring the connect_* methods onto them follows separately.
`connect` dispatched on the auth_method string and `_connect_oauth_from_config` hosted
the whole interactive-OAuth flow inline, while five connect_* classmethods built
providers themselves and spelled out the token endpoint four times over.
`Bfabric.from_config_data` now asks the config's auth method for a provider, so the
dispatch and the ~35-line OAuth branch both collapse; `_connect_oauth_from_config`
stays as a thin alias. The connect_* methods and WebappClient go through the named
provider constructors, so `{base_url}/rest/oauth/token` is derived in one place.
Loading an OAuth environment now reads the token cache once rather than twice -- the
existence check and the provider's own seeding were separate reads.
ConfigData gains auth_config(), the flat-to-union view. ConfigData itself stays flat:
its field order is pinned by a repr assertion, bfabric_rest_proxy and bfabric_asgi_auth
construct it directly, and the override JSON is documented as hand-written.
The connect_* tests asserted the exact provider constructor call, so they move to the
named constructors; behaviour is unchanged, which the untouched CLI tests confirm.
`Bfabric.__init__` kept only `.client` and `.auth`, so `config_data` and `__repr__` rebuilt a ConfigData missing auth_method, client_id and env_name. Round-tripping an OAuth client's config -- as the planned BFABRICPY_CONFIG_OVERRIDE forwarding in app_runner would -- handed the next process a config silently degraded to no-auth. The existing test only covered the password case, where the loss is invisible. The instance now retains the ConfigData it was built from and `config_data` reapplies the auth currently in effect, so the `with_auth` context manager stays coherent. It is added to the pickle state with a fallback for pickles written before this. `with_auth` uses model_copy instead of enumerating all five fields, which is how the fields came to be dropped in the first place, and config_data's `_read_config_file` is renamed to spell out what it reads -- config_writer has a different function of the same name.
`write_environment_to_config` always replaced the auth-owned keys wholesale, which is right for a fresh login and wrong for a partial update: writing just a rotated secret silently dropped everything else in the group. The mode is now a required keyword rather than an implicit property of the function, since the two directions corrupt in opposite ways -- replace can strand a client by dropping its registration credentials, merge can resurrect a superseded one -- and there is no safe default. A key mapped to None is removed, so callers need no separate delete step. The auth-owned key set is derived from the auth-method variants instead of being a second hand-maintained literal beside the reader's. `read_environment_auth_keys` covers callers that only want to inspect what is stored. `validate_writable_environment` rejects contradictory combinations before anything reaches disk: client_credentials without a secret, a declared pat or password with nothing to authenticate with, an oauth environment carrying a client_secret or pat, half a registration credential pair. Reading stays deliberately tolerant -- 1.21.0 wrote files without these checks, so a stricter reader would lock users out of their own config, and `auth status` has to be able to describe a broken environment.
The writer opened the file with O_TRUNC and wrote in place, so an interrupted write left a truncated config -- the file holding the user's only credentials. A partial write is easy to demonstrate: a failure after the first chunk cut a 105-byte config down to 20 bytes. It now goes to a sibling temp file and is renamed, matching what TokenCache.save already does in this repo. Mode 0600 and "a failed write leaves the previous file untouched" both still hold. The inode changes, though, which matters for anything bind-mounting the file itself rather than its directory: app_runner mounts ~/.bfabricpy.yml into containers, so a container running across a host-side re-login keeps reading the old inode. That is better than reading a truncated file, but it is a behaviour change.
`auth status` had a four-way elif chain on the auth_method string, and `auth logout` / `auth remove` decided whether to clear the token cache by reading `env.auth_method` directly rather than through `_auth_method()` -- so a legacy password environment was classified one way for display and another for cache clearing. Both now ask the auth method, via a match and a named `_uses_token_cache` predicate. An unrecognised auth_method is now named in the output with a note to upgrade. Previously the reader coerced it to None along with its sibling client_id, so `auth status` reported the environment as having no auth method at all and a later write would have erased both. Display output for every shape that parsed before is unchanged.
Records where the flat YAML is translated, so the next change adds a variant rather than a second list of auth keys somewhere else, and describes the tolerant-read/strict-write split and the writer's new merge/replace mode. Changelog entries for both packages, including the inode change from the atomic write.
Every variant hand-wrote the same loop: include the non-None owned keys, unwrap SecretStr. The base class derives it from owned_keys and a declared_method, so adding an auth method means declaring its fields and nothing else. UnknownAuth keeps its own, since its method name is data rather than a class constant.
The variants could serialise themselves back to flat YAML, but nothing ever asked them to: the CLI hands write_environment_to_config the flat keys directly, so the union is only ever used to read. That left to_flat, declared_method, _FLAT_KEY_ORDER, UnknownAuth.extra and an auth_method_written flag existing purely to make the unused direction correct. The one thing that direction was really carrying is whether the file declared an auth_method at all, which `auth_method` needs so an undeclared login/password environment keeps reporting None. That is now a `declared_name` field holding what the file said, so the property is a read rather than a derivation, and UnknownAuth needs no separate name. The two properties the round-trip tests covered -- a legacy environment not gaining an auth_method, and an unsupported method keeping its sibling keys -- are asserted against the writer instead, which is what actually guarantees them: it never touches keys the payload does not mention.
"The config format is frozen" was too cautious and would have blocked shipping new config options for no reason. The real constraint is narrower: new keys inside an environment are invisible to a <=1.19.0 reader, as is a whole new environment using an auth_method it does not know. What breaks such a reader -- for the entire shared file -- is a new *top-level* key, since anything outside GENERAL is taken as an environment name; nesting credentials under `auth`, which is its own field name; an environment with no base_url; and a login paired with a non-32-character password. A version field cannot relax any of this: app_runner bind-mounts the host config into containers whose image may ship an older bfabric, so those readers are already deployed and will never look for one. Each rule now has a test against the vendored 1.19.0 replica, so the boundary is executable rather than folklore, and the design doc says which side of it a new option falls on.
Rebase fixups onto #596: oauth_token_url and the provider constructors take a BaseUrl and drop the defensive rstrip, since the type already guarantees no trailing slash, and the atomic-write test mocks safe_dump rather than dump.
The four declarable auth_method values were spelled out four times: twice as a Literal (ConfigData and the EnvironmentConfig property), once as the _KNOWN_METHODS set the parser guards on, and once across the variants' own kind fields. They are now one AuthMethodName alias, with _KNOWN_METHODS derived from it via get_args. That also removes a pyright suppression: the property claimed the narrow Literal while returning a plain `str | None`, so the type was only accidentally true. `declared_name` now carries the alias, and UnknownAuth holds the value it could not match in its own `unknown_name` field -- which is what the CLI prints -- rather than borrowing the field whose type says the value is known.
`auth_config()` re-flattened ConfigData into YAML-shaped keys and fed them back through auth_method_from_flat, so structured data was round-tripped through strings to recover structure it already had. The flat form could not represent a PAT -- ConfigData keeps the token in `auth` as login/password, not under `pat` -- so every PAT connect logged "declares auth_method 'pat' but has no matching credentials" and classified itself as NoAuth. It worked only because a PAT needs no provider and Bfabric.auth falls back to the stored auth. The single caller only ever wanted the provider, so both methods and their hand-written key list collapse into one `credential_provider()` that constructs the variant directly.
typing.override is 3.12+, but bfabric declares requires-python >=3.11. On 3.11 the import raised, and since every test module reaches config transitively, all four 3.11 CI jobs died with 49 collection errors. Dropping the decorators outright would trade that for a basedpyright failure: reportImplicitOverride is enabled, so the five overriding methods each warned. typing_extensions backports override to 3.11 and keeps the checker satisfied. It was already in the tree via pydantic, but declare it explicitly rather than lean on a transitive dep. The >=4.4.0 floor is the release that added override, verified against the lowest-direct resolution.
The union was parsed from the flat YAML, flattened back into strings via properties, copied into ConfigData, then re-instantiated from that copy to build a credential provider. ConfigData now holds the union itself, so the provider comes straight off it and the round trip is gone. - Replace ConfigData's flat auth_method/client_id fields with the union; both are kept as read-only properties, and a model validator still accepts the flat kwargs so the BFABRICPY_CONFIG_OVERRIDE wire format is unchanged. - Collapse declared_name to a `declared` bool and derive the name from the discriminator, dropping the name-echoing ternaries in auth_method_from_flat. - Pass an existing SecretStr through unwrapped when parsing. str() on one yields '**********', so re-wrapping an in-memory value replaced the secret with the mask; only reachable now that a parsed model is re-parsed. - Drop _connect_oauth_from_config, a private alias with no callers, and needs_credential_provider, which was never called at all. - Trim test_frozen_contracts.py to the two invariants no other module covers. The rest pinned refactor scaffolding — field order, key sets and modes all already asserted in test_config_writer/test_config_data/test_auth_methods. Claude-Session: https://claude.ai/code/session_01RZNr7BRtYpGSP4EXNgp8hk
The section still described the pre-union shape: flat fields, three auth methods, and routing through _connect_oauth_from_config, which no longer exists. Also record the SecretStr re-wrapping fix. Claude-Session: https://claude.ai/code/session_01RZNr7BRtYpGSP4EXNgp8hk
The variants become plain parsed data: resolve_static_auth and resolve_credential_provider match on them instead of each class overriding a base-class hook. The two stay separate because EnvironmentConfig.auth has to answer without a base URL, an environment name, or a token-cache read, which a single resolver could not do. declared/declared_name collapse into one field the parser sets, which also removes the last @OverRide and with it the typing-extensions dependency.
Pickles are not a cross-version contract, so drop the .get fallbacks and the duplicated config/auth entries: config_data already carries both.
Both resolvers ended in a catch-all returning None, so a new variant that carried credentials would silently produce an unauthenticated client. List every variant instead and close with assert_never, which turns that into a type error at the two call sites rather than a runtime surprise. This needs the union as a plain alias (AnyAuthMethod) for narrowing; AUTH_METHOD_CLASSES is then derived from it instead of repeating the six names, so a variant is registered by adding it to the union alone.
They had landed under the already-published 1.21.0 heading, and the stray blank line was splitting that section's list in two.
They were written under [Unreleased] before 1.22.0 was cut; the release renamed that heading, so they ended up announced under a version that shipped without them. nox -s changelog matches the version heading literally, so 1.22.0 would claim four fixes its users never got and the next release would announce none. Restores the 1.22.0 section to exactly what the bfabric/1.22.0 tag published.
refactor(bfabric): model the auth method as a discriminated union
Adds an `auth_method: client_credentials` environment that authenticates from an inline `client_secret`, so cron jobs and scripts connect without a browser or a cached token, and `bfabric-cli auth service-account` to record one. Registration responses can now be saved with `auth register --save-env`, including the RFC 7592 `registration_access_token` / `registration_client_uri`. That makes `auth client-show` / `client-update` / `client-delete` possible, so a client registered with a wrong redirect URI is corrected in place rather than re-registered. Two write paths need care, both verified against the live TEST instance: B-Fabric rotates *both* the client secret and the registration access token on every successful update and revokes the old ones immediately (confirmed: the previous token returns 401). Since the config writer replaces every auth-owned key at once, writing only the rotated keys would drop the untouched ones and strand the client unmanageable. `merge_auth_owned_keys` carries the rest along. Rotation is detected by comparing against the stored value rather than testing for presence, so a server echoing an unchanged credential reports nothing. `--save-env` into an existing environment would likewise replace its stored credentials, so it now refuses unless `--force` is passed. Also refuses `api delete` / `api update` without a terminal to confirm on: unattended service-account runs would otherwise hang or die on a prompt.
A client_credentials grant has no authorization step to inherit a scope from, so the token request is the only place one can be expressed. The service account passed scope="" and dropped the scope the operator recorded, taking whatever the server granted by default while `auth status` displayed the narrower scope as though it applied. Carry `scope` on ConfigData (through the file reader and the JSON export, so it survives BFABRICPY_CONFIG_OVERRIDE) and pass it to the provider. An empty scope omits the parameter, keeping the server default as the fallback. The interactive path keeps scope="": its scope is fixed at authorization time and a refresh inherits it, so re-requesting one could only narrow the token below what the user consented to. A regression test pins that.
`auth service-account` rewrote auth_method on an existing environment with no prompt, so every later connect() authenticated as the service account instead of whoever was configured. The superseded login/password stayed behind in the file, unreachable but still a stored secret. Ask before converting, refuse without a terminal (matching _confirm_repoint), and drop the superseded credentials on the way. A re-run against an environment that is already a service account does not prompt — that is the documented path for storing a rotated secret. Also correct save_registration's guard docstring: it claimed the write would strip the credentials an existing login authenticates with, which merge_auth_owned_keys made false. The real hazard is client_id being replaced while auth_method stays "oauth", leaving the token cache (keyed on client_id) unable to match. Pinned by a test.
The feature was written against merge_auth_owned_keys, which the refactor replaced with an explicit auth mode. The four call sites move to write_environment_to_config(auth="merge") and read_environment_auth_keys, so the read-modify-write sandwich and its extra file reads are gone. auth client-delete now also clears auth_method when the environment was a service account: validate_writable_environment refuses to persist client_credentials without a secret, which is exactly the state deleting the client leaves behind.
…ve path save_registration and _resolve_token_from_config took a plain str, so a BaseUrl was cast back down to str at the two call sites and then re-normalised inside save_registration. Type both on BaseUrl and normalise once, where the CLI argument enters. Also drops an inconsistency in cmd_login_register, which passed the normalised URL to register_client but the raw one to save_registration.
save_registration re-read the environment it had just written to decide whether to warn about missing registration credentials. The merge cannot add keys the server did not return, so the response already answers the question. Also repoint the service-account tests at from_config_data, replacing the _connect_oauth_from_config alias dropped in the parent branch. Claude-Session: https://claude.ai/code/session_01RZNr7BRtYpGSP4EXNgp8hk
…nion feat(bfabric): OAuth service accounts and RFC 7592 client management
…tials __setstate__ required the config_data key introduced in the auth-method refactor, so any client pickled by 1.22.0 or earlier failed to load with KeyError. Read the legacy config/auth keys when config_data is absent, and restore the .get() for credential_provider that predates it. _registration.py logged whole registration and client-management responses at debug level, putting client_secret and registration_access_token into any enabled sink. Log the client_id and the response key names instead. Both found by the Copilot review on the release PR.
fix(bfabric): restore legacy unpickling and stop logging OAuth credentials
load_config_data cleared only auth, leaving auth_config intact, so an include_auth=False client still resolved a credential provider and a service account authenticated anyway. Clear the auth method too, via a new ConfigData.without_credentials(). auth client-delete left client_id and scope behind. Either key alone parses as an undeclared interactive OAuth environment, so the next connect() read a stale token cache instead of reporting the client as gone. Also collapse the duplicated Added/Changed/Fixed headings in the unreleased bfabric_scripts changelog, which two PRs had each appended their own set of, and drop the one entry that appeared verbatim twice. Found by the Copilot review on the release PR.
fix: honour include_auth=False and fully clear a deleted OAuth client
…unner 0.9.0 Graduate each [Unreleased] section to its dated version heading and bump the bfabric floor in the two dependent packages, following each one's established pattern (scripts <2, app_runner pinned to the minor). Release mechanics only; no code changes.
Contributor
🚀 Release PreviewThe following packages will be released when this PR is merged:
This comment is automatically updated when the PR changes. |
Contributor
✅ Release Distribution TestingPackages to release: bfabric, bfabric_scripts, bfabric_app_runner Test Results:
Status: All tests passed with built distributions. The packages are ready for release. Last updated: 2026-09-16T12:13:51.809Z |
cpanse
approved these changes
Sep 17, 2026
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.
No description provided.