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 |
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
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
| 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
5b4c039 to
5aefa67
Compare
5aefa67 to
0ea9449
Compare
03a85e6 to
720b1a6
Compare
99c6218 to
ed27709
Compare
jentyk
changed the title
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!
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!
a56560c to
d5a398e
Compare
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>
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.