MPT-22108 add phase-step provisioning-phase gating library#53
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds the ChangesPhase-step package addition
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
from
c289caf to
e4ca140
Compare
✅ Found Jira issue key in the title: MPT-22108 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@status-step/docs/usage.md`:
- Around line 52-72: The later usage examples are not self-contained because
they use `@override` without importing it, so copied snippets will fail. Update
each affected code block that defines classes like CreateSubscription,
PurchasePipeline, and the other status-step examples to either include the
missing from typing import override import or explicitly note that the snippet
continues from the prior example and is not standalone.
In `@status-step/README.md`:
- Around line 71-91: The README example uses the `@override` decorator in
CreateSubscription.process without importing it, so make the snippet copy/paste
runnable by adding the missing override import alongside the existing
BasePipeline, BaseStep, and OrderContext imports, or otherwise clearly indicate
it continues from the prior example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ae867f99-b4de-4aff-9282-d101c4d869bd
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/CODEOWNERSAGENTS.mdREADME.mdmake/common.mkpyproject.tomlstatus-step/.copier-answers.ymlstatus-step/AGENTS.mdstatus-step/LICENSEstatus-step/README.mdstatus-step/docs/architecture.mdstatus-step/docs/contributing.mdstatus-step/docs/releases.mdstatus-step/docs/testing.mdstatus-step/docs/usage.mdstatus-step/mpt_extension_contrib/status_step/__init__.pystatus-step/mpt_extension_contrib/status_step/parameters.pystatus-step/mpt_extension_contrib/status_step/py.typedstatus-step/mpt_extension_contrib/status_step/steps.pystatus-step/pyproject.tomlstatus-step/tests/conftest.pystatus-step/tests/test_parameters.pystatus-step/tests/test_steps.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the package or repository-level concern affected by the task.
- Read only the relevant local files before making changes.
- Read the matching shared standard or operational guidance from
softwareone-platform/mpt-extension-skills
when the task involves shared engineering conventions (resolved locally from
${MPT_EXTENSION_SKILLS_HOME:-$HOME/.mpt-extension-skills}/current).- Treat repository-local documents as additions, restrictions, or overrides to
shared guidance.- If a repository-local rule conflicts with a shared or global rule, the
repository-local rule takes precedence.Repository model
mpt-extension-contribis a uv workspace monorepo
of independently released Python libraries for SoftwareONE MPT extensions. It does
not publish an umbrellampt-extension-contribdistribution.
- The root
pyproject.tomldefines the uv workspace and the shared
tool configuration (ruff, flake8, mypy, pytest, coverage). It is not an
installable distribution.- A single
uv.lockat the repository root locks the whole workspace.- Each root-level directory is one distribution. All distributions share the
PEP 420 namespace packagempt_extension_contrib.Mechanical naming rule for a module
<module>:Repository directory: <module> (kebab-case) Distribution name: mpt-extension-contrib-<module> Python import: mpt_extension_contrib.<module_with_underscores>Packages
shared/: internal reusable code exposed asmpt_extension_contrib.shared
(distributionmpt-extension-contrib-shared).order-status/: public package exposed asmpt_extension_contrib.order_status(distributionmpt-extension-contrib-order-status).- [
custom-notifications/](cus...
Files:
status-step/docs/releases.mdREADME.mdstatus-step/docs/testing.mdstatus-step/mpt_extension_contrib/status_step/__init__.pystatus-step/mpt_extension_contrib/status_step/parameters.pystatus-step/docs/contributing.mdpyproject.tomlstatus-step/pyproject.tomlAGENTS.mdstatus-step/AGENTS.mdstatus-step/LICENSEstatus-step/tests/conftest.pymake/common.mkstatus-step/tests/test_parameters.pystatus-step/docs/architecture.mdstatus-step/README.mdstatus-step/tests/test_steps.pystatus-step/mpt_extension_contrib/status_step/steps.pystatus-step/docs/usage.md
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
status-step/docs/releases.mdREADME.mdstatus-step/docs/testing.mdstatus-step/mpt_extension_contrib/status_step/__init__.pystatus-step/mpt_extension_contrib/status_step/parameters.pystatus-step/docs/contributing.mdpyproject.tomlstatus-step/pyproject.tomlAGENTS.mdstatus-step/AGENTS.mdstatus-step/LICENSEstatus-step/tests/conftest.pymake/common.mkstatus-step/tests/test_parameters.pystatus-step/docs/architecture.mdstatus-step/README.mdstatus-step/tests/test_steps.pystatus-step/mpt_extension_contrib/status_step/steps.pystatus-step/docs/usage.md
README.md
📄 CodeRabbit inference engine (Custom checks)
README.md: UpdateREADME.mdordocs/usage.mdwhen a change affects user-facing or public behavior and commands.
UpdateREADME.mdordocs/usage.mdwhen a change adds or changes CLI commands, followingstandards/cli.md.
Files:
README.md
status-step/mpt_extension_contrib/status_step/**
📄 CodeRabbit inference engine (status-step/AGENTS.md)
status-step/mpt_extension_contrib/status_step/**: Keep the public API undermpt_extension_contrib.status_step.
Keep the library free of product-specific status values and parameter ids; these must come from the extension viaStatusStepSettings.
Files:
status-step/mpt_extension_contrib/status_step/__init__.pystatus-step/mpt_extension_contrib/status_step/parameters.pystatus-step/mpt_extension_contrib/status_step/steps.py
pyproject.toml
📄 CodeRabbit inference engine (AGENTS.md)
pyproject.toml: The rootpyproject.tomldefines the uv workspace and shared tool configuration (ruff, flake8, mypy, pytest, coverage) and is not an installable distribution.
Add shared development tools to the rootpyproject.toml.
Files:
pyproject.toml
*/pyproject.toml
📄 CodeRabbit inference engine (AGENTS.md)
Add package-specific runtime and development dependencies to the matching package
pyproject.toml.
Files:
status-step/pyproject.toml
AGENTS.md
📄 CodeRabbit inference engine (Custom checks)
Update
AGENTS.mdwhen a change introduces or modifies top-level structure or agent navigation.
Files:
AGENTS.md
make/**
⚙️ CodeRabbit configuration file
make/**: Review changes inmake/againstdocs/contributing.mdand the linked repository'sstandards/makefiles.md.
Use the shared standard as the source of truth for Makefile architecture, file layout, and command-group organization.
Files:
make/common.mk
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-06T14:15:57.706Z
Learning: When working on a task, first identify the package- or repository-level concern it affects before making changes.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-06T14:15:57.706Z
Learning: Read only the relevant local files before making changes.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-06T14:15:57.706Z
Learning: When a task involves shared engineering conventions, read the matching shared standard or operational guidance from `softwareone-platform/mpt-extension-skills` (resolved locally from `${MPT_EXTENSION_SKILLS_HOME:-$HOME/.mpt-extension-skills}/current`).
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-06T14:15:57.706Z
Learning: Treat repository-local documents as additions, restrictions, or overrides to shared guidance.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-06T14:15:57.706Z
Learning: If a repository-local rule conflicts with a shared or global rule, the repository-local rule takes precedence.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-06T14:16:01.638Z
Learning: Run `make check-all pkg=status-step` from the repository root while iterating.
📚 Learning: 2026-07-03T13:40:50.305Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-extension-python-contrib PR: 50
File: order-status/mpt_extension_contrib/order_status/templates.py:29-56
Timestamp: 2026-07-03T13:40:50.305Z
Learning: When using `TemplateService.get_template(product_id, status, name=template_name)` from the `mpt-extension-sdk` dependency, assume it already implements “named template or default fallback” in a single call (fetches the named template for that status, and if it doesn’t exist, returns the default). Therefore, callers should not issue an extra, separate query for the default template when the named one is missing—just call `get_template(..., name=...)` and use the returned template. This is consistent with the SDK source/tests (e.g., `tests/services/mpt_api_service/test_template.py`) and the method docstring stating it falls back to default.
Applied to files:
status-step/mpt_extension_contrib/status_step/__init__.pystatus-step/mpt_extension_contrib/status_step/parameters.pystatus-step/tests/conftest.pystatus-step/tests/test_parameters.pystatus-step/tests/test_steps.pystatus-step/mpt_extension_contrib/status_step/steps.py
📚 Learning: 2026-06-30T11:25:32.303Z
Learnt from: d3rky
Repo: softwareone-platform/mpt-extension-python-contrib PR: 45
File: custom-notifications/tests/conftest.py:35-40
Timestamp: 2026-06-30T11:25:32.303Z
Learning: In softwareone-platform/mpt-extension-python-contrib, Ruff rule S106 is already ignored for all `tests/**` via `[tool.ruff.lint.per-file-ignores]` in the root `pyproject.toml`. When editing test code (e.g., `custom-notifications/tests/conftest.py`), avoid adding inline `# noqa: S106` suppressions if the file is covered by that central `tests/**` ignore—Ruff will already skip S106 for these paths per repository configuration.
Applied to files:
status-step/tests/conftest.pystatus-step/tests/test_parameters.pystatus-step/tests/test_steps.py
📚 Learning: 2026-06-16T09:48:32.998Z
Learnt from: d3rky
Repo: softwareone-platform/mpt-extension-python-contrib PR: 9
File: make/common.mk:11-29
Timestamp: 2026-06-16T09:48:32.998Z
Learning: In SoftwareONE Platform repos that use the shared Makefile convention, do not add explicit `.PHONY` declarations inside `make/*.mk` included make snippet files. The root `Makefile` centrally auto-declares phony targets by scanning `MAKEFILE_LIST` (e.g., via the awk-based `.PHONY: $(shell awk ...)` line). Since targets from included `make/*.mk` files are already present in `MAKEFILE_LIST`, per-file `.PHONY` blocks are redundant and would diverge from the org standard. Only suggest explicit `.PHONY` in `make/*.mk` if this repo does not use the shared root Makefile convention / phony auto-scan.
Applied to files:
make/common.mk
🪛 LanguageTool
status-step/AGENTS.md
[style] ~10-~10: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re.md) for the public API boundary. 4. docs/contributing.md ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~11-~11: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ng.md) before changing this module. 5. docs/testing.md before cha...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../testing.md) before changing tests. 6. docs/releases.md before r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔀 Multi-repo context softwareone-platform/mpt-extension-skills
Linked repositories findings
softwareone-platform/mpt-extension-skills
[::softwareone-platform/mpt-extension-skills::] standards/sdk-pipeline-steps.md:2-11, 37-47, 75-90 directly supports the new status-step design:
- use
SkipStepErrorfor skip/“is there work to do?” guards, - put those guards in
pre(), - keep
process()focused on work, - treat
ParameterBagas immutable and persist viawith_fulfillment_value(...).
This matches the new require_status / StatusGatedStep API and the new get_status / set_status helpers, so the added package is aligned with the shared SDK guidance rather than introducing a conflicting pattern.
🔇 Additional comments (29)
.github/CODEOWNERS (1)
9-9: LGTM!AGENTS.md (1)
42-42: LGTM!README.md (1)
16-16: LGTM!make/common.mk (1)
5-5: LGTM!pyproject.toml (1)
1-8: LGTM! Workspace members, pytesttestpaths, and mypymypy_pathare all consistently updated in the same relative position as the other package lists.Also applies to: 135-144, 208-219
status-step/.copier-answers.yml (1)
1-5: LGTM!status-step/pyproject.toml (3)
9-9: LGTM!pydantic>=2.13,<3is a valid, currently-released version constraint.
1-27: LGTM on the rest of the packaging metadata (build backend, wheel/sdist targets,requires-python).
8-8: 🎯 Functional Correctness
mpt-extension-sdk>=6.3,<7is resolvable.> Likely an incorrect or invalid review comment.status-step/mpt_extension_contrib/status_step/__init__.py (1)
1-14: LGTM!status-step/mpt_extension_contrib/status_step/parameters.py (1)
1-36: LGTM!get_status reads a fulfillment value (returning None when unset/empty), and set_status returns a copy of the ParameterBag with a fulfillment value set or cleared (status=None clears it) via with_fulfillment_value, matching the immutable
ParameterBagconvention from the linked shared SDK standard.status-step/mpt_extension_contrib/status_step/steps.py (4)
23-25: LGTM!Reads the provisioning status from the fulfillment parameter named by the extension settings field status_parameter and checks it against expected_statuses ... Raises: SkipStepError: When the current status is not one of the expected ones. No hardcoded product-specific statuses or parameter ids appear in the library, consistent with the requirement that these come from the extension via
StatusStepSettings.Also applies to: 44-65
68-95: LGTM!Matches the PR's stated design where subclasses override
pre()and explicitly callawait super().pre(context);expected_statusesreturns a defensive copy, avoiding external mutation of internal state.
1-1: 🎯 Functional Correctness
typing.overrideis compatible with the package target.
status-steprequires Python>=3.12, so this import is supported here.> Likely an incorrect or invalid review comment.
84-86: 🩺 Stability & AvailabilityRemove this concern:
StatusGatedStep.__init__only stores per-instance state, and the repo’sBaseStepimplementations set fields directly without chainingsuper().__init__().> Likely an incorrect or invalid review comment.status-step/tests/conftest.py (2)
20-30: LGTM!The status_parameter_factory fixture constructs ParameterValue(external_id="phase", value=stored_value); these tests rely on get_status fetching that same external_id and normalizing empty/unset values to None.
10-17: 🎯 Functional CorrectnessCheck dataclass field ordering with
BaseExtensionSettings.StatusStepExtensionSettingsadds a defaulted field on top ofBaseExtensionSettings; if the base class defines any non-default dataclass fields, class creation will fail withTypeError: non-default argument follows default argument.status-step/tests/test_parameters.py (1)
1-55: LGTM!Covers unset, empty, stored-value read, write, input-bag immutability, and clearing via
None— matchingset_status's documented behavior that set_status produces a new ParameterBag with updated/cleared fulfillment value based on status argument (None clears; non-None stores) and does not mutate the input bag.status-step/tests/test_steps.py (3)
14-33: LGTM!
SampleGatedStep/SampleGatedStepWithPrecleanly exercise the "explicitawait super().pre(context)" convention documented forStatusGatedStep.
35-100: LGTM!Good boundary coverage for
require_status/StatusGatedStepconstruction: single status, multi-status match, mismatch, unset, and pydantic validation rejection of empty string/list/list-element.
103-174: LGTM!StatusGatedStep.pre delegates gating to require_status, establishing the condition under which process is (or is not) reached during step.run(), and these async tests correctly verify both the run-through and skip paths, including the subclass
pre()chaining case.status-step/LICENSE (1)
1-202: LGTM!status-step/AGENTS.md (1)
1-22: LGTM!status-step/README.md (1)
1-66: LGTM!Also applies to: 93-103
status-step/docs/architecture.md (1)
1-83: LGTM!status-step/docs/contributing.md (1)
1-22: LGTM!status-step/docs/releases.md (1)
1-9: LGTM!status-step/docs/testing.md (1)
1-25: LGTM!status-step/docs/usage.md (1)
1-50: LGTM!Also applies to: 74-85, 96-100, 113-122, 139-141
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
from
e4ca140 to
049509f
Compare
| class SetDueDateWhenPending(SetDueDate): | ||
| @override | ||
| async def pre(self, context: OrderContext) -> None: | ||
| require_status(context, "pending") |
There was a problem hiding this comment.
Let's compare number of different cases you need to implement using different approaches.
-
Option number 1 --> mixin class --> 3 different cases (your PR)
-
Option number 2 --> decorator
a class
@require_status("pending)
class CreateSubscription(BaseStep):
@override
async def pre(...):
if _subscription_already_exists(context.order):
raise SkipStepError()
a class from another library
@require_status("pending")
class SetDueDateWhenPending(SetDueDate):
pass
status in the pipeline
class PurchasePipeline(BasePipeline):
@property
def steps(self) -> list[BaseStep]:
return [
@require_status("pending")
CreateSubscription(...),
# ... more status-gated steps ...
]
So having just a smart decorator number of permutations on how to implement the library decreases from 3 different approaches in your PR --> to just 1 --> use the decorator
And I think at least this approach wins if we compare with your current solution
There was a problem hiding this comment.
Thanks — comparing the number of cases each approach forces on the user is the right lens. But I think two of the three cases here don't exist for us, and the one that remains isn't a mixin.
StatusGatedStep is a base class, not a mixin. It's a drop-in replacement for the SDK's BaseStep, just gated. An extension author uses it exactly the way they already use BaseStep: need a normal step → subclass BaseStep; need a status-gated step → import StatusGatedStep from mpt_extension_contrib and subclass it. Same import-and-subclass pattern, single inheritance, no MRO. So the "mixin → 3 cases" framing doesn't match what the PR ships.
Two of the three cases won't happen for us:
-
Gate a step from another library (
SetDueDateWhenPending(SetDueDate)) — this never occurs. We own every repo, and we deliberately created the SDK andmpt_extension_contribprecisely so extensions never import steps from one another. The moment a step is worth sharing, we move it intompt_extension_contrib— that's the reason contrib exists. So there's no "foreign step to gate." I'll own this: the "Gate a step you do not own" docs section and option 4 in my earlier comparison shouldn't have been there — I'll delete them; that scenario won't happen. -
Apply gating in the pipeline (
@require_status(...)on an entry in thestepslist) — also not a real case for us, and as written it isn't valid Python: a decorator can only precede adef/class, not an instance inside a list literal.
That leaves exactly one real case: an extension needs an extension-specific gated step. So both proposals actually collapse to a single mechanism — the comparison is just "subclass a gated BaseStep" vs "a class decorator that rewrites pre()". For that one case I still prefer the subclass:
- It mirrors
BaseStep, which the team already subclasses everywhere — nothing new to learn. - When the step also needs its own
pre()(your first example has exactly that), the subclass composes explicitly —await super().pre(context)then the extra guard — with ordering visible in the source. A class decorator has to wrap the user'spre()and decide that ordering implicitly, which is the implicit-ordering problem we set out to avoid. - The class keeps its own type and
name; there's no wrapper layer to forward for logging/tracing.
So: for the single case that actually happens, decorator and base class are both "one approach," and the base class is the more explicit one and consistent with how the SDK is already used.
🤖 Generated by AI
There was a problem hiding this comment.
BTW many of these issues are created by what I believe is a wrong design. Do you remember when I was complaining about skipping steps, switching them on and off based on some factors on following passes through the pipeline? From the beginning I felt something is off here. Now with small help from our AI friend I created a dirty article describing my doubts. Some details may not be 100% accurate, let me know if it is the case, it was generated by AI and since it is just a dirty idea may be fixed later when needed.
https://softwareone.atlassian.net/wiki/spaces/mpt/pages/7673020434/Order+fulfillment+status-dispatch+vs+pipeline-with-steps
There was a problem hiding this comment.
That's totally wrong
Gate a step from another library (SetDueDateWhenPending(SetDueDate)) — this never occurs
It doesn't mean that we are not going to have it in the future. And having more shared steps the probability is going to increase. So you are just ignoring the requirement that phase step should be composable with libraries step
|
|
||
| class CreateSubscription(StatusGatedStep): | ||
| @override | ||
| async def process(self, context: OrderContext) -> None: |
There was a problem hiding this comment.
uff, that's what I would like to avoid. This code is going to be copy-pasted in all StatusGatedStep that you have :-(
Maybe we should add it to the decorator also and have a code that just moves the status in process or post handlers
@status("pending", move_to="completed")
There was a problem hiding this comment.
Gating and advancing the status are two different responsibilities, and I'd keep them apart:
- Gating is a read. The base class answers one question in
pre()— "should this step run for the current status?" — and never writes. - Advancing is a write. Moving the phase forward is a business decision the step makes explicitly in
process(): when to advance and to which next status. That belongs in the subclass, not the framework.
Keeping each responsibility in one place is exactly what stops the status from being mutated half in the base class and half in the subclass. Folding the advance into the base/decorator collapses that separation back together.
It also can't cover our real cases: advancement is usually conditional — advance only if the work succeeded, and to a different next status depending on the outcome (see the AWS phase transitions). And post() runs even when process() fails, so a blanket "move to X" would advance the phase on failures too.
You're right that copy-pasting set_status + orders.update into every step isn't nice — but the fix is to remove the duplication, not the separation. I've added a one-line helper, await advance_status(context, "completed") (does set_status + persist), so a step that advances calls one line instead of three, with the write still explicit and in the subclass. Done in 5b4c039.
🤖 Generated by AI
There was a problem hiding this comment.
It also can't cover our real cases: advancement is usually conditional — advance only if the work succeeded, and to a different next status depending on the outcome (see the AWS phase transitions). And post() runs even when process() fails, so a blanket "move to X" would advance the phase on failures too.
It only means that library should support both scenarios --> default and simple one. Step is proceed - move to the next transition. And have a way for more advanced logic to be implemented.
The main concern for me still is the about of biolerplate code that you should write.
Also keep in mind that libary should have capability to skip status update
| def set_status( | ||
| parameter_bag: ParameterBag, | ||
| status: str | None, | ||
| parameter_external_id: str, |
There was a problem hiding this comment.
Does it make sense to have this function if you still need to pass the external id of the parameter here?
it requires even more parameters if you just write
parameter_bag.with_fulfillment_value(parameter_external_id, status)
and with your function
set_status(parameter_bag, status, parameter_external_id)
So for me it doesn't incapsulate any logic inside :-( just proxying parameters in different order
There was a problem hiding this comment.
Good point — set_status is just with_fulfillment_value with the arguments reordered; it encapsulates nothing, so I've dropped it. Same really goes for get_status (its only extra was normalizing blank→None, which doesn't even change gating), so I removed that too and inlined both reads/writes where they're used.
That leaves the API as just the behavior-bearing pieces: require_status (read + compare + skip) and advance_status (set + persist via orders.update), plus StatusGatedStep and StatusStepSettings. require_status reads the parameter with get_fulfillment_value(...) directly, and advance_status writes with with_fulfillment_value(...) directly — no proxy layer in between. Done in 5aefa67.
🤖 Generated by AI
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
3 times, most recently
from
5b4c039 to
5aefa67
Compare
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
from
5aefa67 to
0ea9449
Compare
| @property | ||
| def steps(self) -> list[BaseStep]: | ||
| return [ | ||
| CreateSubscription("createSubscription"), |
There was a problem hiding this comment.
This example feels a bit strange. I would expect to pass a status like Processing or Active. Could we change it?
There was a problem hiding this comment.
I've realized we can use phase or status or whatever attribute for this check. I find the use of status a little confusing because Order.status, Agreement.status, and similar fields are already a well-known domain concepts, while this library reads and writes an arbitrary fulfillment parameter whose name could be phase, status or something else.
A developer could reasonably assume that advance_status change Order.status rather than the value of a fulfilment parameter
Should we use a generic parameter oriented names instead? For example, FulfillmentParameterGatedStep, ParameterValueGatedStep, ParameterConditionStep, ... I think FulfillmentParameterGatedStep describes the current behavior most precisely.
There was a problem hiding this comment.
Good point, and it's pointing at a naming problem worth fixing properly. The value here is a provisioning phase — the custom fulfillment parameter from step 2 that tracks where in the fulfillment flow the order is (e.g. createSubscription, checkOnboardStatus, from the AWS source). It's deliberately not the MPT order status (Processing, Querying, Completed), which lives on context.order.status — so Processing/Active wouldn't actually fit here.
But you've hit the real issue: calling it "status" collides with order.status, which is exactly what makes this confusing. "phase" matches nothing in the platform, and it's what the source we're extracting from already uses (PhasesEnum, get_phase/set_phase, a phase parameter). So rather than just clarifying the docs, I think the right fix is to rename the concept end-to-end to phase:
StatusGatedStep→PhaseGatedSteprequire_status→require_phase,advance_status→advance_phaseStatusStepSettings.status_parameter→PhaseStepSettings.phase_parameter- module / distribution / import:
status-step→phase-step
The package isn't on PyPI yet (0.0.0), so there are no consumers and the rename is essentially free right now — it would be a breaking change after the first release.
@d3rky — you originally scoped this as the "Status Step" library (MPT-22108); are you OK renaming the concept to phase to disambiguate from order status? If so, I'll apply it across the code/docs/workspace wiring and retitle the ticket + PR. Susana, this rename is the proper answer to your comment.
🤖 Generated by AI
There was a problem hiding this comment.
Yup, I agree here, better to keep status to the order and agreement entity. Phase is better here
There was a problem hiding this comment.
Done — renamed the concept to phase throughout in ed27709:
PhaseGatedStep,require_phase,advance_phase,PhaseStepSettings.phase_parameter- module / distribution / import:
status-step→phase-step - usage §2 now also notes the provisioning phase is distinct from the MPT order status (
context.order.status)
PR title and the Jira tickets (MPT-22108 / MPT-22158 / MPT-22159) are retitled to "Phase Step" too. Thanks @d3rky for the sign-off and @svazquezco for catching it.
🤖 Generated by AI
| ``` | ||
|
|
||
| To run a step for any of several statuses, pass a list instead of a single | ||
| value — `CreateSubscription(["createSubscription", "createExistingSubscription"])`; |
There was a problem hiding this comment.
Same comment here about the steps 😉
There was a problem hiding this comment.
Same root cause as the example above — the "status" wording colliding with order.status. I've proposed renaming the concept end-to-end to phase in that thread (tagged @d3rky for sign-off). This list example will be updated the same way, so I'll fold this thread into that decision.
🤖 Generated by AI
There was a problem hiding this comment.
Done in ed27709 — this example now uses the phase vocabulary (PhaseGatedStep, phase values), as agreed in the rename thread.
🤖 Generated by AI
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
2 times, most recently
from
03a85e6 to
720b1a6
Compare
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
2 times, most recently
from
99c6218 to
ed27709
Compare
jentyk
changed the title
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
from
ed27709 to
8f795de
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phase-step/docs/releases.md`:
- Around line 3-8: Update the release criteria in the package’s release
documentation to include implementation changes that fix defects or alter
behavior, in addition to API and declared dependency changes. Preserve the
independent phase-step tag format and repository-wide release workflow
references.
In `@phase-step/docs/usage.md`:
- Around line 102-107: The CreateSubscription example references the undefined
_subscription_already_exists predicate. Define this helper within the example,
or explicitly label it as an application-provided extension that must be
supplied before use, while preserving the existing pre method behavior.
- Around line 17-20: Clarify in the usage documentation that the phase
fulfillment parameter must be initialized before any gated step executes, since
PhaseGatedStep.pre() skips when it is unset. State that initialization can come
from product configuration or an ungated first pipeline step, while preserving
the existing guidance about using the current provisioning phase.
- Around line 149-153: Update the low-level example around
context.mpt_api_service.orders.update to use the configured phase parameter from
context.ext_settings.phase_parameter instead of hardcoding "phase". Preserve the
existing fulfillment value "completed" and persistence behavior.
In `@phase-step/README.md`:
- Around line 19-20: Update the phase initialization guidance in the README near
the Extension SDK requirement to state that the phase must be seeded before the
first gated step, either by a product default or an ungated initializer, because
PhaseGatedStep.pre() runs before process().
- Around line 14-18: Update the installation guidance in phase-step/README.md
lines 14-18 and phase-step/docs/usage.md lines 6-10 to avoid promising an
unavailable PyPI package before release. Mark the pip command as post-release or
replace it with the supported source/workspace installation path in both guides,
keeping the instructions consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 83e35d24-e321-46b4-86e2-b46725b24eef
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/CODEOWNERSAGENTS.mdREADME.mdmake/common.mkphase-step/.copier-answers.ymlphase-step/AGENTS.mdphase-step/LICENSEphase-step/README.mdphase-step/docs/architecture.mdphase-step/docs/contributing.mdphase-step/docs/releases.mdphase-step/docs/testing.mdphase-step/docs/usage.mdphase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/mpt_extension_contrib/phase_step/py.typedphase-step/mpt_extension_contrib/phase_step/steps.pyphase-step/pyproject.tomlphase-step/tests/conftest.pyphase-step/tests/test_steps.pypyproject.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- make/common.mk
- README.md
- .github/CODEOWNERS
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Identify the affected package or repository-level concern before making changes.
Read only relevant local files before making changes, and consult the matching shared standard when shared engineering conventions are involved.
Repository-local documents add to, restrict, or override shared guidance; local rules take precedence when they conflict.
Use the naming mapping: repository directories are kebab-case, distributions usempt-extension-contrib-<module>, and Python imports usempt_extension_contrib.<module_with_underscores>.
Do not invent release, compatibility, or testing guarantees that are not represented in repository code or documentation.
Files:
phase-step/LICENSEphase-step/AGENTS.mdphase-step/docs/contributing.mdphase-step/docs/releases.mdphase-step/docs/usage.mdAGENTS.mdphase-step/docs/testing.mdpyproject.tomlphase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/pyproject.tomlphase-step/tests/conftest.pyphase-step/docs/architecture.mdphase-step/mpt_extension_contrib/phase_step/steps.pyphase-step/README.mdphase-step/tests/test_steps.py
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
phase-step/LICENSEphase-step/AGENTS.mdphase-step/docs/contributing.mdphase-step/docs/releases.mdphase-step/docs/usage.mdAGENTS.mdphase-step/docs/testing.mdpyproject.tomlphase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/pyproject.tomlphase-step/tests/conftest.pyphase-step/docs/architecture.mdphase-step/mpt_extension_contrib/phase_step/steps.pyphase-step/README.mdphase-step/tests/test_steps.py
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the package or repository-level concern affected by the task.
- Read only the relevant local files before making changes.
- Read the matching shared standard or operational guidance from
softwareone-platform/mpt-extension-skills
when the task involves shared engineering conventions (resolved locally from
${MPT_EXTENSION_SKILLS_HOME:-$HOME/.mpt-extension-skills}/current).- Treat repository-local documents as additions, restrictions, or overrides to
shared guidance.- If a repository-local rule conflicts with a shared or global rule, the
repository-local rule takes precedence.Repository model
mpt-extension-contribis a uv workspace monorepo
of independently released Python libraries for SoftwareONE MPT extensions. It does
not publish an umbrellampt-extension-contribdistribution.
- The root
pyproject.tomldefines the uv workspace and the shared
tool configuration (ruff, flake8, mypy, pytest, coverage). It is not an
installable distribution.- A single
uv.lockat the repository root locks the whole workspace.- Each root-level directory is one distribution. All distributions share the
PEP 420 namespace packagempt_extension_contrib.Mechanical naming rule for a module
<module>:Repository directory: <module> (kebab-case) Distribution name: mpt-extension-contrib-<module> Python import: mpt_extension_contrib.<module_with_underscores>Packages
shared/: internal reusable code exposed asmpt_extension_contrib.shared
(distributionmpt-extension-contrib-shared).order-status/: public package exposed asmpt_extension_contrib.order_status(distributionmpt-extension-contrib-order-status).- [
custom-notifications/](cus...
Files:
phase-step/LICENSEphase-step/AGENTS.mdphase-step/docs/contributing.mdphase-step/docs/releases.mdphase-step/docs/usage.mdAGENTS.mdphase-step/docs/testing.mdpyproject.tomlphase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/pyproject.tomlphase-step/tests/conftest.pyphase-step/docs/architecture.mdphase-step/mpt_extension_contrib/phase_step/steps.pyphase-step/README.mdphase-step/tests/test_steps.py
AGENTS.md
📄 CodeRabbit inference engine (Custom checks)
Update
AGENTS.mdwhen a change introduces or modifies top-level structure or agent navigation.
Files:
AGENTS.md
**/pyproject.toml
📄 CodeRabbit inference engine (AGENTS.md)
**/pyproject.toml: Treat the repository as a uv workspace monorepo of independently released Python libraries; the rootpyproject.tomldefines shared workspace tooling and is not an installable distribution.
Keep each root-level package independently releasable; put shared development tools in the rootpyproject.tomland package-specific runtime or development dependencies in the matching packagepyproject.toml.
Files:
pyproject.tomlphase-step/pyproject.toml
phase-step/**/mpt_extension_contrib/phase_step/**/*.py
📄 CodeRabbit inference engine (phase-step/AGENTS.md)
phase-step/**/mpt_extension_contrib/phase_step/**/*.py: Keep the public API undermpt_extension_contrib.phase_step.
Keep the library free of product-specific phase values and parameter IDs; obtain them from the extension viaPhaseStepSettings.
Files:
phase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/mpt_extension_contrib/phase_step/steps.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-13T18:57:42.493Z
Learning: Run `make check-all pkg=phase-step` from the repository root while iterating.
📚 Learning: 2026-07-03T13:40:50.305Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-extension-python-contrib PR: 50
File: order-status/mpt_extension_contrib/order_status/templates.py:29-56
Timestamp: 2026-07-03T13:40:50.305Z
Learning: When using `TemplateService.get_template(product_id, status, name=template_name)` from the `mpt-extension-sdk` dependency, assume it already implements “named template or default fallback” in a single call (fetches the named template for that status, and if it doesn’t exist, returns the default). Therefore, callers should not issue an extra, separate query for the default template when the named one is missing—just call `get_template(..., name=...)` and use the returned template. This is consistent with the SDK source/tests (e.g., `tests/services/mpt_api_service/test_template.py`) and the method docstring stating it falls back to default.
Applied to files:
phase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/tests/conftest.pyphase-step/mpt_extension_contrib/phase_step/steps.pyphase-step/tests/test_steps.py
📚 Learning: 2026-06-30T11:25:32.303Z
Learnt from: d3rky
Repo: softwareone-platform/mpt-extension-python-contrib PR: 45
File: custom-notifications/tests/conftest.py:35-40
Timestamp: 2026-06-30T11:25:32.303Z
Learning: In softwareone-platform/mpt-extension-python-contrib, Ruff rule S106 is already ignored for all `tests/**` via `[tool.ruff.lint.per-file-ignores]` in the root `pyproject.toml`. When editing test code (e.g., `custom-notifications/tests/conftest.py`), avoid adding inline `# noqa: S106` suppressions if the file is covered by that central `tests/**` ignore—Ruff will already skip S106 for these paths per repository configuration.
Applied to files:
phase-step/tests/conftest.pyphase-step/tests/test_steps.py
🪛 LanguageTool
phase-step/AGENTS.md
[style] ~10-~10: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re.md) for the public API boundary. 4. docs/contributing.md ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~11-~11: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ng.md) before changing this module. 5. docs/testing.md before cha...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../testing.md) before changing tests. 6. docs/releases.md before r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔀 Multi-repo context softwareone-platform/mpt-extension-skills
Linked repositories findings
softwareone-platform/mpt-extension-skills
- Repository guidance recommends using
SkipStepErrorinpre()for step gating and keepingprocess()focused on work, matching the new module’s documented design. [::softwareone-platform/mpt-extension-skills::] - No consumers or references to
PhaseGatedStep,require_phase,advance_phase, orPhaseStepSettingswere found. [::softwareone-platform/mpt-extension-skills::]
🔇 Additional comments (19)
AGENTS.md (1)
42-43: LGTM!phase-step/.copier-answers.yml (1)
1-3: LGTM!phase-step/pyproject.toml (1)
1-27: LGTM!pyproject.toml (1)
5-5: LGTM!Also applies to: 142-142, 218-218
phase-step/AGENTS.md (1)
3-21: LGTM!phase-step/LICENSE (1)
1-201: LGTM!phase-step/README.md (1)
1-13: LGTM!Also applies to: 21-103
phase-step/docs/architecture.md (1)
3-87: LGTM!phase-step/docs/contributing.md (1)
3-21: LGTM!phase-step/docs/testing.md (1)
3-26: LGTM!phase-step/docs/usage.md (1)
1-5: LGTM!Also applies to: 11-16, 21-49, 50-101, 108-148, 154-158
phase-step/mpt_extension_contrib/phase_step/__init__.py (1)
1-13: LGTM!phase-step/mpt_extension_contrib/phase_step/steps.py (5)
1-1: 🎯 Functional CorrectnessConfirm
typing.overrideis supported by the package's target Python version.
typing.overriderequires Python 3.12+. This file isn't paired withphase-step/pyproject.tomlin this review batch, so the declaredrequires-pythonfor the package can't be confirmed here.
27-64: LGTM!
67-98: LGTM!
119-121: 🩺 Stability & Availability | ⚡ Quick winVerify
BaseStep.__init__doesn't need to run.
PhaseGatedStep.__init__never callssuper().__init__(). Ifmpt_extension_sdk.pipeline.BaseStep.__init__sets any state (name, logger, etc.), everyPhaseGatedStepsubclass silently misses it, and the tests here wouldn't catch it since they never assert on base-class state.🛡️ Defensive fix
def __init__(self, expected_phases: ExpectedPhases) -> None: + super().__init__() self._expected_phases = _normalize_phases(expected_phases)Please search for documentation on the
mpt-extension-sdkBaseStep.__init__signature/behavior to confirm whether skipping it is safe.
122-131: LGTM!phase-step/tests/conftest.py (1)
1-31: LGTM!phase-step/tests/test_steps.py (1)
1-205: LGTM!
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
from
8f795de to
a56560c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phase-step/.copier-answers.yml`:
- Line 2: Update the _src_path entry in .copier-answers.yml to reference the
repository-accessible module template location instead of the machine-specific
/workspace path, ensuring copier update works from any checkout location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cd9e70f4-82fa-4cbe-b039-63f424b8eb62
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/CODEOWNERSAGENTS.mdREADME.mdmake/common.mkphase-step/.copier-answers.ymlphase-step/AGENTS.mdphase-step/LICENSEphase-step/README.mdphase-step/docs/architecture.mdphase-step/docs/contributing.mdphase-step/docs/releases.mdphase-step/docs/testing.mdphase-step/docs/usage.mdphase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/mpt_extension_contrib/phase_step/py.typedphase-step/mpt_extension_contrib/phase_step/steps.pyphase-step/pyproject.tomlphase-step/tests/conftest.pyphase-step/tests/test_steps.pypyproject.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
🚧 Files skipped from review as they are similar to previous changes (15)
- phase-step/LICENSE
- AGENTS.md
- phase-step/docs/contributing.md
- phase-step/mpt_extension_contrib/phase_step/init.py
- .github/CODEOWNERS
- phase-step/README.md
- phase-step/pyproject.toml
- README.md
- pyproject.toml
- phase-step/docs/usage.md
- phase-step/docs/testing.md
- make/common.mk
- phase-step/tests/conftest.py
- phase-step/mpt_extension_contrib/phase_step/steps.py
- phase-step/docs/architecture.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
phase-step/docs/releases.mdphase-step/tests/test_steps.pyphase-step/AGENTS.md
phase-step/tests/**/*.py
📄 CodeRabbit inference engine (phase-step/AGENTS.md)
Add tests under
tests/for every behavior change.
Files:
phase-step/tests/test_steps.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-13T19:12:03.917Z
Learning: Treat repository-local documents as additions, restrictions, or overrides to shared guidance; repository-local rules take precedence when they conflict with shared or global rules.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-13T19:12:03.917Z
Learning: For each task, identify the affected package or repository-level concern, read only relevant local files, and consult matching shared standards when shared engineering conventions are involved.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-13T19:12:03.917Z
Learning: Read the repository documentation in the prescribed order before making relevant changes: README, architecture, contributing, local development, testing, releases, and documentation as applicable.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-13T19:12:03.917Z
Learning: Do not invent release, compatibility, or testing guarantees that are not represented in repository code or documentation.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-13T19:12:08.739Z
Learning: Before making changes, read the module documentation in order: README.md, docs/usage.md, docs/architecture.md, docs/contributing.md, and repository-wide ../AGENTS.md; read docs/testing.md before changing tests and docs/releases.md before releasing.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-13T19:12:08.739Z
Learning: Run `make check-all pkg=phase-step` from the repository root while iterating.
📚 Learning: 2026-06-30T11:25:32.303Z
Learnt from: d3rky
Repo: softwareone-platform/mpt-extension-python-contrib PR: 45
File: custom-notifications/tests/conftest.py:35-40
Timestamp: 2026-06-30T11:25:32.303Z
Learning: In softwareone-platform/mpt-extension-python-contrib, Ruff rule S106 is already ignored for all `tests/**` via `[tool.ruff.lint.per-file-ignores]` in the root `pyproject.toml`. When editing test code (e.g., `custom-notifications/tests/conftest.py`), avoid adding inline `# noqa: S106` suppressions if the file is covered by that central `tests/**` ignore—Ruff will already skip S106 for these paths per repository configuration.
Applied to files:
phase-step/tests/test_steps.py
📚 Learning: 2026-07-03T13:40:50.305Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-extension-python-contrib PR: 50
File: order-status/mpt_extension_contrib/order_status/templates.py:29-56
Timestamp: 2026-07-03T13:40:50.305Z
Learning: When using `TemplateService.get_template(product_id, status, name=template_name)` from the `mpt-extension-sdk` dependency, assume it already implements “named template or default fallback” in a single call (fetches the named template for that status, and if it doesn’t exist, returns the default). Therefore, callers should not issue an extra, separate query for the default template when the named one is missing—just call `get_template(..., name=...)` and use the returned template. This is consistent with the SDK source/tests (e.g., `tests/services/mpt_api_service/test_template.py`) and the method docstring stating it falls back to default.
Applied to files:
phase-step/tests/test_steps.py
🪛 LanguageTool
phase-step/AGENTS.md
[style] ~10-~10: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re.md) for the public API boundary. 4. docs/contributing.md ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~11-~11: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ng.md) before changing this module. 5. docs/testing.md before cha...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../testing.md) before changing tests. 6. docs/releases.md before r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔀 Multi-repo context softwareone-platform/mpt-extension-skills
Linked repositories findings
softwareone-platform/mpt-extension-skills
- No references to
PhaseGatedStep,require_phase,advance_phase,PhaseStepSettings, orphase-stepwere found. [::softwareone-platform/mpt-extension-skills::] - Existing guidance uses
SkipStepErrorinpre()for gating, consistent with the new module’s documented design. [::softwareone-platform/mpt-extension-skills::]
🔇 Additional comments (3)
phase-step/AGENTS.md (1)
1-22: LGTM!phase-step/docs/releases.md (1)
3-8: Previous release-criteria concern remains unresolved.Line 6 still excludes releases for implementation fixes and behavior changes.
phase-step/tests/test_steps.py (1)
1-204: LGTM!
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
from
a56560c to
d5a398e
Compare
| def test_require_phase_passes_on_match(order_context_factory, phase_parameter_factory): | ||
| context = order_context_factory(fulfillment=[phase_parameter_factory(EXPECTED_PHASE)]) | ||
|
|
||
| result = require_phase(context, EXPECTED_PHASE) | ||
|
|
||
| assert result is None | ||
|
|
||
|
|
||
| def test_require_phase_passes_on_one_of_many(order_context_factory, phase_parameter_factory): | ||
| context = order_context_factory(fulfillment=[phase_parameter_factory(SECOND_PHASE)]) | ||
|
|
||
| result = require_phase(context, [EXPECTED_PHASE, SECOND_PHASE]) | ||
|
|
||
| assert result is None |
There was a problem hiding this comment.
We can simplify this test by using pytest.mark.parametrize
There was a problem hiding this comment.
Done in 315b2a3 — the two match cases are now one parametrized test, test_require_phase_passes_when_matching (single phase + list). Matches the repo standard (unittests.md: use parametrize for permutations of the same behavior).
🤖 Generated by AI
| def test_require_phase_skips_on_mismatch(order_context_factory, phase_parameter_factory): | ||
| context = order_context_factory(fulfillment=[phase_parameter_factory(OTHER_PHASE)]) | ||
|
|
||
| with pytest.raises(SkipStepError, match=r"checkOnboardStatus.*createSubscription"): | ||
| require_phase(context, EXPECTED_PHASE) | ||
|
|
||
|
|
||
| def test_require_phase_skips_when_none_of_many(order_context_factory, phase_parameter_factory): | ||
| context = order_context_factory(fulfillment=[phase_parameter_factory(OTHER_PHASE)]) | ||
|
|
||
| with pytest.raises(SkipStepError, match=r"one of"): | ||
| require_phase(context, [EXPECTED_PHASE, SECOND_PHASE]) |
There was a problem hiding this comment.
We can simplify this test by using pytest.mark.parametrize
There was a problem hiding this comment.
Done in 315b2a3 — the skip cases are consolidated into test_require_phase_skips_when_not_matching, parametrized over mismatch / none-of-a-list / unset (with the expected skip-message pattern per case).
🤖 Generated by AI
| def test_require_phase_skips_when_unset(order_context_factory): | ||
| context = order_context_factory() | ||
|
|
||
| with pytest.raises(SkipStepError): | ||
| require_phase(context, EXPECTED_PHASE) | ||
|
|
||
|
|
||
| def test_gated_step_rejects_empty_phase(): | ||
| with pytest.raises(ValidationError): | ||
| SampleGatedStep("") | ||
|
|
||
|
|
||
| def test_gated_step_rejects_empty_list(): | ||
| with pytest.raises(ValidationError): | ||
| SampleGatedStep([]) | ||
|
|
||
|
|
||
| def test_gated_step_rejects_empty_phase_in_list(): | ||
| with pytest.raises(ValidationError): | ||
| SampleGatedStep(["createSubscription", ""]) |
There was a problem hiding this comment.
We can simplify this test by using pytest.mark.parametrize
There was a problem hiding this comment.
Done in 315b2a3 — the three rejection cases are now one parametrized test_gated_step_rejects_invalid_phase (empty string, empty list, empty entry in a list).
🤖 Generated by AI
| def test_gated_step_exposes_single_phase(): | ||
| step = SampleGatedStep(EXPECTED_PHASE) | ||
|
|
||
| result = step.expected_phases | ||
|
|
||
| assert result == [EXPECTED_PHASE] | ||
|
|
||
|
|
||
| def test_gated_step_exposes_phase_list(): | ||
| step = SampleGatedStep([EXPECTED_PHASE, SECOND_PHASE]) | ||
|
|
||
| result = step.expected_phases | ||
|
|
||
| assert result == [EXPECTED_PHASE, SECOND_PHASE] |
There was a problem hiding this comment.
We can simplify this test by using pytest.mark.parametrize
There was a problem hiding this comment.
Done in 315b2a3 — merged into a parametrized test_gated_step_exposes_phases (single value normalized to a list, and a list preserved). I also parametrized the two analogous process/skip gated-step pairs below for consistency.
🤖 Generated by AI
| body. | ||
|
|
||
| It replaces the ad-hoc `if <parameter> …: skip` branches scattered across the | ||
| Adobe and AWS steps with one phase check reused across steps. |
There was a problem hiding this comment.
I'd like to remove the mention to Adobe and AWS since this is a shared library and it should describe the generic problem. For example
"It replaces ad-hoc phase checks inside individual extension steps with one reusable gating mechanism shared across pipelines"
There was a problem hiding this comment.
Done in 315b2a3 — used your wording: "It replaces ad-hoc phase checks inside individual extension steps with one reusable gating mechanism shared across pipelines." No more Adobe/AWS mention.
🤖 Generated by AI
| The phase parameter must be seeded before the first gated step: `PhaseGatedStep.pre()` | ||
| runs before `process()`, so a first step with an unset phase would skip before it | ||
| could initialize it. Set an initial value via a product default or an ungated | ||
| initializer step (see [Usage](docs/usage.md)). |
There was a problem hiding this comment.
Ummm...this is not part of the installation section, right?
There was a problem hiding this comment.
Good catch — it isn't. Moved it out of the Install section in 315b2a3 into the Usage section, right after the settings example where phase configuration is discussed.
🤖 Generated by AI
Add the mpt-extension-contrib-phase-step module so a pipeline step can declare the provisioning phase it runs for and be skipped otherwise, replacing the ad-hoc phase if-branches duplicated across extension steps. The library reuses the SDK skip mechanism instead of extending it: gating lives in a step pre() hook that raises SkipStepError, which BasePipeline already logs and continues past. It ships require_phase (the guard), PhaseGatedStep (a thin BaseStep base binding one or more expected phases per instance), and advance_phase (persist the new phase + refresh the context). Phase values and the parameter external id stay out of the library: the id comes from the PhaseStepSettings protocol on the extension settings. "Phase" is used throughout instead of "status" to avoid confusion with the MPT order status (context.order.status); it matches the AWS source the code is extracted from (PhasesEnum / get_phase / set_phase). Include unit tests (100% coverage), self-discoverable docs, a CODEOWNERS entry, and the workspace wiring for the new module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jentyk
force-pushed
the
feature/MPT-22108/extraction-status-step-shared-library
branch
from
d5a398e to
315b2a3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
phase-step/tests/test_steps.py (1)
154-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the three
advance_phasetests.
test_advance_phase_persists,test_advance_phase_writes_new_phase, andtest_advance_phase_refreshes_orderall repeat the identicalawait advance_phase(context, "completed")call and only differ in which single assertion they check. Merging them into one test with three assertions (or parametrizing the assertion) would reduce duplicated setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phase-step/tests/test_steps.py` around lines 154 - 180, Consolidate test_advance_phase_persists, test_advance_phase_writes_new_phase, and test_advance_phase_refreshes_order into a single advance_phase test that performs the shared setup and call once, then verifies the update invocation, expected parameters, and order refresh. Remove the duplicated test functions while preserving all existing assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@phase-step/tests/test_steps.py`:
- Around line 154-180: Consolidate test_advance_phase_persists,
test_advance_phase_writes_new_phase, and test_advance_phase_refreshes_order into
a single advance_phase test that performs the shared setup and call once, then
verifies the update invocation, expected parameters, and order refresh. Remove
the duplicated test functions while preserving all existing assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dd58b24e-0bc9-485b-a093-a99be7cf2ca3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/CODEOWNERSAGENTS.mdREADME.mdmake/common.mkphase-step/.copier-answers.ymlphase-step/AGENTS.mdphase-step/LICENSEphase-step/README.mdphase-step/docs/architecture.mdphase-step/docs/contributing.mdphase-step/docs/releases.mdphase-step/docs/testing.mdphase-step/docs/usage.mdphase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/mpt_extension_contrib/phase_step/py.typedphase-step/mpt_extension_contrib/phase_step/steps.pyphase-step/pyproject.tomlphase-step/tests/conftest.pyphase-step/tests/test_steps.pypyproject.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
🚧 Files skipped from review as they are similar to previous changes (15)
- phase-step/.copier-answers.yml
- phase-step/mpt_extension_contrib/phase_step/init.py
- phase-step/docs/usage.md
- README.md
- pyproject.toml
- AGENTS.md
- make/common.mk
- phase-step/tests/conftest.py
- phase-step/README.md
- phase-step/docs/releases.md
- phase-step/docs/contributing.md
- phase-step/docs/architecture.md
- .github/CODEOWNERS
- phase-step/docs/testing.md
- phase-step/mpt_extension_contrib/phase_step/steps.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: For each task, identify the affected package or repository-level concern and read only relevant local files before making changes.
When shared engineering conventions are involved, read the matching guidance from${MPT_EXTENSION_SKILLS_HOME:-$HOME/.mpt-extension-skills}/current; repository-local rules add to, restrict, or override shared guidance, and local rules take precedence on conflicts.
Treatmpt-extension-contribas a uv workspace monorepo of independently released Python libraries; the rootpyproject.tomlis workspace/tool configuration, not an installable distribution.
Use the module naming mapping: root directory<module>in kebab-case, distributionmpt-extension-contrib-<module>, and Python importmpt_extension_contrib.<module_with_underscores>.
Keep every root-level package independently releasable.
Do not invent release, compatibility, or testing guarantees that are not represented in repository code or documentation.
Before changing dependencies, adding a module, or preparing a pull request, consultdocs/contributing.md; consultdocs/local-development.mdfor local workflows, dependency commands, and scaffolding; consultdocs/testing.mdbefore changing code or tests; consultdocs/releases.mdbefore tagging or publishing; and consultdocs/documentation.mdwhen changing documentation.
Files:
phase-step/pyproject.tomlphase-step/LICENSEphase-step/tests/test_steps.pyphase-step/AGENTS.md
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
phase-step/pyproject.tomlphase-step/LICENSEphase-step/tests/test_steps.pyphase-step/AGENTS.md
phase-step/tests/**
📄 CodeRabbit inference engine (phase-step/AGENTS.md)
Add tests under
tests/for every behavior change.
Files:
phase-step/tests/test_steps.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-14T08:28:55.709Z
Learning: Read the module documentation in the prescribed order before making changes, and read `docs/testing.md` before changing tests.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-14T08:28:55.709Z
Learning: Run `make check-all pkg=phase-step` from the repository root while iterating.
📚 Learning: 2026-06-30T11:25:32.303Z
Learnt from: d3rky
Repo: softwareone-platform/mpt-extension-python-contrib PR: 45
File: custom-notifications/tests/conftest.py:35-40
Timestamp: 2026-06-30T11:25:32.303Z
Learning: In softwareone-platform/mpt-extension-python-contrib, Ruff rule S106 is already ignored for all `tests/**` via `[tool.ruff.lint.per-file-ignores]` in the root `pyproject.toml`. When editing test code (e.g., `custom-notifications/tests/conftest.py`), avoid adding inline `# noqa: S106` suppressions if the file is covered by that central `tests/**` ignore—Ruff will already skip S106 for these paths per repository configuration.
Applied to files:
phase-step/tests/test_steps.py
📚 Learning: 2026-07-03T13:40:50.305Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-extension-python-contrib PR: 50
File: order-status/mpt_extension_contrib/order_status/templates.py:29-56
Timestamp: 2026-07-03T13:40:50.305Z
Learning: When using `TemplateService.get_template(product_id, status, name=template_name)` from the `mpt-extension-sdk` dependency, assume it already implements “named template or default fallback” in a single call (fetches the named template for that status, and if it doesn’t exist, returns the default). Therefore, callers should not issue an extra, separate query for the default template when the named one is missing—just call `get_template(..., name=...)` and use the returned template. This is consistent with the SDK source/tests (e.g., `tests/services/mpt_api_service/test_template.py`) and the method docstring stating it falls back to default.
Applied to files:
phase-step/tests/test_steps.py
🪛 LanguageTool
phase-step/AGENTS.md
[style] ~10-~10: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re.md) for the public API boundary. 4. docs/contributing.md ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~11-~11: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ng.md) before changing this module. 5. docs/testing.md before cha...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../testing.md) before changing tests. 6. docs/releases.md before r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔀 Multi-repo context softwareone-platform/mpt-extension-skills
Linked repositories findings
softwareone-platform/mpt-extension-skills
- No consumers or references to
PhaseGatedStep,require_phase,advance_phase, ormpt-extension-contrib-phase-stepwere found. standards/sdk-pipeline-steps.mddocuments immutableParameterBagupdates,ctx.refresh_order(), andSkipStepErrorfrompre(), aligning with the new module’s documented behavior.standards/packages-and-dependencies.mdrecommends bounded SDK dependencies such asmpt-extension-sdk>=6.3,<7, matching the package constraint. [::softwareone-platform/mpt-extension-skills::]
🔇 Additional comments (7)
phase-step/AGENTS.md (1)
1-22: LGTM!phase-step/tests/test_steps.py (4)
95-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify matching/skipping test pairs with
pytest.mark.parametrize.Both
test_gated_step_processes_when_matching/test_gated_step_skips_when_not_matchingandtest_subclass_pre_runs_after_check/test_subclass_pre_skipped_on_mismatchare structurally identical pairs differing only in the stored phase and the pass/skip assertion. This is the same simplification a previous reviewer already requested for this file.Also applies to: 129-151
1-37: LGTM!
39-92: LGTM!
163-171: 🗄️ Data Integrity & IntegrationThe
phaseparameter id matches the fixture default here, so this assertion stays aligned.> Likely an incorrect or invalid review comment.phase-step/pyproject.toml (1)
1-27: LGTM!phase-step/LICENSE (1)
1-201: LGTM!
|
@jentyk I propose to have two implementations here what I don't like here is the amount of you code you need to write even for simple transitions and also you cannot wrap some libraries steps to be gated. So for me we can have 2 options here
for example Looks like these approach is going to cover syntax sugar in simple cases and a custom logic in more complex scenarios |
Add `phase_gate_step(cls, expected_phases, next_phase=None)`, which returns a runtime subclass of an existing `BaseStep` class that runs the phase check in `pre()` before the wrapped step's own `pre()`. It covers the case where a step class cannot be changed to subclass `PhaseGatedStep` directly; prefer `PhaseGatedStep` when you own the step. When `next_phase` is given, the wrapped step advances the order to that phase (via `advance_phase`) after its `process()` completes successfully — the simple linear transition. For a next phase that depends on the step's own logic, subclass `PhaseGatedStep` and advance explicitly. It lives in its own module (`phase_gate.py`) and carries a scoped `# type: ignore[misc, valid-type]` on the subclass line because the base class is only known at runtime (mypy cannot type a dynamic base), plus a `# noqa: WPS431` for the in-function subclass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@d3rky — done: I built option 2 and it's on the PR now (commit But I want to be honest: I think we should not merge this helper. Here is why, in simple terms. 1. It saves almost no code. To gate a step you don't own, you only need a tiny subclass: class GatedSetDueDate(SetDueDate):
async def pre(self, ctx):
require_phase(ctx, "checkOnboard")
await super().pre(ctx)That is a few lines and anyone can read it. The helper removes maybe two lines, and gives back a lot of hidden behaviour in return. We are splitting hairs over a few lines. 2. Gated steps should be normal classes, provided where the step lives. Our shared libraries can ship a plain step and, when it is useful, a gated one too. The gated one is just a 3. It hides logic and is hard to read. The helper builds a class at run time and swaps a method in. It even needs special comments to turn the type checker and the linter off ( 4. Moving to the next phase belongs inside the step. That is where the work is, and it keeps related things in one place, like everything else. The helper pulls that choice out into a wrapper argument, so the logic ends up split across two places. And in the real cases — where the next phase depends on what the step did — the helper cannot decide it anyway, so you go back to doing it in the step. Two ways to do one thing is more confusing, not less. 5. It teaches a bad habit. A one-line wrapper invites people to gate steps from the outside and spread phase logic around the pipeline, instead of keeping each step complete on its own. A shared library should push people toward the clear way, not the clever one. The base class already covers both of your cases in one consistent way:
So my vote: keep the base class only, and drop the wrapper. It's on the PR now so you can see it working, but I would not merge it. Happy to talk it through if you still want it. 🤖 Generated by AI |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
phase-step/mpt_extension_contrib/phase_step/phase_gate.py (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider also copying
__module__for full introspection fidelity.
__name__/__qualname__are copied fromcls, but__module__stays asphase_gate.py, which can make tracebacks/repr()point to the wrapper module instead of the wrapped step's original module.♻️ Optional fix
_Gated.__name__ = cls.__name__ _Gated.__qualname__ = cls.__qualname__ + _Gated.__module__ = cls.__module__ return _Gated🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phase-step/mpt_extension_contrib/phase_step/phase_gate.py` around lines 56 - 58, Update the _Gated wrapper metadata assignment to also copy __module__ from cls, alongside __name__ and __qualname__, so introspection and representations retain the wrapped step’s original module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phase-step/mpt_extension_contrib/phase_step/phase_gate.py`:
- Line 44: Remove the invalid WPS431 code from the noqa suppression on the
_Gated class, leaving only recognized Ruff rule codes or removing the
suppression if none remain.
---
Nitpick comments:
In `@phase-step/mpt_extension_contrib/phase_step/phase_gate.py`:
- Around line 56-58: Update the _Gated wrapper metadata assignment to also copy
__module__ from cls, alongside __name__ and __qualname__, so introspection and
representations retain the wrapped step’s original module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3b339107-bc0f-4d01-a82e-2f67912a0aec
📒 Files selected for processing (3)
phase-step/mpt_extension_contrib/phase_step/__init__.pyphase-step/mpt_extension_contrib/phase_step/phase_gate.pyphase-step/tests/test_phase_gate.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- phase-step/mpt_extension_contrib/phase_step/init.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Do not invent release, compatibility, or testing guarantees that are not represented in repository code or documentation.
Files:
phase-step/mpt_extension_contrib/phase_step/phase_gate.pyphase-step/tests/test_phase_gate.py
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
phase-step/mpt_extension_contrib/phase_step/phase_gate.pyphase-step/tests/test_phase_gate.py
**/*.{py,pyproject.toml}
📄 CodeRabbit inference engine (AGENTS.md)
Use the repository's shared Python coding, packaging, and tooling standards, including the configured ruff, flake8, mypy, pytest, and coverage settings.
Files:
phase-step/mpt_extension_contrib/phase_step/phase_gate.pyphase-step/tests/test_phase_gate.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the package naming model: root directories use kebab-case, distributions are named mpt-extension-contrib-, and Python imports use mpt_extension_contrib.<module_with_underscores>.
Files:
phase-step/mpt_extension_contrib/phase_step/phase_gate.pyphase-step/tests/test_phase_gate.py
phase-step/**/mpt_extension_contrib/phase_step/**/*.py
📄 CodeRabbit inference engine (phase-step/AGENTS.md)
phase-step/**/mpt_extension_contrib/phase_step/**/*.py: Keep the public API undermpt_extension_contrib.phase_step.
Keep the library free of product-specific phase values and parameter IDs; obtain them from the extension viaPhaseStepSettings.
Files:
phase-step/mpt_extension_contrib/phase_step/phase_gate.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-15T17:21:44.737Z
Learning: Add tests under `tests/` for every behavior change.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-15T17:21:44.737Z
Learning: Run `make check-all pkg=phase-step` from the repository root while iterating.
Learnt from: CR
Repo: softwareone-platform/mpt-extension-python-contrib
Timestamp: 2026-07-15T17:21:44.737Z
Learning: Read the module README, usage, architecture, contributing, testing, and release documentation—and the repository-wide `AGENTS.md`—in the specified order before making changes or releases.
📚 Learning: 2026-07-03T13:40:50.305Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-extension-python-contrib PR: 50
File: order-status/mpt_extension_contrib/order_status/templates.py:29-56
Timestamp: 2026-07-03T13:40:50.305Z
Learning: When using `TemplateService.get_template(product_id, status, name=template_name)` from the `mpt-extension-sdk` dependency, assume it already implements “named template or default fallback” in a single call (fetches the named template for that status, and if it doesn’t exist, returns the default). Therefore, callers should not issue an extra, separate query for the default template when the named one is missing—just call `get_template(..., name=...)` and use the returned template. This is consistent with the SDK source/tests (e.g., `tests/services/mpt_api_service/test_template.py`) and the method docstring stating it falls back to default.
Applied to files:
phase-step/mpt_extension_contrib/phase_step/phase_gate.pyphase-step/tests/test_phase_gate.py
📚 Learning: 2026-06-30T11:25:32.303Z
Learnt from: d3rky
Repo: softwareone-platform/mpt-extension-python-contrib PR: 45
File: custom-notifications/tests/conftest.py:35-40
Timestamp: 2026-06-30T11:25:32.303Z
Learning: In softwareone-platform/mpt-extension-python-contrib, Ruff rule S106 is already ignored for all `tests/**` via `[tool.ruff.lint.per-file-ignores]` in the root `pyproject.toml`. When editing test code (e.g., `custom-notifications/tests/conftest.py`), avoid adding inline `# noqa: S106` suppressions if the file is covered by that central `tests/**` ignore—Ruff will already skip S106 for these paths per repository configuration.
Applied to files:
phase-step/tests/test_phase_gate.py
🪛 Ruff (0.15.21)
phase-step/mpt_extension_contrib/phase_step/phase_gate.py
[warning] 44-44: Invalid rule code in # noqa: WPS431
Add non-Ruff rule codes to the lint.external configuration option
(RUF102)
🔀 Multi-repo context softwareone-platform/mpt-extension-skills
Linked repositories findings
softwareone-platform/mpt-extension-skills
- No direct consumers or references to the new phase-step APIs or package were found. [::softwareone-platform/mpt-extension-skills::]
- Existing standards document immutable
ParameterBagupdates,refresh_order(), andSkipStepErrorusage, aligning with the package’s documented behavior. [::softwareone-platform/mpt-extension-skills::] - Dependency guidance supports bounded SDK constraints such as
mpt-extension-sdk >=6.3, <7. [::softwareone-platform/mpt-extension-skills::]
🔇 Additional comments (4)
phase-step/mpt_extension_contrib/phase_step/phase_gate.py (3)
12-54: 🎯 Functional CorrectnessGating and advancement logic correctly matches the upstream contracts.
pre()gates viarequire_phasebefore delegating (matchesrequire_phase's skip-and-continue contract insteps.py), andprocess()only advances after the wrapped step succeeds, correctly skippingadvance_phasewhen the wrappedprocess()raises.
1-58: 📐 Maintainability & Code QualityConfirm intent to merge this helper given the author's own recommendation against it.
Per the PR objectives, the author recommends not merging
phase_gate_step(limited savings, runtime-generated classes, lint/type-check suppressions, hidden control flow) and retaining onlyPhaseGatedStep. This file (and its test file) is nonetheless present in this review. Please confirm whether this is the final intended state or if it should be dropped before merge.
1-1: 🩺 Stability & AvailabilityNo issue:
phase-stepalready requires Python 3.12+, sofrom typing import overrideis valid in these files.> Likely an incorrect or invalid review comment.phase-step/tests/test_phase_gate.py (1)
12-93: 🎯 Functional CorrectnessThorough, well-targeted coverage of
phase_gate_step's behavior.Name preservation, pass-through vs. skip on phase match/mismatch, and advance-on-success vs. no-advance-on-unset/failure are all exercised and align with the wrapper's actual control flow (gate in
pre(), conditional advance inprocess()only after the wrapped call succeeds).
3 participants
🤖 AI-generated PR — Please review carefully.
What
Adds a new independently released module,
mpt-extension-contrib-phase-step(
mpt_extension_contrib.phase_step), for provisioning-phase step gating: apipeline step declares the phase it runs for and is skipped when the order's
phase parameter holds a different value. This replaces the ad-hoc
if <parameter> …: skipbranches duplicated across the AWS/Adobe steps(source: AWS
swo_aws_extension/flows/steps/base.py).The concept is named phase (not "status") to avoid confusion with the MPT
order status on
context.order.status; it also matches the AWS source the codeis extracted from (
PhasesEnum/get_phase/set_phase).Approach
The control-flow half already lives in the SDK —
BaseStep.run()awaitspre()first, and
BasePipelinecatchesSkipStepError, logs the skip, and continues.So the library adds only the missing convention, not a new base pipeline or
error type:
require_phase(context, expected_phases)— the guard: reads the phaseparameter and raises
SkipStepErrorunless the current phase is one of theexpected ones. Use it directly in a hand-written
pre().PhaseGatedStep(expected_phases)— a thinBaseStepbase that binds one ormore expected phases per instance (at pipeline-composition time) and
pre-wires the check; subclass and implement
process().advance_phase(context, phase)— persists a new phase throughorders.updateand refreshes
context.order, so a later gated step in the same run reads thenew phase and gates correctly.
PhaseStepSettings—Protocolexposingphase_parameter: str; the parameterexternal id is injected by the extension, not hardcoded.
No decorators/wrappers/mixins: gating is expressed by subclassing and, when a
step needs its own
pre(), an explicitawait super().pre(context). This keepsthe facilitation smaller than the logic it facilitates, avoids MRO ordering
surprises, and preserves each step's own
namefor logging/tracing.The library carries no product-specific phase values or parameter ids and has
no cross-module dependencies.
Reviewer note
The interface choice (subclass + explicit
super().pre()over adecorator/wrapper/mixin) is deliberate — see
docs/architecture.md.
Included
usage, architecture, contributing, testing, releases).
pyproject.toml,make/common.mk,root README/AGENTS package indexes).
Out of scope / follow-up
the package version is still
0.0.0.Testing
uv run ruff format --check/ruff check/flake8/mypy— all pass.uv run pytest— full workspace suite passes, 100% coverage (incl. namespacecoexistence).
scripts/check_repository.pyanduv lock --check— pass.Closes MPT-22108 (excluding the PyPI publish follow-up).
mpt-extension-contrib-phase-step(mpt_extension_contrib.phase_step) implementing reusable provisioning-phase step gating for Extension SDK pipelines.require_phase(context, expected_phases),PhaseGatedStep(expected_phases),advance_phase(context, phase), andPhaseStepSettingsto validate/skip step execution based on the order’s configured phase fulfillment parameter.SkipStepErrorflow inpre().advance_phase()throughcontext.mpt_api_service.orders.update(...)and refreshes the order context for subsequent steps.phase-step/workspace packaging + wiring (AGENTS.md,README.md,make/common.mkdefault package list,pyproject.tomltool/test/type-check coverage) and.github/CODEOWNERSownership for the module.pre()ordering, andadvance_phasepersistence + order refresh.