Expose PowerShell help staging output#265
Merged
Merged
Conversation
Add an explicit `ps-module-name` input to `stage-release-artefacts` and export `powershell_help_dir` when matching sidecar files are staged beneath that module directory. Accept `dest` as a TOML alias for `destination` so target-specific PowerShell artefact entries can use the documented compact shape. Keep `windows-package` unchanged apart from a TODO that tracks future MSI embedding work in issue #264.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdd optional ChangesPowerShell Module Sidecar Support
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Pipeline as stage_artefacts
participant Collector as _collect_artefacts
participant Resolver as _resolve_powershell_help_dir
participant OutputPrep as prepare_output_data
Caller->>Pipeline: call stage_artefacts(config, ps_module_name, corr_id)
Pipeline->>Collector: collect and stage artefacts
Collector-->>Pipeline: staged_paths, outputs, checksums, skipped_artefacts
Pipeline->>Resolver: resolve powershell_help_dir(staging_root, ps_module_name)
Resolver-->>Pipeline: powershell_help_dir or None
Pipeline->>OutputPrep: build StagingOutputData
OutputPrep-->>Pipeline: output dict (includes powershell_help_dir)
Pipeline-->>Caller: return StageResult
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
Contributor
Reviewer's GuideExtends the stage-release-artefacts action to accept an explicit PowerShell module name and emit a new powershell_help_dir output, including wiring through the Python staging pipeline, configuration parsing, and tests, while leaving windows-package with just a TODO for future MSI embedding. Sequence diagram for PowerShell help dir staging outputsequenceDiagram
actor Workflow
participant StageReleaseArtefactsAction as stage-release-artefacts action
participant StageScript as stage.py main
participant Pipeline as stage_artefacts
participant Output as prepare_output_data
Workflow->>StageReleaseArtefactsAction: inputs.ps-module-name
StageReleaseArtefactsAction->>StageScript: INPUT_PS_MODULE_NAME env
StageScript->>StageScript: parse ps_module_name
StageScript->>Pipeline: stage_artefacts(config, github_output_file, normalize_windows_paths, ps_module_name)
Pipeline->>Pipeline: _resolve_powershell_help_dir(staging_dir, staged_paths, ps_module_name)
alt ps_module_name set and artefacts under module_dir
Pipeline-->>Pipeline: powershell_help_dir = staging_dir/ps_module_name
else no matching module artefacts
Pipeline-->>Pipeline: powershell_help_dir = None
end
Pipeline->>Output: prepare_output_data(staging_dir, staged_paths, outputs, checksums, powershell_help_dir)
Output-->>Pipeline: output_data with powershell_help_dir
Pipeline-->>StageScript: StageResult(powershell_help_dir)
StageScript-->>StageReleaseArtefactsAction: write GITHUB_OUTPUT powershell_help_dir
StageReleaseArtefactsAction-->>Workflow: outputs.powershell_help_dir
Flow diagram for destination and dest artefact config parsingflowchart TD
A[Start _parse_artefact_entry] --> B[Read entry.destination]
B --> C[Read entry.dest]
C --> D{destination and dest both present?}
D -->|Yes| E[Raise StageError: Artefact must not define both destination and dest]
D -->|No| F[Set destination = entry.destination or entry.dest]
F --> G[Create ArtefactConfig with destination]
G --> H[End _parse_artefact_entry]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
|
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. .github/actions/stage-release-artefacts/tests/test_stage.py Comment on lines +496 to +576 def test_stages_windows_powershell_help_dir(self, tmp_path: Path) -> None:
"""Windows PowerShell sidecars are staged under the module directory."""
workspace = tmp_path / "workspace"
workspace.mkdir()
source_dir = (
workspace
/ "target"
/ "orthohelp"
/ "x86_64-pc-windows-msvc"
/ "release"
/ "powershell"
/ "MyTool"
)
help_dir = source_dir / "en-US"
help_dir.mkdir(parents=True)
files = [
source_dir / "MyTool.psm1",
source_dir / "MyTool.psd1",
help_dir / "MyTool-help.xml",
help_dir / "about_MyTool.help.txt",
]
for file in files:
file.write_text(file.name, encoding="utf-8")
output_file = tmp_path / "output"
config = StagingConfig(
workspace=workspace,
bin_name="mytool",
dist_dir="dist",
checksum_algorithm="sha256",
artefacts=[
ArtefactConfig(
source=(
"target/orthohelp/{target}/release/powershell/"
"MyTool/MyTool.psm1"
),
destination="MyTool/MyTool.psm1",
required=False,
),
ArtefactConfig(
source=(
"target/orthohelp/{target}/release/powershell/"
"MyTool/MyTool.psd1"
),
destination="MyTool/MyTool.psd1",
required=False,
),
ArtefactConfig(
source=(
"target/orthohelp/{target}/release/powershell/"
"MyTool/en-US/MyTool-help.xml"
),
destination="MyTool/en-US/MyTool-help.xml",
required=False,
),
ArtefactConfig(
source=(
"target/orthohelp/{target}/release/powershell/"
"MyTool/en-US/about_MyTool.help.txt"
),
destination="MyTool/en-US/about_MyTool.help.txt",
required=False,
),
],
platform="windows",
arch="x86_64",
target="x86_64-pc-windows-msvc",
)
result = stage_artefacts(config, output_file, ps_module_name="MyTool")
assert len(result.staged_artefacts) == 4
assert (result.staging_dir / "MyTool" / "MyTool.psm1").exists()
assert (result.staging_dir / "MyTool" / "MyTool.psd1").exists()
assert (result.staging_dir / "MyTool" / "en-US" / "MyTool-help.xml").exists()
assert (
result.staging_dir / "MyTool" / "en-US" / "about_MyTool.help.txt"
).exists()
assert result.powershell_help_dir == result.staging_dir / "MyTool"
output = output_file.read_text(encoding="utf-8")
assert f"powershell_help_dir={result.staging_dir / 'MyTool'}" in output❌ New issue: Large Method |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. .github/actions/stage-release-artefacts/tests/test_stage.py Comment on lines +171 to +196 def test_rejects_dest_and_destination_together(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""Artefact entries must not provide both destination spellings."""
monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path))
config_file = tmp_path / "config.toml"
config_file.write_text(
"""
[common]
bin_name = "myapp"
[[common.artefacts]]
source = "binary"
dest = "short"
destination = "long"
[targets.linux]
platform = "linux"
arch = "x86_64"
target = "x86_64-unknown-linux-gnu"
""",
encoding="utf-8",
)
with pytest.raises(StageError, match="both 'destination' and 'dest'"):
load_config(config_file, "linux")❌ New issue: Code Duplication |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. .github/actions/stage-release-artefacts/tests/test_stage.py Comment on file assert config.platform == "linux"
assert len(config.artefacts) == 1
def test_loads_target_artefacts_with_dest_alias(❌ New issue: Low Cohesion |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. .github/actions/stage-release-artefacts/scripts/stage_common/output.py Comment on lines +38 to +39 *,
powershell_help_dir: Path | None = None,❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Move the monolithic `test_stage.py` coverage into focused modules for environment, config, resolution, output, and pipeline behaviour. Extract shared staging config and PowerShell workspace helpers so the pipeline tests keep the same assertions with less duplicated setup.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Excess Number of Function Arguments.github/actions/stage-release-artefacts/scripts/stage_common/output.py: prepare_output_data What lead to degradation?prepare_output_data has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
Owner
Author
|
@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Require the staged PowerShell module directory to exist before reporting `powershell_help_dir`. This prevents Windows path comparisons from treating a staged binary with different casing as a module directory. Assert the serialized output path with POSIX separators to match the action output format.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add `StagingOutputData` to bundle the values needed by `prepare_output_data` and update the pipeline caller and output tests to use the new parameter object.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. .github/actions/stage-release-artefacts/scripts/stage_common/pipeline.py Comment on file github_output_file: Path,
*,
normalize_windows_paths: bool = False,
ps_module_name: str = "",❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
Move the artefact collection loop out of `stage_artefacts` into `_collect_artefacts` so the staging orchestration stays below the large method threshold while preserving the existing staging behaviour.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. .github/actions/stage-release-artefacts/tests/test_pipeline.py Comment on lines +485 to +503 def test_parent_traversal_destinations_are_rejected_through_staging(
self, tmp_path: Path, destination: str
) -> None:
"""Generated parent traversal destinations cannot escape staging."""
workspace = tmp_path / "workspace"
workspace.mkdir(exist_ok=True)
(workspace / "myapp").write_text("binary content", encoding="utf-8")
config = self._make_linux_config(
workspace,
[
ArtefactConfig(
source="myapp",
destination=f"../{destination}",
)
],
)
with pytest.raises(StageError, match="escapes staging directory"):
stage_artefacts(config)❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Replace repeated Windows staging config setup in pipeline tests with a shared helper, and apply the requested PowerShell documentation wording fixes.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
Oops, something went wrong! Please try again later. 🐰 💔 |
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
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 @.github/actions/stage-release-artefacts/tests/test_pipeline.py:
- Around line 227-244: Make _make_windows_config a staticmethod: add the
`@staticmethod` decorator and remove the implicit self parameter, update its body
to compute artefacts before calling StagingConfig (e.g. artefacts_to_use =
artefacts if artefacts is not None else
TestStageArtefacts._powershell_artefact_configs()), and replace any
self._powershell_artefact_configs() calls with the class-qualified call
TestStageArtefacts._powershell_artefact_configs(); then pass
artefacts=artefacts_to_use into the StagingConfig constructor so the helper no
longer depends on instance state.
- Around line 47-55: TRAVERSAL_DESTINATIONS currently only blacklists "\x00"
which is inconsistent with PS_MODULE_NAMES and PATH_SEGMENTS and can generate
Windows-illegal/path-separator chars that cause OS errors; update the Hypothesis
strategy TRAVERSAL_DESTINATIONS to use the same blacklist_characters string as
PS_MODULE_NAMES and PATH_SEGMENTS (i.e. '/\\\x00:*?"<>|') while keeping the
existing blacklist_categories, max_codepoint, min_size/max_size and the existing
filter(lambda value: "{" not in value and "}" not in value) so generated values
avoid POSIX separators and Windows-illegal filename characters and prevent
platform-specific test failures.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd49a12a-75e9-4591-8f5a-fa95135c3416
📒 Files selected for processing (3)
.github/actions/stage-release-artefacts/README.md.github/actions/stage-release-artefacts/tests/test_pipeline.pydocs/adr/0002-explicit-ps-module-name.md
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. .github/actions/stage-release-artefacts/tests/test_pipeline.py Comment on lines +468 to +486 def test_parent_traversal_destinations_are_rejected_through_staging(
self, tmp_path: Path, destination: str
) -> None:
"""Generated parent traversal destinations cannot escape staging."""
workspace = tmp_path / "workspace"
workspace.mkdir(exist_ok=True)
(workspace / "myapp").write_text("binary content", encoding="utf-8")
config = self._make_linux_config(
workspace,
[
ArtefactConfig(
source="myapp",
destination=f"../{destination}",
)
],
)
with pytest.raises(StageError, match="escapes staging directory"):
stage_artefacts(config)❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Require callers to inject the workspace when loading stage-release configuration, removing the fallback read from `GITHUB_WORKSPACE` in the configuration layer. Split artefact path resolution from copy and checksum writes in the staging pipeline, while keeping `_iter_staged_artefacts` as a compatibility wrapper for later removal. Add correlation IDs and per-artefact DEBUG logging so staging runs can be traced across pipeline and CLI failure logs.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Make the Windows staging config helper independent of instance state and align the traversal destination strategy with the platform-safe filename character policy used by the related Hypothesis strategies.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
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 `@docs/developers-guide.md`:
- Line 169: Edit the sentence "Pipeline INFO, WARNING and DEBUG records include
that `corr_id` so" and insert the Oxford (serial) comma before the final item so
it reads "Pipeline INFO, WARNING, and DEBUG records include that `corr_id` so",
ensuring the Markdown file uses en-GB-oxendict styling.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85b388a4-34ae-40d1-8c0f-9165902743bf
📒 Files selected for processing (6)
.github/actions/stage-release-artefacts/scripts/stage.py.github/actions/stage-release-artefacts/scripts/stage_common/config.py.github/actions/stage-release-artefacts/scripts/stage_common/pipeline.py.github/actions/stage-release-artefacts/tests/test_config.py.github/actions/stage-release-artefacts/tests/test_pipeline.pydocs/developers-guide.md
Add the Oxford comma to the stage-release observability log-level list in the developer guide.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Share the minimal workspace setup and StageError assertion between the parametrized and Hypothesis traversal rejection tests.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
Status against current PR head:
No further code work is required for this PR to satisfy the listed checks. If your CI gate still shows stale warnings, re-trigger the checks to pick up the updated files. If anything still flags, say the word and I will generate targeted diffs. |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Excess Number of Function Arguments.github/actions/stage-release-artefacts/scripts/stage_common/pipeline.py: _ensure_source_available, _stage_configured_artefact, _resolve_configured_artefact, _iter_resolved_artefacts, _stage_resolved_artefact What lead to degradation?_ensure_source_available has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
Assert that missing optional artefacts populate `StageResult.skipped_artefacts` and that the CLI warning helper emits GitHub Actions warning annotations to stderr. Document the required `workspace` argument on `load_config` and the CLI's responsibility for injecting `GITHUB_WORKSPACE`.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Group the shared staging configuration, resolved artefact data, and mutable collection state into small dataclasses so the internal pipeline helpers no longer pass repeated argument bundles around. Keep the public staging entry points and existing error/log message text unchanged while preserving the compatibility wrapper for staged artefact iteration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch extends
stage-release-artefactsso callers that stage PowerShell MAML sidecars can receive a dedicatedpowershell_help_diroutput instead of reconstructing the staged module path themselves. It also keeps MSI embedding outwith this change:windows-packageonly receives a TODO that points to the follow-up enhancement issue.Follow-up issue: #264.
Review walkthrough
ps-module-nameinput andpowershell_help_diroutput contract.destalias and required/optional artefact parsing.Validation
uv run pytest .github/actions/stage-release-artefacts/tests/test_stage.py 2>&1 | tee /tmp/stage-release-artefacts-focused-powershell-help-dir.out: passed, 36 passed.make check-fmt 2>&1 | tee /tmp/shared-actions-check-fmt-powershell-help-dir.out: passed.make typecheck 2>&1 | tee /tmp/shared-actions-typecheck-powershell-help-dir.out: passed.make test 2>&1 | tee /tmp/shared-actions-test-powershell-help-dir.out: passed, 697 passed and 93 skipped.make lint 2>&1 | tee /tmp/shared-actions-lint-powershell-help-dir.out: passed.grep -r 'powershell_help_dir' .github/actions/stage-release-artefacts/: confirmed action metadata, README, tests and implementation references.grep -RIn "ps-help-dir\|powershell_help_dir\|powershell-help" .github/actions/windows-package || true: confirmedwindows-packageonly contains the TODO and no new input wiring.Notes
This branch deliberately uses the explicit
ps-module-nameinput rather than heuristic module-name detection. The action only exportspowershell_help_dirwhen at least one artefact is staged underartifact_dir/<module>, so non-Windows callers can leave the input empty and receive an empty output.Summary by Sourcery
Expose a new PowerShell help directory output from the stage-release-artefacts action and support a short dest alias in artefact configs while keeping behaviour safe and backwards compatible.
New Features:
Enhancements:
Documentation:
Tests: