ci: extract inline Python from workflows into testable scripts - #113
Merged
Conversation
📝 WalkthroughWalkthroughRefactors inline CI workflow logic into testable Python CLIs and a shared ChangesWorker CI/CD Script Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
.github/scripts/validate_worker.py (2)
67-78: 💤 Low valueAvoid double-reporting when
deploy/languageis missing.If
m.deploy(orm.language) is empty/None, both the "missing key" branch on Line 69 and the enum branch on Line 73/75 fire, producing two::error::annotations for the same root cause. Gate the enum checks on a truthy value so only the more-specific error appears.♻️ Proposed change
- if m.deploy not in ("binary", "image"): + if m.deploy and m.deploy not in ("binary", "image"): hard(f"{worker}/iii.worker.yaml deploy must be 'binary' or 'image'") - if m.language not in ("rust", "node", "python"): + if m.language and m.language not in ("rust", "node", "python"): hard( f"{worker}/iii.worker.yaml language must be 'rust' | 'node' | 'python'" )🤖 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 @.github/scripts/validate_worker.py around lines 67 - 78, The enum validation currently runs even when fields are missing, causing duplicate errors; update the validation in validate_worker.py to only perform the deploy and language enum checks when those attributes are truthy (e.g., change the conditions around m.deploy and m.language so they are checked with an existence guard), so that the "missing key" hard(...) fires alone if the value is absent and the enum hard(...) fires only when a non-empty value is present; keep references to the existing variables m and worker and the same error messages from the deploy/language checks.
36-38: 💤 Low valueMinor: malformed
--source-changedwill surface as an uncaught traceback.
json.loads(args.source_changed)propagatesjson.JSONDecodeErrorwithout an::error::annotation, so a caller mistake (e.g. passing an unquoted shell expansion) is reported as a Python stack trace in CI logs rather than a one-line failure. A small try/except returning1with a clear message would be friendlier; behaviour-wise this is fine.🤖 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 @.github/scripts/validate_worker.py around lines 36 - 38, Wrap the json.loads(args.source_changed) call in a try/except that catches json.JSONDecodeError, logs a single-line CI-friendly error (e.g. using print with "::error::" and the original input) and exits with status 1; update the code around the worker/source_changed logic (variables: args.source_changed, source_changed, strict) so that on JSON decode failure the script prints the annotated error and returns non-zero instead of letting the traceback propagate..github/scripts/tests/conftest.py (1)
82-99: 💤 Low valueOptional: collapse repeated subprocess boilerplate.
The six identical
subprocess.run(..., cwd=tmp_path, check=True, env=GIT_HERMETIC_ENV)calls could be factored into a smallgit(*args)closure (as done intest_discover_changed_workers.make_repo_with_workers) to make the fixture a touch shorter and harder to misalign. Not blocking.🤖 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 @.github/scripts/tests/conftest.py around lines 82 - 99, The tmp_git_repo_with_tag fixture repeats identical subprocess.run invocations; factor that boilerplate into a small helper (e.g., a local closure like git(*args) or a private helper function) that calls subprocess.run(args, cwd=tmp_path, check=True, env=GIT_HERMETIC_ENV) and then replace each direct subprocess.run call in tmp_git_repo_with_tag with calls to that helper (retain the same args for init, config, add, commit, tag so behavior is unchanged)..github/scripts/collect_worker_interface.py (1)
127-132: 💤 Low valueMinor:
and not args.assert_fileis unreachable here.When
args.assert_fileis truthy the function already returns at Line 101, so this guard never fires. Dropping it would tighten the control flow, but leaving it as defensive coding is fine.♻️ Optional simplification
- if args.assert_non_empty and not args.assert_file: + if args.assert_non_empty: with open(args.out) as f: data = json.load(f) if not data.get("functions"): print(f"::error::no worker functions in {args.out} (empty)", file=sys.stderr) return 1🤖 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 @.github/scripts/collect_worker_interface.py around lines 127 - 132, The condition `and not args.assert_file` in the block guarded by `if args.assert_non_empty and not args.assert_file:` is unreachable because `args.assert_file` is already handled earlier (returns when truthy); remove the redundant `and not args.assert_file` from the `if` so the check becomes `if args.assert_non_empty:` and keep the body that opens `args.out`, loads JSON, and checks `data.get("functions")` as-is; update any comments accordingly to reflect the simplified control flow..github/scripts/discover_changed_workers.py (2)
55-64: 💤 Low valueDiff fallback masks the original failure.
When the
base...headdiff fails (e.g. shallow clone missingbase), the fallback toHEAD~1...HEADruns without surfacing the first error. If that fallback also fails the exception is raised raw. Consider logging the swallowed error as a::warning::so debugging shallow-clone or detached-HEAD issues in CI doesn't require local repro. Non-blocking.🤖 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 @.github/scripts/discover_changed_workers.py around lines 55 - 64, The changed_files function swallows the original CalledProcessError when falling back to HEAD~1...HEAD; modify changed_files to catch the exception as a variable (e.g., except subprocess.CalledProcessError as e) and emit a CI-friendly warning containing the original error (for example using a ::warning:: print to stdout) before attempting the fallback command, so the initial failure is visible while preserving the existing fallback behavior.
67-75: ⚡ Quick winConsider reusing
_lib.read_iii_worker_yamlfor consistency withvalidate_worker.py.The current line-by-line parser is fragile compared to proper YAML parsing via
yaml.safe_load(). While it works for the unquoted language values present in the codebase today, it would handle quoted values (language: "rust") and indented lines inconsistently. Sincevalidate_worker.pyalready uses_lib.read_iii_worker_yaml()to parse the same files, standardizing on that approach across both scripts would improve maintainability. Note thatpyyamlis already installed in the discover job (ci.yml line 60), so no additional dependency setup is needed.🤖 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 @.github/scripts/discover_changed_workers.py around lines 67 - 75, The manual line-by-line YAML parsing in language_of(worker_dir) is fragile; replace it by importing and calling _lib.read_iii_worker_yaml(worker_dir) (as used in validate_worker.py), then extract and return the "language" key (or None) from the returned dict. Ensure you handle when read_iii_worker_yaml returns None or a dict missing "language", and keep the same return type (str | None). Update language_of to rely on the library parser to correctly handle quoted/indented values and avoid duplicating parsing logic..github/scripts/tests/test_lib.py (1)
29-30: ⚡ Quick winTest name and assertion encode the prerelease-lex behavior as desired.
This will pass for the current implementation, but as flagged on
_lib.parse_semver,"rc.1" < "rc.2"is the spec-compliant outcome only by coincidence (digit-by-digit lex equals numeric for single digits). If you adopt the suggested fix for numeric prerelease identifiers, also add a case like"1.2.3-rc.2" < "1.2.3-rc.10"so the test suite enforces SemVer §11 instead of locking in lex-order.🤖 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 @.github/scripts/tests/test_lib.py around lines 29 - 30, Update the test to assert numeric-aware prerelease ordering rather than pure lexicographic order: in the test_two_prereleases_sort_lexicographically function, keep the existing _lib.parse_semver("1.2.3-rc.1") < _lib.parse_semver("1.2.3-rc.2") assertion and add an assertion such as _lib.parse_semver("1.2.3-rc.2") < _lib.parse_semver("1.2.3-rc.10") to ensure numeric prerelease identifiers are compared numerically per SemVer §11..github/scripts/manifest_version.py (1)
80-94: 💤 Low value
deploy-modeassumesscripts:andruntime:are mappings.
raw.get("scripts") or {}only guards against the key being missing or null; if a worker'siii.worker.yamlever setsscripts:to a list or scalar,scripts.get("start")will raiseAttributeErrorand the workflow will fail with an opaque error rather than a cleanunsupported. Same applies toruntime. Cheap to harden:- raw = m.raw - scripts = raw.get("scripts") or {} - runtime = raw.get("runtime") or {} + raw = m.raw + scripts = raw.get("scripts") if isinstance(raw.get("scripts"), dict) else {} + runtime = raw.get("runtime") if isinstance(raw.get("runtime"), dict) else {}🤖 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 @.github/scripts/manifest_version.py around lines 80 - 94, The code assumes scripts and runtime are mappings (variables raw, scripts, runtime, has_scripts_start, has_runtime) and will raise if those fields are a list/scalar; guard by validating types before calling .get (e.g., treat scripts = raw.get("scripts") and if not isinstance(scripts, dict) then scripts = {}; same for runtime) or wrap the .get access in a safe check/try to compute has_scripts_start and has_runtime; update the logic that sets has_scripts_start and has_runtime to only call .get when the value is a mapping so non-mapping YAML values yield the fallback "unsupported" path cleanly..github/scripts/_lib.py (2)
33-52: 💤 Low value
bumpshould strip build metadata for consistency withparse_semver.
parse_semverignores everything after+per spec §10, butbumponly partitions on-. As a result,bump("1.0.0+build", "patch")raisesValueErroronint("0+build"). Today's manifests don't carry build metadata, so this is latent rather than active, but the asymmetry will trip up any future caller.♻️ Proposed fix
- core, _, _pre = current.partition("-") + nobuild, _, _ = current.partition("+") + core, _, _pre = nobuild.partition("-") parts = [int(x) for x in core.split(".")]🤖 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 @.github/scripts/_lib.py around lines 33 - 52, The bump function currently only strips pre-release suffixes by partitioning on "-" which leaves build metadata (after "+") and causes int() to fail (e.g., "1.0.0+build"); update bump to also remove build metadata the same way parse_semver does by trimming anything after a "+" before splitting the core into parts (or partition on "+" first, then partition the remaining on "-" for pre-release), then proceed to pad parts and apply the major/minor/patch logic in the bump function.
16-30: ⚡ Quick winPrerelease comparison deviates from SemVer 2.0.0 §11 for numeric identifiers.
The trailing string is compared lexically, but per spec, dot-separated identifiers consisting only of digits MUST be compared numerically. Concretely:
- Current:
parse_semver("1.2.3-rc.10") < parse_semver("1.2.3-rc.2")(because"rc.10" < "rc.2"lexically).- Spec:
rc.10 > rc.2.The docstring acknowledges this with "lexically orders", but the PR description claims "spec-correct parse_semver". Although no double-digit prerelease versions exist in the codebase today, the moment any worker uses
rc.10or higher, comparisons will be incorrect. Consider splitting the prerelease into a tuple where numeric identifiers compare as(0, int)and non-numeric as(1, str).♻️ Proposed fix
-SemverKey = tuple[tuple[int, ...], int, str] +SemverKey = tuple[tuple[int, ...], int, tuple[tuple[int, int | str], ...]] @@ - parts = [int(x) for x in core.split(".")] - while len(parts) < 3: - parts.append(0) - return (tuple(parts), 0 if pre else 1, pre) + parts = [int(x) for x in core.split(".")] + while len(parts) < 3: + parts.append(0) + pre_key: tuple[tuple[int, int | str], ...] = () + if pre: + ids: list[tuple[int, int | str]] = [] + for ident in pre.split("."): + if ident.isdigit(): + ids.append((0, int(ident))) # numeric < alphanumeric per §11 + else: + ids.append((1, ident)) + pre_key = tuple(ids) + return (tuple(parts), 0 if pre else 1, pre_key)🤖 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 @.github/scripts/_lib.py around lines 16 - 30, parse_semver currently treats the prerelease portion as a single string and orders it lexically, which breaks SemVer §11 numeric identifier rules; update parse_semver so after stripping build metadata and splitting core/pre, convert prerelease into a tuple of identifier tokens (split on '.') where each token is normalized to a comparable form: numeric identifiers become (0, int(value)) and non-numeric become (1, str(value)); return (core_tuple, 0 if prerelease else 1, prerelease_tokens_tuple) so numeric prerelease segments compare numerically while preserving the existing stable-vs-prerelease ordering; refer to the function parse_semver and the SemverKey concept when making the change..github/scripts/parse_release_tag.py (1)
1-7: 💤 Low valueDocstring says "Writes 10 keys" but the script writes 11.
targetswas added topairsbut the header still advertises 10 keys.📝 Suggested edit
-Writes 10 keys to $GITHUB_OUTPUT: +Writes 11 keys to $GITHUB_OUTPUT: tag, worker, version, deploy, language, - bin, manifest, registry_tag, is_prerelease, dry_run + bin, manifest, registry_tag, is_prerelease, dry_run, targets🤖 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 @.github/scripts/parse_release_tag.py around lines 1 - 7, The module docstring incorrectly states "Writes 10 keys" while the code adds an 11th key 'targets' to the pairs list; update the docstring to accurately list 11 keys (or explicitly include "targets" in the documented keys) so it matches the actual outputs emitted by the script (referencing the docstring string and the 'pairs' variable and 'targets' item)..github/workflows/create-tag.yml (1)
97-117: 💤 Low value
Discover manifestis still grep/awk while the rest of the workflow uses_lib.This step extracts the manifest filename with
grep '^manifest:' | head -n1 | awk '{print $2}', which will silently misread quoted values (manifest: "Cargo.toml"→"Cargo.toml"with the literal quotes) and ignores anything outside a top-level key. Now that_lib.read_iii_worker_yamlis available andpyyamlis already a CI dep, consider exposing amanifest-path(or similar) subcommand onmanifest_version.pyand switching to it for symmetry and robustness.♻️ Sketch of a Python-driven discover
- MANIFEST=$(grep '^manifest:' "$WORKER/iii.worker.yaml" | head -n1 | awk '{print $2}') - if [[ -z "$MANIFEST" ]]; then - echo "::error::$WORKER/iii.worker.yaml has no 'manifest' key" - exit 1 - fi + pip install --quiet pyyaml + MANIFEST=$(python3 .github/scripts/manifest_version.py manifest-path "$WORKER") echo "manifest=$MANIFEST" >> "$GITHUB_OUTPUT"…paired with a tiny
cmd_manifest_pathhandler that reuses_lib.read_iii_worker_yaml(...).manifest.🤖 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 @.github/workflows/create-tag.yml around lines 97 - 117, Replace the shell parsing in the "Discover manifest" step with the new Python-based helper: add a subcommand (e.g., cmd_manifest_path) to manifest_version.py that calls _lib.read_iii_worker_yaml(...) and returns the worker manifest path (exposing it as a CLI flag like --manifest-path), then update the workflow step to invoke that subcommand (pass WORKER env) instead of using grep/awk; ensure the new CLI strips quotes and reads the top-level manifest key robustly by returning the .manifest attribute from the parsed object and write it to GITHUB_OUTPUT as before.
🤖 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 @.github/scripts/_lib.py:
- Around line 33-52: The bump function currently only strips pre-release
suffixes by partitioning on "-" which leaves build metadata (after "+") and
causes int() to fail (e.g., "1.0.0+build"); update bump to also remove build
metadata the same way parse_semver does by trimming anything after a "+" before
splitting the core into parts (or partition on "+" first, then partition the
remaining on "-" for pre-release), then proceed to pad parts and apply the
major/minor/patch logic in the bump function.
- Around line 16-30: parse_semver currently treats the prerelease portion as a
single string and orders it lexically, which breaks SemVer §11 numeric
identifier rules; update parse_semver so after stripping build metadata and
splitting core/pre, convert prerelease into a tuple of identifier tokens (split
on '.') where each token is normalized to a comparable form: numeric identifiers
become (0, int(value)) and non-numeric become (1, str(value)); return
(core_tuple, 0 if prerelease else 1, prerelease_tokens_tuple) so numeric
prerelease segments compare numerically while preserving the existing
stable-vs-prerelease ordering; refer to the function parse_semver and the
SemverKey concept when making the change.
In @.github/scripts/collect_worker_interface.py:
- Around line 127-132: The condition `and not args.assert_file` in the block
guarded by `if args.assert_non_empty and not args.assert_file:` is unreachable
because `args.assert_file` is already handled earlier (returns when truthy);
remove the redundant `and not args.assert_file` from the `if` so the check
becomes `if args.assert_non_empty:` and keep the body that opens `args.out`,
loads JSON, and checks `data.get("functions")` as-is; update any comments
accordingly to reflect the simplified control flow.
In @.github/scripts/discover_changed_workers.py:
- Around line 55-64: The changed_files function swallows the original
CalledProcessError when falling back to HEAD~1...HEAD; modify changed_files to
catch the exception as a variable (e.g., except subprocess.CalledProcessError as
e) and emit a CI-friendly warning containing the original error (for example
using a ::warning:: print to stdout) before attempting the fallback command, so
the initial failure is visible while preserving the existing fallback behavior.
- Around line 67-75: The manual line-by-line YAML parsing in
language_of(worker_dir) is fragile; replace it by importing and calling
_lib.read_iii_worker_yaml(worker_dir) (as used in validate_worker.py), then
extract and return the "language" key (or None) from the returned dict. Ensure
you handle when read_iii_worker_yaml returns None or a dict missing "language",
and keep the same return type (str | None). Update language_of to rely on the
library parser to correctly handle quoted/indented values and avoid duplicating
parsing logic.
In @.github/scripts/manifest_version.py:
- Around line 80-94: The code assumes scripts and runtime are mappings
(variables raw, scripts, runtime, has_scripts_start, has_runtime) and will raise
if those fields are a list/scalar; guard by validating types before calling .get
(e.g., treat scripts = raw.get("scripts") and if not isinstance(scripts, dict)
then scripts = {}; same for runtime) or wrap the .get access in a safe check/try
to compute has_scripts_start and has_runtime; update the logic that sets
has_scripts_start and has_runtime to only call .get when the value is a mapping
so non-mapping YAML values yield the fallback "unsupported" path cleanly.
In @.github/scripts/parse_release_tag.py:
- Around line 1-7: The module docstring incorrectly states "Writes 10 keys"
while the code adds an 11th key 'targets' to the pairs list; update the
docstring to accurately list 11 keys (or explicitly include "targets" in the
documented keys) so it matches the actual outputs emitted by the script
(referencing the docstring string and the 'pairs' variable and 'targets' item).
In @.github/scripts/tests/conftest.py:
- Around line 82-99: The tmp_git_repo_with_tag fixture repeats identical
subprocess.run invocations; factor that boilerplate into a small helper (e.g., a
local closure like git(*args) or a private helper function) that calls
subprocess.run(args, cwd=tmp_path, check=True, env=GIT_HERMETIC_ENV) and then
replace each direct subprocess.run call in tmp_git_repo_with_tag with calls to
that helper (retain the same args for init, config, add, commit, tag so behavior
is unchanged).
In @.github/scripts/tests/test_lib.py:
- Around line 29-30: Update the test to assert numeric-aware prerelease ordering
rather than pure lexicographic order: in the
test_two_prereleases_sort_lexicographically function, keep the existing
_lib.parse_semver("1.2.3-rc.1") < _lib.parse_semver("1.2.3-rc.2") assertion and
add an assertion such as _lib.parse_semver("1.2.3-rc.2") <
_lib.parse_semver("1.2.3-rc.10") to ensure numeric prerelease identifiers are
compared numerically per SemVer §11.
In @.github/scripts/validate_worker.py:
- Around line 67-78: The enum validation currently runs even when fields are
missing, causing duplicate errors; update the validation in validate_worker.py
to only perform the deploy and language enum checks when those attributes are
truthy (e.g., change the conditions around m.deploy and m.language so they are
checked with an existence guard), so that the "missing key" hard(...) fires
alone if the value is absent and the enum hard(...) fires only when a non-empty
value is present; keep references to the existing variables m and worker and the
same error messages from the deploy/language checks.
- Around line 36-38: Wrap the json.loads(args.source_changed) call in a
try/except that catches json.JSONDecodeError, logs a single-line CI-friendly
error (e.g. using print with "::error::" and the original input) and exits with
status 1; update the code around the worker/source_changed logic (variables:
args.source_changed, source_changed, strict) so that on JSON decode failure the
script prints the annotated error and returns non-zero instead of letting the
traceback propagate.
In @.github/workflows/create-tag.yml:
- Around line 97-117: Replace the shell parsing in the "Discover manifest" step
with the new Python-based helper: add a subcommand (e.g., cmd_manifest_path) to
manifest_version.py that calls _lib.read_iii_worker_yaml(...) and returns the
worker manifest path (exposing it as a CLI flag like --manifest-path), then
update the workflow step to invoke that subcommand (pass WORKER env) instead of
using grep/awk; ensure the new CLI strips quotes and reads the top-level
manifest key robustly by returning the .manifest attribute from the parsed
object and write it to GITHUB_OUTPUT as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be347755-174d-409f-9c99-9ce7f316df0b
📒 Files selected for processing (19)
.github/scripts/_lib.py.github/scripts/collect_worker_interface.py.github/scripts/discover_changed_workers.py.github/scripts/manifest_version.py.github/scripts/parse_release_tag.py.github/scripts/tests/__init__.py.github/scripts/tests/_test_helpers.py.github/scripts/tests/conftest.py.github/scripts/tests/test_collect_assert_non_empty.py.github/scripts/tests/test_discover_changed_workers.py.github/scripts/tests/test_lib.py.github/scripts/tests/test_manifest_version.py.github/scripts/tests/test_parse_release_tag.py.github/scripts/tests/test_validate_worker.py.github/scripts/validate_worker.py.github/workflows/_publish-registry.yml.github/workflows/ci.yml.github/workflows/create-tag.yml.github/workflows/release.yml
Comment on lines
+20
to
+22
| TAG_RE = re.compile(r"^([a-z0-9][a-z0-9_-]*)/v(.+)$") | ||
| DRY_RUN_RE = re.compile(r"-dry-run\.\d+$") | ||
| PRERELEASE_RE = re.compile(r"-[a-z]+\.\d+$") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every git tag pattern this repo actually produces to confirm coverage.
rg -nP --type=yaml -C1 "'-[a-z]+/v\*'|tags:" .github/workflows
rg -nP -C2 "rc\.|alpha|beta|next\.|dry-run\." .githubRepository: iii-hq/workers
Length of output: 7600
🏁 Script executed:
cat -n .github/scripts/parse_release_tag.pyRepository: iii-hq/workers
Length of output: 3595
Add a comment documenting the strict prerelease tag format constraint.
The PRERELEASE_RE pattern enforces a narrow convention: lowercase identifier, dot, digit suffix (e.g., -rc.1, -next.2). This intentionally rejects other SemVer prerelease formats like -alpha, -RC.1, or -beta.1.0. Add a comment pinning this contract since changing the pattern without reviewing all tag conventions in the repo could misclassify tags. If the team plans to support other formats, broaden the pattern to r"^.*-[a-zA-Z0-9.-]+" (any prerelease identifier per SemVer 2.0.0 §9).
Separately, the docstring at line 4 claims "Writes 10 keys" but the code writes 11 (including targets at line 82).
A PR touching harness/ now adds every in-repo worker listed in harness/iii.worker.yaml dependencies to the rust matrix, so lint+test runs against the new shared crate instead of waiting for tag time. Deps enter changed_workers only, not source_changed: version-bump and README/tests gates still apply only to workers the author edited. Bundle release continues to own downstream version bumps.
_lib.read_iii_worker_yaml silently fills name with the folder name when the yaml omits it, so the previous getattr-based required-key check passed trivially and the m.name != folder gate was unreachable. Switch validate_worker to inspect the raw dict so a missing name is caught.
ytallo
force-pushed
the
feat/ci-script-extraction
branch
from
May 13, 2026 12:11
26eb84d to
c7673c6
Compare
andersonleal
approved these changes
May 13, 2026
Contributor
skill-check — worker6 verified, 19 skipped (no docs/). 72 errors across the verified workers.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/scripts/manifest_version.py (1)
106-116: 💤 Low valueConsider adding help text for consistency.
Arguments on lines 106, 111, and 116 lack help text, while the
manifestargument on line 102 includes it. Adding brief descriptions would improve consistency and UX.📝 Proposed help text additions
p_bump = sub.add_parser("bump", help="bump the manifest version in place") - p_bump.add_argument("manifest") + p_bump.add_argument("manifest", help="path to Cargo.toml/package.json/pyproject.toml") p_bump.add_argument("--kind", choices=["patch", "minor", "major"], required=True) p_bump.set_defaults(func=cmd_bump) p_verify = sub.add_parser("verify", help="assert the manifest version equals --expected") - p_verify.add_argument("manifest") + p_verify.add_argument("manifest", help="path to Cargo.toml/package.json/pyproject.toml") p_verify.add_argument("--expected", required=True) p_verify.set_defaults(func=cmd_verify) p_dm = sub.add_parser("deploy-mode", help="print the interface-collection mode") - p_dm.add_argument("worker_dir") + p_dm.add_argument("worker_dir", help="path to worker directory") p_dm.set_defaults(func=cmd_deploy_mode)🤖 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 @.github/scripts/manifest_version.py around lines 106 - 116, Add brief help text to the argument definitions that currently lack descriptions: update p_bump.add_argument("manifest"), p_bump.add_argument("--kind"...) if desired, p_verify.add_argument("manifest"), and p_dm.add_argument("worker_dir") to include short help strings (e.g., "path to manifest file", "bump type: patch/minor/major", "expected manifest version", "worker directory containing interfaces") so the CLI is consistent with the existing manifest argument help and improves UX; locate these calls in the functions where p_bump, p_verify, and p_dm are defined and pass the help="..." parameter to each add_argument invocation.
🤖 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/scripts/discover_changed_workers.py:
- Around line 24-31: Update the METADATA_GLOBS tuple in
discover_changed_workers.py so metadata-only manifest edits for Node and Python
are treated as metadata changes: add "package.json" and "pyproject.toml" (and
any other language-specific manifest files you want normalized) to the
METADATA_GLOBS tuple so the logic that uses METADATA_GLOBS will classify those
edits as metadata-only rather than source changes.
In @.github/scripts/manifest_version.py:
- Around line 24-60: The except clauses in cmd_read, cmd_bump, and cmd_verify
currently only catch FileNotFoundError and ValueError; update them to catch
OSError instead of FileNotFoundError (or add OSError) so all filesystem I/O
errors from Path.read_text()/write_text() (PermissionError, IsADirectoryError,
etc.) are handled and the functions return exit code 1 as intended; modify the
except tuples in the functions cmd_read, cmd_bump, and cmd_verify to include
OSError.
---
Nitpick comments:
In @.github/scripts/manifest_version.py:
- Around line 106-116: Add brief help text to the argument definitions that
currently lack descriptions: update p_bump.add_argument("manifest"),
p_bump.add_argument("--kind"...) if desired, p_verify.add_argument("manifest"),
and p_dm.add_argument("worker_dir") to include short help strings (e.g., "path
to manifest file", "bump type: patch/minor/major", "expected manifest version",
"worker directory containing interfaces") so the CLI is consistent with the
existing manifest argument help and improves UX; locate these calls in the
functions where p_bump, p_verify, and p_dm are defined and pass the help="..."
parameter to each add_argument invocation.
🪄 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: CHILL
Plan: Pro
Run ID: cde10aa4-703d-4417-9058-ea96ce8911e8
📒 Files selected for processing (19)
.github/scripts/_lib.py.github/scripts/collect_worker_interface.py.github/scripts/discover_changed_workers.py.github/scripts/manifest_version.py.github/scripts/parse_release_tag.py.github/scripts/tests/__init__.py.github/scripts/tests/_test_helpers.py.github/scripts/tests/conftest.py.github/scripts/tests/test_collect_assert_non_empty.py.github/scripts/tests/test_discover_changed_workers.py.github/scripts/tests/test_lib.py.github/scripts/tests/test_manifest_version.py.github/scripts/tests/test_parse_release_tag.py.github/scripts/tests/test_validate_worker.py.github/scripts/validate_worker.py.github/workflows/_publish-registry.yml.github/workflows/ci.yml.github/workflows/create-tag.yml.github/workflows/release.yml
🚧 Files skipped from review as they are similar to previous changes (14)
- .github/workflows/release.yml
- .github/scripts/tests/test_parse_release_tag.py
- .github/scripts/tests/test_collect_assert_non_empty.py
- .github/scripts/tests/_test_helpers.py
- .github/workflows/_publish-registry.yml
- .github/scripts/tests/test_manifest_version.py
- .github/scripts/parse_release_tag.py
- .github/scripts/tests/test_validate_worker.py
- .github/scripts/validate_worker.py
- .github/scripts/collect_worker_interface.py
- .github/workflows/create-tag.yml
- .github/scripts/tests/test_lib.py
- .github/scripts/_lib.py
- .github/workflows/ci.yml
Comment on lines
+24
to
+31
| METADATA_GLOBS = ( | ||
| "iii.worker.yaml", | ||
| "README.md", | ||
| "AGENTS.md", | ||
| "AGENTS-*.md", | ||
| "Cargo.lock", | ||
| "Cargo.toml", | ||
| ) |
There was a problem hiding this comment.
Normalize metadata-only manifest handling across languages
On Line 24, METADATA_GLOBS includes Rust manifests but excludes package.json and pyproject.toml. As written, Node/Python version-only manifest edits are classified as source changes, which can incorrectly trigger source-change gates.
Suggested fix
METADATA_GLOBS = (
"iii.worker.yaml",
"README.md",
"AGENTS.md",
"AGENTS-*.md",
"Cargo.lock",
"Cargo.toml",
+ "package.json",
+ "pyproject.toml",
)📝 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.
Suggested change
| METADATA_GLOBS = ( | |
| "iii.worker.yaml", | |
| "README.md", | |
| "AGENTS.md", | |
| "AGENTS-*.md", | |
| "Cargo.lock", | |
| "Cargo.toml", | |
| ) | |
| METADATA_GLOBS = ( | |
| "iii.worker.yaml", | |
| "README.md", | |
| "AGENTS.md", | |
| "AGENTS-*.md", | |
| "Cargo.lock", | |
| "Cargo.toml", | |
| "package.json", | |
| "pyproject.toml", | |
| ) |
🤖 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 @.github/scripts/discover_changed_workers.py around lines 24 - 31, Update the
METADATA_GLOBS tuple in discover_changed_workers.py so metadata-only manifest
edits for Node and Python are treated as metadata changes: add "package.json"
and "pyproject.toml" (and any other language-specific manifest files you want
normalized) to the METADATA_GLOBS tuple so the logic that uses METADATA_GLOBS
will classify those edits as metadata-only rather than source changes.
Comment on lines
+24
to
+60
| def cmd_read(args: argparse.Namespace) -> int: | ||
| path = Path(args.manifest) | ||
| try: | ||
| print(_lib.read_version(path)) | ||
| except (FileNotFoundError, ValueError) as e: | ||
| print(f"error: {e}", file=sys.stderr) | ||
| return 1 | ||
| return 0 | ||
|
|
||
|
|
||
| def cmd_bump(args: argparse.Namespace) -> int: | ||
| path = Path(args.manifest) | ||
| try: | ||
| current = _lib.read_version(path) | ||
| new = _lib.bump(current, args.kind) | ||
| _lib.write_version(path, new) | ||
| except (FileNotFoundError, ValueError) as e: | ||
| print(f"error: {e}", file=sys.stderr) | ||
| return 1 | ||
| print(new) | ||
| return 0 | ||
|
|
||
|
|
||
| def cmd_verify(args: argparse.Namespace) -> int: | ||
| path = Path(args.manifest) | ||
| try: | ||
| actual = _lib.read_version(path) | ||
| except (FileNotFoundError, ValueError) as e: | ||
| print(f"error: {e}", file=sys.stderr) | ||
| return 1 | ||
| if actual != args.expected: | ||
| print( | ||
| f"version mismatch in {path}: expected {args.expected}, got {actual}", | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 | ||
| return 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check _lib exception handling and docstrings
# Find exception types raised or documented in _lib.py
rg -n -A 3 -B 3 'def (read_version|write_version|bump)\(' .github/scripts/_lib.py
# Look for raise statements in these functions
ast-grep --pattern $'def read_version($$$):
$$$
raise $$$
$$$'
ast-grep --pattern $'def write_version($$$):
$$$
raise $$$
$$$'Repository: iii-hq/workers
Length of output: 892
🏁 Script executed:
cat .github/scripts/_lib.pyRepository: iii-hq/workers
Length of output: 6981
Incomplete exception handling for I/O errors.
The try/except blocks in cmd_read, cmd_bump, and cmd_verify only catch FileNotFoundError and ValueError, but the underlying _lib functions call Path.read_text() and Path.write_text() which can raise other OSError subclasses: PermissionError, IsADirectoryError, NotADirectoryError, etc. These uncaught exceptions will cause the script to crash with a Python traceback instead of returning exit code 1 as documented in the docstring (line 11: "Exit codes: 0 on success, 1 on parse / IO / mismatch failure").
Catch OSError instead of FileNotFoundError in all three functions, since OSError is the base class for all I/O-related exceptions including FileNotFoundError:
Proposed fix
def cmd_read(args: argparse.Namespace) -> int:
path = Path(args.manifest)
try:
print(_lib.read_version(path))
- except (FileNotFoundError, ValueError) as e:
+ except (OSError, ValueError) as e:
print(f"error: {e}", file=sys.stderr)
return 1
return 0Apply the same change to cmd_bump (line 40) and cmd_verify (line 51).
🤖 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 @.github/scripts/manifest_version.py around lines 24 - 60, The except clauses
in cmd_read, cmd_bump, and cmd_verify currently only catch FileNotFoundError and
ValueError; update them to catch OSError instead of FileNotFoundError (or add
OSError) so all filesystem I/O errors from Path.read_text()/write_text()
(PermissionError, IsADirectoryError, etc.) are handled and the functions return
exit code 1 as intended; modify the except tuples in the functions cmd_read,
cmd_bump, and cmd_verify to include OSError.
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
Moves the inline Python heredocs that were embedded in
.github/workflows/*.ymlinto proper scripts under.github/scripts/, with a pytest suite that gates them via a newscripts-testsjob inci.yml.Why
Before this change, non-trivial logic (semver parsing, per-language manifest IO, PR-change detection, worker validation, tag parsing, deploy-mode detection) lived as
python3 - <<PYblocks inside YAML — no syntax highlighting, no unit tests, no shared helpers.parse_semverin particular had three subtly different implementations acrossci.yml,release-harness-bundle.yml, andcreate-tag-harness-bundle.yml; one of them silently treated1.2.3-rc.1as equal to1.2.3, which would cause pre-release publishes to be skipped against an existing stable in the registry.After: one source of truth in
.github/scripts/_lib.py, every consumer imports from it, and 73 pytest cases pin the behavior. The audit-flaggedparse_semverbug is fixed (stable now strictly outranks pre-release at the same core, per semver 2.0.0).Changes
New helpers (
.github/scripts/)_lib.py— single source of truth for shared primitives:parse_semver(v)with correct ordering (1.2.3-rc.1 < 1.2.3 < 1.2.4-rc.1) and+buildmetadata stripped per semver 2.0.0 §10bump(current, kind)for patch/minor/majordetect_kind(path)dispatching onCargo.toml/package.json/pyproject.tomlread_version(path)/write_version(path, new)via shared TOML-section helpers (cargo and pyproject share one implementation; node usesjson)WorkerManifestfrozen dataclass +read_iii_worker_yaml(worker_dir)read_tag_annotation(tag)parsingkey: valuelines fromgit tag -l --format=%(contents)manifest_version.py— CLI withread | bump | verify | deploy-modesubcommandsdiscover_changed_workers.py— enumerates PR-changed worker folders, buckets by language, marks workers whose only changes were metadatavalidate_worker.py— per-worker PR-checks (README present,iii.worker.yamlvalid, manifest version strictly greater than base,tests/non-empty); soft-errors when the worker only had metadata changesparse_release_tag.py— parses<worker>/v<X.Y.Z>tags and emits the setup outputsrelease.ymlneeds (including thetargetsfield for cross-compile)Extended
collect_worker_interface.pygains--assert-non-emptyplus a standalone--assert-file <path>mode that bypasses collection (used by_publish-registry.ymlto validate an existing interface JSON)Workflow rewires
ci.ymlmanifest_typeplumbing removedcreate-tag.ymlmanifest_version.py bumpcall (read + compute + write atomic) plus oneverifycall;manifest_typediscovery dropped (script dispatches on filename)release.ymlpython3 .github/scripts/parse_release_tag.py "$RAW_TAG"_publish-registry.ymlmanifest_version.py deploy-mode+collect_worker_interface.py --assert-fileTest suite
.github/scripts/tests/withconftest.py,_test_helpers.py(hermetic git env), and 5 test filesscripts-testsjob inci.yml+buildmetadata, multi-language manifests (cargo/node/python), metadata-only changes downgraded to notices, malformed tags, missing files, frozen-dataclass guarantees,git rev-parsefailure on equal base/HEADNotable behavior changes
parse_semveris now spec-correct —1.2.3-rc.1 < 1.2.3(was: equal, in the bundle workflows). Any future bundle-pipeline comparison that thought pre-release was equal to stable will now correctly publish the pre-release version when it's newer than what's in the registry. No existing release tag in this repo uses pre-release suffixes, so no in-flight workflow behavior changes for current data.create-tag.ymlcollapses "calculate next version" + "update manifest" into one step —manifest_version.py bumpis atomic (read → compute → write → print new). One fewer step, same end state._publish-registry.yml's interface assertion runs as its own step rather than as a trailing heredoc inside the collector step. Failures attribute to the right step in the Actions UI.Compatibility
release.yml's setup emits the same 11 keys (includingtargets),ci.yml's discover emits the same 7 keys,create-tag.yml's versions emits the same 3 keys.build_publish_payload.py,resolve_binary_artifacts.py) are untouched.collect_worker_interface.py's existing CLI is fully backwards-compatible; new flags are additive.Follow-ups (not in this PR)
Three inline Python heredocs remain in workflows that landed on
mainconcurrent with this PR (via the harness-bundle release) and weren't in the original inventory:release-harness-bundle.yml— bundle publish_set computation (~100 lines)create-tag-harness-bundle.yml— in-repo dep pre-bump walker_rust-binary.yml—targetsinput processorThese can reuse
_lib.parse_semver,_lib.read_iii_worker_yaml, andmanifest_version.pydirectly, but the extraction is mechanical and worth a separate follow-up to keep this PR's diff focused.Test plan
pytest .github/scripts/tests/ -v→ 73 passed locallygrep -c "python3 - <<\|python3 -c \"" .github/workflows/*.ymlshows 0 for every file this PR touched (ci.yml,create-tag.yml,release.yml,_publish-registry.yml)scripts-testsjob passespr-checksmatrix correctly buckets the workers changed in this PR (none, since this only touches.github/)next-channel patch release of a low-stakes worker (todo-worker-pythonexercises the python path) via Create Tag and confirm the full release flow succeeds end-to-endSummary by CodeRabbit