Issue-135: introduce shared run-python-script skill and migrate python3 invocations - #136
Issue-135: introduce shared run-python-script skill and migrate python3 invocations#136jodavis-claude wants to merge 2 commits into
Conversation
…n3 invocations Centralizes the python3 preflight check and script invocation behind a new plugins/dev-team/skills/run-python-script/SKILL.md, then migrates every skill that inlined a bare python3 <script> Bash call (concurrent-orchestrate, dev-spec-task-breakdown, ensure-working-branch, get-project-configuration, implement-tdd, monitor-pr, update-project-configuration, use-context-file, workflow-orchestrate) to delegate to it. Removes the now-redundant standalone python3 preflight steps in concurrent-orchestrate and workflow-orchestrate (added in 4cf19b7) since run-python-script's own once-per-session check subsumes them. Documents workflow-script's --command contract, the two skills' inline 'python3 -c' blocks, and dev_team.py's sys.executable call sites as intentional carve-outs in run-python-script's own 'Carve-outs' section, rather than migrating them.
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed Issue-135 (PR #136): introduces the shared run-python-script skill and migrates all nine target skills off inline python3 <script> invocations.
Exit criteria check:
- New
run-python-script/SKILL.mdexists with the required behavior: verifiespython3(skipping if already verified this session), stop-and-tell failure if missing, then runs the script viapython3with--script/--python-flags/--args/--stdin/--timeout. ✅ - All nine listed skills (
concurrent-orchestrate,dev-spec-task-breakdown,ensure-working-branch,get-project-configuration,implement-tdd,monitor-pr,update-project-configuration,use-context-file,workflow-orchestrate) are migrated; spot-checked each call site against the pre-migration command and the script argument,--python-flags,--stdin, and--timeoutcontracts are all preserved. ✅ concurrent-orchestrate's andworkflow-orchestrate's standalone "### 0" preflight steps were removed in favor ofrun-python-script's own single check — exactly one place doing the check. ✅workflow-script's--commandcarve-out is explicitly documented, no functional change. ✅dev_team.py'ssys.executabledecision is documented as a deliberate no-change carve-out (see inline comment below for one accuracy issue in how it's described).- No
.pyfiles changed in this PR, so the existing pytest suite is unaffected — no regression risk from a code perspective.
Issues found: one documentation-accuracy issue, inline below. No Priority 1-3 (correctness/security/performance) issues — this is prose-only SKILL.md documentation work with no runtime code paths of its own.
Style (non-blocking):--args quoting style differs between call sites (single-quote wrapping in concurrent-orchestrate vs. escaped-double-quote wrapping elsewhere). Harmless since this is agent-interpreted prose, not literal shell, but could be made consistent for readability.
| already established in-session by the time either inline block runs — no separate preflight is | ||
| needed at these two call sites. | ||
| - **`dev_team.py`'s two generated command strings** (`_resolve_validation_script()`, | ||
| `BuildValidationStep.get_actions()`, and the PR-wait command build in `HandoffStep`) — these |
There was a problem hiding this comment.
This bullet says "two generated command strings" but then parenthetically lists three items. The third, "the PR-wait command build in HandoffStep", isn't a separate call site — it's BuildValidationStep.get_actions() (already named earlier in the same bullet) restated, and misattributed: that method lives in BuildValidationStep (instantiated by SignoffStep), not HandoffStep. HandoffStep.get_actions() (dev_team.py:1039-1053) only dispatches a spawn_agent action for final-sign-off — it builds no sys.executable command string at all. Recommend removing the ", and the PR-wait command build in HandoffStep" clause so the bullet accurately names exactly the two call sites it says it covers (_resolve_validation_script() at dev_team.py:288, BuildValidationStep.get_actions() at dev_team.py:813).
There was a problem hiding this comment.
Agreed — confirmed HandoffStep.get_actions() (dev_team.py:1039-1053) only dispatches a spawn_agent action for final-sign-off and builds no sys.executable command string. Removed the misattributed ", and the PR-wait command build in HandoffStep" clause in c4714b2 so the bullet accurately names exactly the two real call sites: _resolve_validation_script() (dev_team.py:288) and BuildValidationStep.get_actions() (dev_team.py:813).
…L.md Removed the misattributed 'PR-wait command build in HandoffStep' clause per PR #136 review comment — HandoffStep.get_actions() only dispatches a spawn_agent action and builds no sys.executable command string; the bullet now accurately names exactly the two real call sites (_resolve_validation_script(), BuildValidationStep.get_actions()).
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review for Issue-135 (PR #136).
Prior review thread check:
- The single unresolved thread on
plugins/dev-team/skills/run-python-script/SKILL.md(inaccurate carve-outs bullet misattributing a 'PR-wait command build' toHandoffStep) has been satisfactorily addressed in commitc4714b2. Verified againstdev_team.py:HandoffStep.get_actions()(lines 1039-1053) only dispatches aspawn_agentaction forfinal-sign-offand builds nosys.executablecommand string — the bullet now accurately names exactly the two real call sites (_resolve_validation_script()at line 288,BuildValidationStep.get_actions()at line 813). No dangling references to the removed clause elsewhere in the repo.
New issues scan (files modified since last review push):
- Only
plugins/dev-team/skills/run-python-script/SKILL.mdchanged (commitc4714b2, a 4-line/5-line prose-only diff). No new Priority 1-4 issues found — this is documentation-only content with no runtime code path.
Sign-off decision: approved.
Note: I was unable to programmatically resolve the prior thread via the GitHub API in this session (write action blocked by the local permission classifier); the thread's fix is verified correct and ready for the thread to be marked resolved.
| - `--command` — the shell command to run (e.g. `python3 -u /path/to/validate.py PROJ-123`), | ||
| already fully resolved by the caller (sometimes literally `python3 ...`, sometimes built by | ||
| `dev_team.py` using `sys.executable`). This is a documented carve-out from the `run-python-script` | ||
| skill: `--command` is a pre-built shell string handed in by the caller, not a `python3 <script>` | ||
| invocation this skill constructs itself, so step 2 below keeps running it directly via `Bash` | ||
| rather than delegating to `run-python-script` — see `run-python-script/SKILL.md`'s | ||
| "Carve-outs" section. |
There was a problem hiding this comment.
Why do we need any of this new detail about the carve-out? We're just running the command line as given, so there's no reason to even consider using run-python-script, until it's brought up here. Just leave this change out.
| ## Carve-outs (not migrated to this skill) | ||
| - **`workflow-script`'s `--command` argument.** Its `<command>` is a pre-built shell string handed | ||
| in by the caller (sometimes literally `python3 ...`, sometimes built by `dev_team.py` using | ||
| `sys.executable`) — `workflow-script` runs whatever string it's given via `Bash`, it never | ||
| constructs a `python3 <script>` invocation itself the way every other skill migrated to this | ||
| skill does. No functional change; this is documented here as the intentional reason it stays | ||
| out of scope. | ||
| - **Inline `python3 -c "<code>"` blocks** (`workflow-orchestrate/SKILL.md`'s | ||
| `troubleshooter_input` write, `monitor-pr/SKILL.md`'s rebase-mechanic invocation). These run | ||
| inline code, not a script file at a path — outside this skill's `--script <path>` shape. Each | ||
| stays as a direct inline `python3 -c` call. Since both live in skills whose own earlier steps | ||
| already call this skill first in the same session (`workflow-orchestrate`'s step 1; | ||
| `monitor-pr`'s step 4a via the sibling `watch_pr_poll.py` call), `python3`'s availability is | ||
| already established in-session by the time either inline block runs — no separate preflight is | ||
| needed at these two call sites. | ||
| - **`dev_team.py`'s two generated command strings** (`_resolve_validation_script()`, | ||
| `BuildValidationStep.get_actions()`) — these already use `sys.executable`, not a literal | ||
| `python3` invocation, because that code path is real Python building a command string for a | ||
| later shell invocation, not agent-facing skill prose constructing a `python3 <script>` call | ||
| itself. This skill only wraps the latter; no change is needed here. |
There was a problem hiding this comment.
Is it necessary to call all of these out? They just don't delegate to the skill. Including them here makes ever agent that uses this skill think about all the places where it's not used. Why?
Work item
Issue-135: Introduce a shared
run-python-scriptskill that centralizes thepython3existence check and script invocation, then migrate every skill that currently inlines apython3 <script>call to delegate to it — giving the pipeline one place to change the interpreter requirement later instead of N independent inline checks.Changes
plugins/dev-team/skills/run-python-script/SKILL.md(new) — shared internal skill (user-invocable: false) that verifiespython3is available (skipping the check once already verified this session) with a clear stop-and-tell failure if missing, then runs a given script viapython3, supporting--script,--python-flags,--args,--stdin, and--timeout. Includes a "Carve-outs" section documenting what was deliberately not migrated.plugins/dev-team/skills/concurrent-orchestrate/SKILL.md— removed the standalone "### 0 — Verify python3" preflight step; migrated all threeconcurrent_schedule.pyinvocations torun-python-script; added it to the Skills list.plugins/dev-team/skills/workflow-orchestrate/SKILL.md— removed its standalone "### 0" preflight step; migratedget_context_path.pyanddev_team.py(preserving the-uinterpreter flag via--python-flags) torun-python-script; left the inlinepython3 -ctroubleshooter-input write unchanged (documented carve-out).plugins/dev-team/skills/dev-spec-task-breakdown/SKILL.md— migrated thetask_dependencies.pyinvocation.plugins/dev-team/skills/ensure-working-branch/SKILL.md— migrated thetask_dependencies.pyandtask_readiness.pyinvocations.plugins/dev-team/skills/get-project-configuration/SKILL.md— migrated themerge_config.pyinvocation.plugins/dev-team/skills/implement-tdd/SKILL.md— migrated thetdd_cycle.pyinvocation, preserving its stdin-piped component prompt via--stdin; addedrun-python-scriptto the Skills list.plugins/dev-team/skills/monitor-pr/SKILL.md— migrated thewatch_pr_poll.pyinvocation; left the inlinepython3 -crebase-mechanic call unchanged (documented carve-out); updated the Skills list.plugins/dev-team/skills/update-project-configuration/SKILL.md— migrated bothmerge_config.pyinvocations (initial load and post-write verification).plugins/dev-team/skills/use-context-file/SKILL.md— migrated thecompute-context-file.pyandinit-context-file.pyinvocations.plugins/dev-team/skills/workflow-script/SKILL.md— documented its--commandargument as an intentional, unmigrated carve-out (pre-built shell string handed in by the caller, not apython3 <script>call this skill constructs itself); no functional change.Design decisions
--script <path> [--python-flags] [--args] [--stdin] [--timeout], resolved via one required argument and four optional ones, rather than a single pre-built command string — this fit every real call site without inventing anything unused.python3 -cinline-code call sites (workflow-orchestratetroubleshooter-input write,monitor-prrebase mechanic) were left as documented carve-outs rather than extended intorun-python-script's scope — they run inline code, not a script file, and both already sit downstream of an earlierrun-python-scriptcall in the same session, sopython3's availability is already established by the time they run.concurrent-orchestrate's andworkflow-orchestrate's standalone "### 0" steps rather than keeping an explicit step 0 that callsrun-python-script. Their own step 1 is now the firstrun-python-scriptcall, satisfying "exactly one place doing the check."_doc_*.mdfile for this convention — recorded the decisions (thepython3-only standardization, thesys.executablecarve-out, the-ccarve-out, the--commandcarve-out) directly inrun-python-script/SKILL.md's own "Carve-outs" section instead.dev_team.py's two generated command-string call sites continue usingsys.executableas-is (unchanged, per PR Issue-128: Standardize python3 invocation across skill instructions and scripts #133's rationale that this is "real Python, not agent-facing prose") — confirmed and documented rather than modified.Closes#135
Testing completed
No
.pyfiles were created or modified; this is prose-skill (SKILL.md) work only. Ran the full existing pytest suite (plugins/dev-team/skills/*/scripts,plugins/dev-team/fixtures/*) to confirm zero regressions: all 556 existing tests still pass. Verified manually via a dry run of the described check-then-run sequence (merge_config.py).