Document staging helpers and restructure tests#193
Document staging helpers and restructure tests#193leynos wants to merge 6 commits intocodex/implement-network-and-command-functionsfrom
Conversation
Ensure the staging pipeline raises a StageError when a user defines a custom output that collides with the built-in artifact exports.
Reviewer's GuideThis PR transforms the staging helpers into a fully self-contained GitHub composite action, enriches documentation and docstrings, modularizes the CLI uploader’s dependency logic, refactors the release workflow to consume the new action and normalization script, and reorganizes and extends the Python test suite with focused modules and shared fixtures (including a regression for reserved output collisions). Entity relationship diagram for the staging configuration schema (ER diagram)erDiagram
COMMON {
string bin_name
string dist_dir
string checksum_algorithm
string staging_dir_template
ArtefactConfig[] artefacts
}
TARGETS {
string platform
string arch
string target
string bin_ext
string staging_dir_template
ArtefactConfig[] artefacts
}
ArtefactConfig {
string source
boolean required
string output
string destination
string[] alternatives
}
COMMON ||--o{ TARGETS : "has targets"
COMMON ||--o{ ArtefactConfig : "has artefacts"
TARGETS ||--o{ ArtefactConfig : "has artefacts"
Class diagram for the new and refactored staging helper typesclassDiagram
class ArtefactConfig {
+str source
+bool required
+str~ output
+str~ destination
+list~ alternatives
}
class StagingConfig {
+Path workspace
+str bin_name
+str dist_dir
+str checksum_algorithm
+list~ artefacts
+str platform
+str arch
+str target
+str bin_ext
+str staging_dir_template
+str~ target_key
+staging_dir() Path
+staging_dir_name str
+as_template_context() dict
}
class StageResult {
+Path staging_dir
+list~ staged_artefacts
+dict outputs
+dict checksums
}
class StageError {
<<exception>>
}
ArtefactConfig <.. StagingConfig : contains
StagingConfig <.. StageResult : used by
StageError <|-- RuntimeError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughIntroduce a new composite GitHub Action to stage release artefacts via a TOML-driven Python CLI. Add a staging helper package with config, globbing, checksums, outputs, and error handling. Replace bespoke Windows/macOS staging scripts with the new action and config. Refactor release asset uploader to optionalise deps. Add tests, docs, and workflow updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Workflow Runner
participant GA as Composite Action (stage)
participant UV as setup-uv
participant CLI as scripts/stage.py
participant PKG as stage_common (config/glob/staging)
participant GHO as $GITHUB_OUTPUT
Dev->>GA: uses: ./.github/actions/stage<br/>with: config-file, target
GA->>UV: Install uv (python 3.11)
UV-->>GA: uv available
GA->>CLI: uv run stage.py --config-file ... --target ...
CLI->>PKG: load_config(config.toml, target)
PKG-->>CLI: StagingConfig
CLI->>PKG: stage_artefacts(config, GITHUB_OUTPUT)
PKG->>PKG: Resolve sources (templates, glob)
PKG->>PKG: Copy to staging dir, compute checksums
PKG->>GHO: Append outputs (maps, paths)
PKG-->>CLI: StageResult
CLI-->>GA: Exit 0
GA-->>Dev: outputs: artifact_dir, dist_dir, artefact_map, checksum_map, ...
sequenceDiagram
autonumber
participant CFG as load_config
participant ENV as require_env_path
participant TOML as release-staging.toml
participant VAL as Validators
CFG->>ENV: Require GITHUB_WORKSPACE
ENV-->>CFG: Path(workspace)
CFG->>TOML: Read and parse
CFG->>VAL: Merge common + target, validate keys
VAL-->>CFG: StagingConfig (bin_name, platform, arch, artefacts, checksum)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@sourcery-ai review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- centralise runtime option parsing so Cyclopts and argparse share validation - replace loader stubs with named tuples, add run_cli helper, and raise with causes - expose reserved staging outputs, align action coverage, and update docs/tests
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/upload_release_assets.py (1)
103-107: Add a single‑line docstring for the private iterator.Document intent per guidelines.
-def _iter_candidate_paths(dist_dir: Path, bin_name: str) -> typ.Iterator[Path]: +def _iter_candidate_paths(dist_dir: Path, bin_name: str) -> typ.Iterator[Path]: + """Yield candidate artefact file paths under dist_dir for the given binary."""As per coding guidelines
♻️ Duplicate comments (2)
.github/actions/stage/scripts/stage_common/config.py (2)
167-181: Checksum validation now robust and portableValidate against algorithms_guaranteed and probe hashlib.new; this closes the platform‑specific pitfall. LGTM.
Based on learnings
226-251: Alternatives normalisation enforces types and strips emptiesEnforce list[str] with clear StageError messages; this removes input ambiguity. LGTM.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (31)
.github/actions/stage/action.yml(1 hunks).github/actions/stage/scripts/stage.py(1 hunks).github/actions/stage/scripts/stage_common/__init__.py(1 hunks).github/actions/stage/scripts/stage_common/checksum_utils.py(1 hunks).github/actions/stage/scripts/stage_common/config.py(1 hunks).github/actions/stage/scripts/stage_common/environment.py(1 hunks).github/actions/stage/scripts/stage_common/errors.py(1 hunks).github/actions/stage/scripts/stage_common/fs_utils.py(1 hunks).github/actions/stage/scripts/stage_common/github_output.py(1 hunks).github/actions/stage/scripts/stage_common/glob_utils.py(1 hunks).github/actions/stage/scripts/stage_common/staging.py(1 hunks).github/actions/stage/scripts/stage_common/template_utils.py(1 hunks).github/release-staging.toml(1 hunks).github/workflows/release.yml(4 hunks).github/workflows/scripts/normalise_windows_paths.py(1 hunks).github/workflows/scripts/stage_common.py(0 hunks).github/workflows/scripts/stage_macos.py(0 hunks).github/workflows/scripts/stage_windows.py(0 hunks)codex/refactor-python-staging-scripts-into-github-action(1 hunks)cyclopts/__init__.pyi(1 hunks)docs/netsuke-design.md(2 hunks)scripts/_release_upload_deps.py(1 hunks)scripts/upload_release_assets.py(7 hunks)tests_python/conftest.py(1 hunks)tests_python/stage_test_helpers.py(1 hunks)tests_python/test_stage_action.py(1 hunks)tests_python/test_stage_cli.py(1 hunks)tests_python/test_stage_common.py(0 hunks)tests_python/test_stage_common_config.py(1 hunks)tests_python/test_stage_common_module.py(1 hunks)tests_python/test_stage_common_staging.py(1 hunks)
💤 Files with no reviewable changes (4)
- .github/workflows/scripts/stage_macos.py
- .github/workflows/scripts/stage_common.py
- tests_python/test_stage_common.py
- .github/workflows/scripts/stage_windows.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/stage_test_helpers.pytests_python/test_stage_common_module.pyscripts/_release_upload_deps.pytests_python/test_stage_cli.pytests_python/test_stage_action.pytests_python/test_stage_common_config.pytests_python/test_stage_common_staging.pytests_python/conftest.pyscripts/upload_release_assets.py
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
🪛 Ruff (0.13.3)
.github/actions/stage/scripts/stage_common/template_utils.py
41-41: Remove quotes from type annotation
Remove quotes
(UP037)
scripts/_release_upload_deps.py
9-9: Import from collections.abc instead: Sequence
Import from collections.abc
(UP035)
89-89: Boolean positional value in function call
(FBT003)
92-92: Boolean positional value in function call
(FBT003)
.github/actions/stage/scripts/stage_common/__init__.py
8-17: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
.github/actions/stage/scripts/stage_common/staging.py
68-68: Remove quotes from type annotation
Remove quotes
(UP037)
149-149: Remove quotes from type annotation
Remove quotes
(UP037)
150-150: Remove quotes from type annotation
Remove quotes
(UP037)
tests_python/test_stage_common_staging.py
173-173: Unused function argument: stage_common
(ARG001)
295-295: Unused function argument: stage_common
(ARG001)
307-307: Unused function argument: stage_common
(ARG001)
319-319: Unused function argument: stage_common
(ARG001)
325-325: Probable insecure usage of temporary file or directory: "/tmp/dist/*.zip"
(S108)
330-330: Unused function argument: stage_common
(ARG001)
336-336: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (30)
scripts/upload_release_assets.py (5)
54-66: Good separation of concerns and dependency gating.Initialise support handles once and alias required symbols cleanly.
103-107: Type‑hint modernisation is sound.Use
typ.Iterator/typ.Iterablefor API clarity without over‑materialising collections.Also applies to: 219-226, 229-257
127-171: Runtime option normalisation reads cleanly.Env fallbacks, strict boolean handling, and explicit missing‑args error are correct.
336-351: Route Cyclopts CLI through shared normaliser.Bind CLI to
_prepare_runtime_options, ensuring consistent behaviour across paths.
355-362: Entrypoint delegation to run_cli is correct and minimal.Centralise CLI selection in the helper, keep the module lean.
.github/actions/stage/scripts/stage_common/checksum_utils.py (1)
11-21: LGTM!The chunked reading pattern efficiently handles large files without excessive memory usage, and the two-space checksum format matches GNU coreutils conventions.
.github/actions/stage/scripts/stage_common/fs_utils.py (1)
12-21: LGTM!The path traversal protection correctly resolves both paths before comparison, preventing symlink-based directory escape attacks. The validation-before-creation order ensures the filesystem is only modified for legitimate destinations.
.github/actions/stage/scripts/stage_common/github_output.py (1)
12-26: LGTM!The UUID delimiter approach prevents content collision, and the type-check correctly distinguishes strings from other sequences. The multiline syntax conforms to GitHub Actions' heredoc format.
.github/workflows/scripts/normalise_windows_paths.py (1)
1-17: LGTM!The module docstring clearly explains the script's purpose and provides a practical workflow integration example.
cyclopts/__init__.pyi (1)
1-23: LGTM!The stub definitions provide adequate typing for IDE support whilst maintaining flexibility with the permissive signatures. The
defaultmethod correctly preserves callable types usingParamSpec.docs/netsuke-design.md (1)
1681-1748: LGTM!The documentation updates accurately describe the new staging workflow and maintain consistency with the coding guidelines. The ER diagram clearly illustrates the configuration schema.
.github/actions/stage/scripts/stage_common/errors.py (1)
1-5: LGTM!The
StageErrorclass provides a clear, purpose-specific exception type for staging failures. Inheriting fromRuntimeErrorcorrectly signals non-recoverable errors..github/release-staging.toml (3)
1-10: LGTM!The
alternativeslist for the man page elegantly handles Cargo's variable build output locations, and theoutputkeys provide stable identifiers for workflow consumption.
12-22: LGTM!The Windows target configurations correctly use platform-specific architecture naming (
amd64,arm64) and the appropriate MSVC target triples.
24-42: LGTM!The Unix target configurations appropriately adapt architecture naming to each platform's conventions (Darwin's
x86_64vs Linux'samd64for Debian packages).codex/refactor-python-staging-scripts-into-github-action (1)
1-8: LGTM: Strict error handling and sequential quality gates.The script correctly uses
set -euo pipefailfor robust error propagation, and the sequential tooling steps (ruff format, ruff check, ty check, pytest) form a clear quality gate pipeline..github/actions/stage/scripts/stage_common/environment.py (1)
1-30: LGTM: Clear environment helper with proper error handling.The
require_env_pathfunction is well-documented with a numpy-style docstring, has correct type hints, and provides clear error messages when the environment variable is unset or empty. The implementation is straightforward and aligns with the public API surface tested intest_stage_common_module.py..github/actions/stage/action.yml (2)
1-59: LGTM: Well-structured composite action with dependency verification.The composite action correctly defines required inputs, exposes comprehensive outputs (including reserved keys for binary, man, and licence paths), and enforces strict shell error handling (
set -euo pipefail). The explicit verification ofuvavailability (lines 45-53) provides a clear error message if the dependency is missing.
42-42: Pinned SHA ensures reproducibility.Using a pinned commit SHA (
8d55fbecc275b1c35dbe060458839f8d30439ccf) forastral-sh/setup-uvis excellent practice for supply-chain security and build reproducibility..github/actions/stage/scripts/stage.py (1)
1-61: LGTM: Clear CLI entry point with robust error handling.The staging CLI is well-structured with:
- PEP 723 inline script metadata declaring dependencies (lines 1-6)
- Comprehensive module docstring with example usage (lines 8-19)
- Proper numpy-style docstring for
main(lines 35-43)- Type hints on all parameters and return type
- Robust error handling that formats errors for GitHub Actions (line 50) and exits non-zero
The success path (lines 53-57) provides clear feedback about staged artefacts.
tests_python/conftest.py (1)
1-34: LGTM: Clean test fixtures with proper isolation.Both fixtures are well-structured:
stage_common(lines 15-24) correctly manipulatessys.pathwith cleanup in afinallyblockworkspace(lines 27-34) creates an isolated temporary workspace and sets the environment variable appropriatelyPath manipulation uses
pathlibas required by the coding guidelines.tests_python/test_stage_action.py (1)
1-42: LGTM: Focused tests validating composite action configuration.The test suite effectively validates the composite action by:
- Checking required outputs are declared (lines 11-17)
- Verifying
uvinstallation steps (lines 20-25)- Confirming the staging script is invoked correctly (lines 28-33)
- Ensuring reserved output keys remain aligned with
stage_common.RESERVED_OUTPUT_KEYS(lines 36-42)Path manipulation uses
pathlibas required.tests_python/test_stage_common_module.py (1)
1-49: LGTM: Comprehensive validation of public API surface.The test suite thoroughly validates the staging helper package:
- Public interface matches documented API (lines 10-23)
StageErrorcorrectly subclassesRuntimeErrorwith message preservation (lines 26-31)require_env_pathreturns correctPathwhen environment variable is set (lines 34-38)require_env_pathraisesStageErrorwith clear message when variable is missing (lines 41-49)Type hints are present and correct.
.github/actions/stage/scripts/stage_common/__init__.py (2)
1-17: LGTM: Clear public API re-exports.The module correctly re-exports the public staging helper API from internal submodules, establishing a clean interface boundary. The
__all__list is comprehensive and matches the test expectations intest_stage_common_module.py.
8-17: Sort__all__per static analysis recommendation.Ruff flagged that
__all__is not sorted. Whilst the current ordering groups related symbols (config, environment, errors, staging), an isort-style alphabetical ordering improves maintainability and aligns with the RUF022 rule.Apply this diff to sort
__all__:__all__ = [ "ArtefactConfig", "RESERVED_OUTPUT_KEYS", "StageError", "StageResult", "StagingConfig", "load_config", "require_env_path", "stage_artefacts", ](Note: The current ordering is already alphabetical, so this comment reflects the static analysis hint. If you prefer semantic grouping over strict alphabetical order, add a narrow in-line disable with a FIXME comment as required by the coding guidelines.)
Likely an incorrect or invalid review comment.
tests_python/test_stage_common_config.py (1)
36-70: Solid coverage of loader merge behaviour and outputsExercise core fields and artefact outputs; assertions are precise. LGTM.
.github/actions/stage/scripts/stage_common/glob_utils.py (1)
10-36: Glob and path resolution logic reads clean and covers edge casesAbsolute/relative and Windows/POSIX handling and newest‑by‑mtime selection are correct. LGTM.
.github/workflows/release.yml (1)
175-203: Confirm stage I/O and normalisation outputs align
Inputs config-file and target match action.yml; artefact_map, binary_path and license_path outputs are emitted as expected..github/actions/stage/scripts/stage_common/staging.py (2)
125-133: Reserved output collision guard is correct and message is actionable.The check and guidance read well and block reserved key misuse as intended.
111-111: Avoid checksum key collisions in staging.py
Replacechecksums[outcome.path.name]with a key relative to the staging directory to prevent name collisions in subfolders:- checksums[outcome.path.name] = outcome.digest + rel_key = outcome.path.relative_to(staging_dir).as_posix() + checksums[rel_key] = outcome.digestApply the same change at the other occurrence. Verify whether
safe_destination_pathpermits subdirectories; if not, skip this.
| @dataclasses.dataclass(slots=True) | ||
| class ArtefactConfig: | ||
| """Describe a single artefact to be staged.""" | ||
|
|
||
| source: str | ||
| required: bool = True | ||
| output: str | None = None | ||
| destination: str | None = None | ||
| alternatives: list[str] = dataclasses.field(default_factory=list) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Expand class docstrings to NumPy style for public interfaces
Document fields and usage for ArtefactConfig and StagingConfig using NumPy sections (Parameters, Attributes, Examples) to meet repo guidelines.
As per coding guidelines
Also applies to: 48-63
| digest: str | ||
|
|
||
|
|
||
| def stage_artefacts(config: "StagingConfig", github_output_file: Path) -> StageResult: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Drop quotes from type annotations (UP037).
Keep future annotations and remove unnecessary stringified types.
Apply this diff:
-def stage_artefacts(config: "StagingConfig", github_output_file: Path) -> StageResult:
+def stage_artefacts(config: StagingConfig, github_output_file: Path) -> StageResult:
@@
- config: "StagingConfig",
- artefact: "ArtefactConfig",
+ config: StagingConfig,
+ artefact: ArtefactConfig,Also applies to: 149-150
🧰 Tools
🪛 Ruff (0.13.3)
68-68: Remove quotes from type annotation
Remove quotes
(UP037)
🤖 Prompt for AI Agents
.github/actions/stage/scripts/stage_common/staging.py lines 68 and 149-150:
remove unnecessary stringified type annotations introduced earlier—replace
quoted type names (e.g. "StagingConfig") with their bare identifiers
(StagingConfig) so annotations are actual types (keep the existing from
__future__ import annotations in place), and ensure imports cover those types if
not already imported; do this for the function signature at line 68 and the
other annotated occurrences at lines 149-150.
| staged_paths: list[Path] = [] | ||
| outputs: dict[str, Path] = {} | ||
| checksums: dict[str, str] = {} | ||
|
|
There was a problem hiding this comment.
Prevent destination collisions; reject reusing the same staged path.
Two artefacts can currently stage to the same destination, with the latter silently replacing the former. Guard against this.
Apply this diff:
staged_paths: list[Path] = []
outputs: dict[str, Path] = {}
checksums: dict[str, str] = {}
+ seen_destinations: set[Path] = set()
@@
- staged_paths.append(outcome.path)
+ if outcome.path in seen_destinations:
+ raise StageError(
+ f"Duplicate staged destination: {outcome.path.as_posix()}"
+ )
+ seen_destinations.add(outcome.path)
+ staged_paths.append(outcome.path)
checksums[outcome.path.name] = outcome.digest
if outcome.output_key:
outputs[outcome.output_key] = outcome.pathAlso applies to: 110-114
| if outcome.output_key: | ||
| outputs[outcome.output_key] = outcome.path | ||
|
|
There was a problem hiding this comment.
Detect and fail on duplicate artefact output keys.
Current logic silently overwrites earlier entries when two artefacts declare the same output key. Reject duplicates to avoid nondeterministic outputs.
Apply this diff:
- if outcome.output_key:
- outputs[outcome.output_key] = outcome.path
+ if outcome.output_key:
+ if outcome.output_key in outputs:
+ prev = outputs[outcome.output_key]
+ raise StageError(
+ f"Duplicate artefact output key {outcome.output_key!r}: "
+ f"{prev.as_posix()} and {outcome.path.as_posix()}"
+ )
+ outputs[outcome.output_key] = outcome.path📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if outcome.output_key: | |
| outputs[outcome.output_key] = outcome.path | |
| if outcome.output_key: | |
| if outcome.output_key in outputs: | |
| prev = outputs[outcome.output_key] | |
| raise StageError( | |
| f"Duplicate artefact output key {outcome.output_key!r}: " | |
| f"{prev.as_posix()} and {outcome.path.as_posix()}" | |
| ) | |
| outputs[outcome.output_key] = outcome.path |
🤖 Prompt for AI Agents
In .github/actions/stage/scripts/stage_common/staging.py around lines 112 to
114, the code unconditionally assigns outputs[outcome.output_key] = outcome.path
which silently overwrites earlier artefacts with the same key; change this to
detect duplicates and fail fast by first checking if outcome.output_key is
already present in outputs and, if so, raise an explicit exception or log an
error and exit (include the duplicate output key and both paths/artefact
identifiers in the message for debugging); otherwise proceed to assign the key
to the path.
| print( | ||
| f"Staged '{source_path.relative_to(config.workspace)}' ->" | ||
| f" '{destination_path.relative_to(config.workspace)}'", | ||
| ) |
There was a problem hiding this comment.
Avoid ValueError when destination is not under workspace.
relative_to(config.workspace) raises if the staging dir is outside the workspace. Fall back to an absolute/posix path.
Apply this diff:
- print(
- f"Staged '{source_path.relative_to(config.workspace)}' ->"
- f" '{destination_path.relative_to(config.workspace)}'",
- )
+ try:
+ dest_rel = destination_path.relative_to(config.workspace)
+ except ValueError:
+ dest_rel = destination_path
+ print(
+ f"Staged '{source_path.relative_to(config.workspace)}' ->"
+ f" '{dest_rel}'",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print( | |
| f"Staged '{source_path.relative_to(config.workspace)}' ->" | |
| f" '{destination_path.relative_to(config.workspace)}'", | |
| ) | |
| try: | |
| dest_rel = destination_path.relative_to(config.workspace) | |
| except ValueError: | |
| dest_rel = destination_path | |
| print( | |
| f"Staged '{source_path.relative_to(config.workspace)}' ->" | |
| f" '{dest_rel}'", | |
| ) |
🤖 Prompt for AI Agents
In .github/actions/stage/scripts/stage_common/staging.py around lines 191 to
194, the call to relative_to(config.workspace) can raise ValueError when
destination_path (or source_path) is outside the workspace; catch that case and
fall back to an absolute/posix representation. Modify the print to compute
safe_source = try source_path.relative_to(config.workspace) except ValueError:
source_path.as_posix() and safe_dest = try
destination_path.relative_to(config.workspace) except ValueError:
destination_path.as_posix(), then use those safe_source/safe_dest in the
formatted message so it never raises.
| stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """Absolute Windows-style globs should resolve relative to the drive root.""" | ||
|
|
||
| monkeypatch.chdir(workspace) | ||
|
|
||
| drive_root = Path("C:\\") | ||
| windows_workspace = drive_root / "workspace" | ||
| man_dir = windows_workspace / "man" | ||
| man_dir.mkdir(parents=True, exist_ok=True) | ||
| candidate = man_dir / "netsuke.1" | ||
| candidate.write_text(".TH WINDOWS", encoding="utf-8") | ||
|
|
||
| staging = importlib.import_module("stage_common.staging") | ||
| matched = staging._match_candidate_path( | ||
| windows_workspace, "C:/workspace/man/*.1" | ||
| ) | ||
|
|
||
| assert matched == candidate |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suppress unused fixture warning with a narrow, annotated noqa
Add a targeted, annotated suppression to comply with lint policy.
As per coding guidelines
-def test_match_candidate_path_handles_windows_drive(
- stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch
-) -> None:
+def test_match_candidate_path_handles_windows_drive( # noqa: ARG001 # FIXME: fixture triggers plugin import
+ stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| """Absolute Windows-style globs should resolve relative to the drive root.""" | |
| monkeypatch.chdir(workspace) | |
| drive_root = Path("C:\\") | |
| windows_workspace = drive_root / "workspace" | |
| man_dir = windows_workspace / "man" | |
| man_dir.mkdir(parents=True, exist_ok=True) | |
| candidate = man_dir / "netsuke.1" | |
| candidate.write_text(".TH WINDOWS", encoding="utf-8") | |
| staging = importlib.import_module("stage_common.staging") | |
| matched = staging._match_candidate_path( | |
| windows_workspace, "C:/workspace/man/*.1" | |
| ) | |
| assert matched == candidate | |
| def test_match_candidate_path_handles_windows_drive( # noqa: ARG001 # FIXME: fixture triggers plugin import | |
| stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| """Absolute Windows-style globs should resolve relative to the drive root.""" |
🧰 Tools
🪛 Ruff (0.13.3)
173-173: Unused function argument: stage_common
(ARG001)
🤖 Prompt for AI Agents
In tests_python/test_stage_common_staging.py around lines 173–191, the unused
pytest fixture parameter stage_common raises a lint warning; fix it by renaming
the parameter to _stage_common (preserving its type annotation, e.g.
_stage_common: object) so the fixture remains provided to the test but the
unused-variable warning is suppressed narrowly without a broad noqa.
| def test_glob_root_and_pattern_handles_windows_drive(stage_common: object) -> None: | ||
| """Absolute Windows globs should strip the drive before globbing.""" | ||
|
|
||
| staging = importlib.import_module("stage_common.staging") | ||
| helper = staging._glob_root_and_pattern | ||
|
|
||
| root, pattern = helper(PureWindowsPath("C:/dist/*.zip")) | ||
| assert root == "C:\\" | ||
| assert pattern == "dist/*.zip" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suppress unused fixture warning with a narrow, annotated noqa
Apply the same annotated suppression here.
As per coding guidelines
-def test_glob_root_and_pattern_handles_windows_drive(stage_common: object) -> None:
+def test_glob_root_and_pattern_handles_windows_drive(stage_common: object) -> None: # noqa: ARG001 # FIXME: fixture triggers plugin import📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_glob_root_and_pattern_handles_windows_drive(stage_common: object) -> None: | |
| """Absolute Windows globs should strip the drive before globbing.""" | |
| staging = importlib.import_module("stage_common.staging") | |
| helper = staging._glob_root_and_pattern | |
| root, pattern = helper(PureWindowsPath("C:/dist/*.zip")) | |
| assert root == "C:\\" | |
| assert pattern == "dist/*.zip" | |
| def test_glob_root_and_pattern_handles_windows_drive(stage_common: object) -> None: # noqa: ARG001 # FIXME: fixture triggers plugin import | |
| """Absolute Windows globs should strip the drive before globbing.""" | |
| staging = importlib.import_module("stage_common.staging") | |
| helper = staging._glob_root_and_pattern | |
| root, pattern = helper(PureWindowsPath("C:/dist/*.zip")) | |
| assert root == "C:\\" | |
| assert pattern == "dist/*.zip" |
🧰 Tools
🪛 Ruff (0.13.3)
295-295: Unused function argument: stage_common
(ARG001)
🤖 Prompt for AI Agents
In tests_python/test_stage_common_staging.py around lines 295-304, the test
function parameter stage_common is an unused pytest fixture and needs a narrow,
annotated noqa to suppress the unused-fixture warning; update the test signature
to add a per-parameter noqa comment on stage_common (e.g., annotate the
parameter with the specific flake/linters code) so only this unused-fixture
warning is silenced while leaving other linting intact.
| def test_glob_root_and_pattern_returns_wildcard_for_root_only( | ||
| stage_common: object, | ||
| ) -> None: | ||
| """Root-only absolute paths should glob all children.""" | ||
|
|
||
| staging = importlib.import_module("stage_common.staging") | ||
| helper = staging._glob_root_and_pattern | ||
|
|
||
| root, pattern = helper(PureWindowsPath("C:/")) | ||
| assert root == "C:\\" | ||
| assert pattern == "*" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suppress unused fixture warning with a narrow, annotated noqa
Apply the same annotated suppression here.
As per coding guidelines
-def test_glob_root_and_pattern_returns_wildcard_for_root_only(
- stage_common: object,
-) -> None:
+def test_glob_root_and_pattern_returns_wildcard_for_root_only( # noqa: ARG001 # FIXME: fixture triggers plugin import
+ stage_common: object,
+) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_glob_root_and_pattern_returns_wildcard_for_root_only( | |
| stage_common: object, | |
| ) -> None: | |
| """Root-only absolute paths should glob all children.""" | |
| staging = importlib.import_module("stage_common.staging") | |
| helper = staging._glob_root_and_pattern | |
| root, pattern = helper(PureWindowsPath("C:/")) | |
| assert root == "C:\\" | |
| assert pattern == "*" | |
| def test_glob_root_and_pattern_returns_wildcard_for_root_only( # noqa: ARG001 # FIXME: fixture triggers plugin import | |
| stage_common: object, | |
| ) -> None: | |
| """Root-only absolute paths should glob all children.""" | |
| staging = importlib.import_module("stage_common.staging") | |
| helper = staging._glob_root_and_pattern | |
| root, pattern = helper(PureWindowsPath("C:/")) | |
| assert root == "C:\\" | |
| assert pattern == "*" |
🧰 Tools
🪛 Ruff (0.13.3)
307-307: Unused function argument: stage_common
(ARG001)
🤖 Prompt for AI Agents
In tests_python/test_stage_common_staging.py around lines 306 to 317, the test
function accepts the unused fixture parameter stage_common and should suppress
the unused-fixture warning with a narrow annotated noqa; update the function
signature to attach a specific noqa comment to the parameter (for example:
stage_common: object # noqa: ARG001) so the unused fixture warning is silenced
only for that parameter while leaving other lints intact.
| def test_glob_root_and_pattern_handles_posix_absolute(stage_common: object) -> None: | ||
| """POSIX absolute paths should preserve relative segments for globbing.""" | ||
|
|
||
| staging = importlib.import_module("stage_common.staging") | ||
| helper = staging._glob_root_and_pattern | ||
|
|
||
| root, pattern = helper(PurePosixPath("/tmp/dist/*.zip")) | ||
| assert root == "/" | ||
| assert pattern.endswith("dist/*.zip"), pattern | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suppress unused fixture warning with a narrow, annotated noqa
Apply the same annotated suppression here.
As per coding guidelines
-def test_glob_root_and_pattern_handles_posix_absolute(stage_common: object) -> None:
+def test_glob_root_and_pattern_handles_posix_absolute(stage_common: object) -> None: # noqa: ARG001 # FIXME: fixture triggers plugin import🧰 Tools
🪛 Ruff (0.13.3)
319-319: Unused function argument: stage_common
(ARG001)
325-325: Probable insecure usage of temporary file or directory: "/tmp/dist/*.zip"
(S108)
🤖 Prompt for AI Agents
In tests_python/test_stage_common_staging.py around lines 319 to 328, the pytest
fixture parameter stage_common is unused and triggers a linter warning; add a
narrow annotated noqa on the parameter in the test function signature (e.g.,
append " # noqa: F841" to the stage_common parameter) so the suppression
targets only this unused fixture without silencing other checks.
| def test_glob_root_and_pattern_rejects_relative_paths(stage_common: object) -> None: | ||
| """Relative globs should be rejected to avoid ambiguous anchors.""" | ||
|
|
||
| staging = importlib.import_module("stage_common.staging") | ||
| helper = staging._glob_root_and_pattern | ||
|
|
||
| with pytest.raises(ValueError): | ||
| helper(PurePosixPath("dist/*.zip")) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tighten the exception assertion and suppress the unused fixture warning
Match the error text and add annotated suppression.
As per coding guidelines
-def test_glob_root_and_pattern_rejects_relative_paths(stage_common: object) -> None:
+def test_glob_root_and_pattern_rejects_relative_paths(stage_common: object) -> None: # noqa: ARG001 # FIXME: fixture triggers plugin import
@@
- with pytest.raises(ValueError):
+ with pytest.raises(ValueError, match="Expected absolute path"):
helper(PurePosixPath("dist/*.zip"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_glob_root_and_pattern_rejects_relative_paths(stage_common: object) -> None: | |
| """Relative globs should be rejected to avoid ambiguous anchors.""" | |
| staging = importlib.import_module("stage_common.staging") | |
| helper = staging._glob_root_and_pattern | |
| with pytest.raises(ValueError): | |
| helper(PurePosixPath("dist/*.zip")) | |
| def test_glob_root_and_pattern_rejects_relative_paths(stage_common: object) -> None: # noqa: ARG001 # FIXME: fixture triggers plugin import | |
| """Relative globs should be rejected to avoid ambiguous anchors.""" | |
| staging = importlib.import_module("stage_common.staging") | |
| helper = staging._glob_root_and_pattern | |
| with pytest.raises(ValueError, match="Expected absolute path"): | |
| helper(PurePosixPath("dist/*.zip")) |
🧰 Tools
🪛 Ruff (0.13.3)
330-330: Unused function argument: stage_common
(ARG001)
336-336: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
🤖 Prompt for AI Agents
In tests_python/test_stage_common_staging.py around lines 330 to 338, tighten
the exception assertion by using pytest.raises(ValueError, match="^Relative
globs are not allowed") (or the exact error message emitted by
_glob_root_and_pattern) instead of a bare pytest.raises, and suppress the unused
fixture warning by renaming the parameter to _stage_common: object (or add an
inline noqa comment) so the fixture remains available but the linter no longer
flags it as unused.
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Gates Failed
(2 files with Bumpy Road Ahead)
(6 files with Complex Method, Excess Number of Function Arguments, Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| staging.py | 1 critical rule | 9.34 | Suppress |
| glob_utils.py | 1 critical rule | 9.54 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| staging.py | 1 advisory rule | 9.34 | Suppress |
| test_stage_common_config.py | 1 advisory rule | 9.39 | Suppress |
| test_stage_common_staging.py | 1 advisory rule | 9.39 | Suppress |
| glob_utils.py | 1 advisory rule | 9.54 | Suppress |
| config.py | 1 advisory rule | 9.69 | Suppress |
| upload_release_assets.py | 1 advisory rule | 10.00 → 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
1 participant
Summary
StageErrorTesting
https://chatgpt.com/codex/tasks/task_e_68e85138f48883229e8082f03b90e95e
Summary by Sourcery
Implement a unified staging workflow for release assets by introducing a configurable staging helper package, documenting its usage, refactoring existing scripts for optional dependencies and fallback parsing, and wiring it into GitHub Actions with comprehensive testing and design documentation.
New Features:
Bug Fixes:
Enhancements:
CI:
target_keymatrix entriesDocumentation:
Tests: