feat(movie): support native Windows PowerShell and Git Bash - #2275
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The first, broader OS-compatibility rollout was stopped and replaced by the narrower Windows completion. Its feasibility probe, probe cleanup test, design, review, 12-task plan, results report, and the completion plan and review record were internal execution artifacts with machine-specific paths. The one probe-derived test list is inlined into the terminal suite. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The Python suite ported the three shell tests' assertions so they run on Windows; both copies were kept and the README mapped one to the other. Keep the portable suite. Drop the one-shot Windows acceptance driver and its browser fixture, which produced evidence rather than regressions, and the never-implemented reserved suite names. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Remove generic shell exit-status recipes and a stills wrapper that the card scene already covers. Keep the gdigrab commands and the verify-on notes short. Reduce the spec to the design: drop execution logistics, host names, and references to deleted files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The in-process narration and subtitle tests let the tools' stdout and stderr through to the runner. Capture both and assert the expected diagnostics. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…watch/close Windows has no tmux, so the recorder had grown into a 738-line daemon with a file-based request protocol, request IDs, wait-only result retrieval, and a Win32 Job Object module. Replace it with the Unix route's shape: serve keeps ttyd and a headless browser alive and logs the terminal's output; run, key, watch, and close are one-shot CDP calls against that browser. The installed prompt reports each command's status through the window title, so run can print it without any visible marker. Process cleanup uses taskkill /T (a pgrep walk on Unix) instead of Job Objects, which also simplifies the card renderer. The session tests run on macOS too, since nothing in the script is Windows-specific. Verified: 9 session tests per shell on Windows 11 for PowerShell 5.1, PowerShell 7, and Git Bash; the browser suite with Chrome and Edge; 44 portable tests on macOS against a real ttyd session. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c44ceee0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #!/usr/bin/env -S uv run --script | ||
| # /// script | ||
| # requires-python = ">=3.10" | ||
| # dependencies = ["websocket-client==1.9.0", "pillow"] |
There was a problem hiding this comment.
The new recorder requires websocket-client, and the test runner additionally declares websockets, even though this change explicitly is not new harness support. Script-local installation does not avoid the repository's prohibition on optional or required third-party dependencies, so this implementation must use the existing dependency-free surface or live outside core.
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Retain the recorder's used websocket-client dependency with Drew's explicit approval. 4d4ede29 removes unused websockets from the test runner.
The scoped dependency approval is recorded in the repair plan.
| Windows has no tmux, so the example script holds the session instead. `serve` | ||
| starts ttyd on the shell you name and a headless Chrome or Edge page showing | ||
| it, keeps both alive, and appends the raw terminal output to | ||
| `SESSION/terminal.log`. Every other verb is one short call against that | ||
| browser. Run `serve` in a background task your harness keeps alive, the way |
There was a problem hiding this comment.
The commit's Evaluation section says all four instruction sessions exercised the earlier recorder design and were not repeated after this command surface and documentation were rewritten. Consequently there is no before/after or adversarial behavioral evidence for the instructions that will actually ship; repeat the sessions on this exact workflow and report those results before landing the skill change.
AGENTS.md reference: AGENTS.md:L93-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fresh-reader baseline: Git Bash stops on the copied PowerShell & syntax; PowerShell 7 succeeds from a new directory. Final recipes in 9fe81b48 passed native PowerShell 5.1, PowerShell 7, and Git Bash trials covering readiness, echo, persistent shell state, interactive key continuation, long-command watch continuation, and close. Full transcripts and detailed reports (candidate-ps-report.md and candidate-bash-report.md) are retained locally; old recorder trials are not presented as evidence for this interface. These trials do not establish general skill auto-triggering or movie acceptance.
Committed results and evidence limits. The implementation worker accidentally ran a temporary-frame integration assertion before stopping; Drew was informed, and that run is excluded from acceptance evidence. The requested review videos were not opened.
| out.mkdir(parents=True, exist_ok=True) | ||
| start, index, last, stop = clock(), -1, None, None |
There was a problem hiding this comment.
When a failed or unsatisfactory take is retried with the same --record directory, film() overwrites only the frame indexes reached by the new run and leaves any higher-numbered PNGs from the previous run. The assembler later consumes every PNG in that directory, so a shorter retry silently ends with footage from the old take; remove existing frame-pattern files or require an empty output directory before capture.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4d4ede29 refuses a nonempty take before run/key/watch session side effects and before direct capture. Existing frames and unrelated sentinel bytes are preserved. Choose a new or empty take directory.
| From Git Bash the commands are the same with `skill=$(cygpath -m ...)` and | ||
| `work=$(cygpath -m ...)`, as in assembling.md. |
There was a problem hiding this comment.
For a user invoking the recorder from Git Bash, these lines say to reuse the PowerShell commands, but those commands begin with PowerShell's & uv call operator; Bash reports a syntax error when & prefixes a command. This leaves the advertised Git Bash terminal workflow without a runnable sequence, so document an actual Bash version, including how the blocking serve process is kept alive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
9fe81b48 replaces the PowerShell shorthand with actual Git Bash blocks, native path conversion, background-task lifetime, bounded readiness, run/key/watch/close, and expected exit-2 handling. The final native Git Bash trial completed successfully. Two preliminary outer driver launch failures were preserved separately; they occurred before the recorder ran.
|
|
||
| ```powershell | ||
| $skill = 'C:/path/to/skills/proving-it-works-with-a-movie' | ||
| $work = "$HOME/movie O'Brien λ & [take]" |
There was a problem hiding this comment.
On a fresh recording, assigning $work does not create that directory, while serve() explicitly exits when --cwd is not already a directory. Copying this documented PowerShell sequence therefore fails before ttyd or the browser starts unless some unrelated prior step happened to create the path; create $work before invoking serve.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reproduced for the documented recipe. serve creates $work/session with parents before checking $work, so the fresh cwd exists by validation time. A fresh-reader native PowerShell 7 baseline completed setup, persistent state, running status, and close without pre-creating $work. The earlier isolated reproduction placed the session outside cwd and did not match the documented layout. Explicit directory creation in the updated example is clarity, not a required runtime fix.
| return True | ||
|
|
||
| stdout, stderr = io.StringIO(), io.StringIO() | ||
| with patch.object(sys, "argv", ["burn-subtitles", str(movie), str(subs), str(output)]), patch.object(module, "has_libass", return_value=True), patch.object(module, "run", side_effect=fake_run), redirect_stdout(stdout), redirect_stderr(stderr): |
There was a problem hiding this comment.
In an environment without FFmpeg, these ostensibly mocked unit tests still call burn-subtitles.main(), whose initial shutil.which("ffmpeg") check raises SystemExit before the mocked has_libass or run functions are reached; the integration test also calls has_libass() directly and raises FileNotFoundError. Thus the default portable suite fails rather than reporting the unavailable capability as a skip as its README promises; mock the availability check in unit tests and guard the integration test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
4d4ede29 mocks executable discovery in subtitle unit tests and checks real integration prerequisites before probing libass. With only uv on PATH: four tests pass, one capability test skips, ordinary mode returns 0 and strict mode returns 1.
Prevent failed narration scenes from entering the cache manifest, while leaving their generated WAV files available as failure evidence. Add filesystem-backed regressions covering repeated rejected chat synthesis, accepted-scene reuse, and strict ASR rejection of cached audio. Refuse nonempty recording directories both before CLI session side effects and at the direct film boundary. The regression preserves existing numbered frames and a sentinel byte-for-byte across run, key, watch, and direct film refusal. Make subtitle capability tests independent of the host FFmpeg installation, skip the real pixel test before probing unavailable tools, retain strict skipped-capability rejection, and remove only the unused websockets runner dependency.
Replace the Git Bash PowerShell shorthand with complete native commands, explicit path conversion, a kept-alive serve task, bounded readiness, and run/key/watch/close examples. Use native input and sleep commands so the recipe works without a sample app. Explain empty take directories and PowerShell 5.1 embedded-quote escaping observed in native trials. The original PowerShell missing-cwd finding does not reproduce when the session is nested under the working directory; retain the successful baseline and describe explicit directory creation as setup clarity. Fresh readers exercised the final recipes on native PowerShell 5.1, PowerShell 7, and Git Bash. Preserve the failed first candidate and driver setup failures, distinguish instruction trials from full skill evaluation, and keep movie acceptance with Drew. Record Drew's approval of the normal workflow dependencies and the bounded repair plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fe81b48a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _descendants(pid: int) -> list[int]: | ||
| pids, index = [pid], 0 | ||
| while index < len(pids): | ||
| listed = subprocess.run(["pgrep", "-P", str(pids[index])], capture_output=True, text=True).stdout |
There was a problem hiding this comment.
On Unix installations without the optional pgrep utility, every cleanup attempt raises FileNotFoundError; render_card() therefore turns a successful screenshot into an assembly failure, while recorder cleanup can abort before killing either process. Use a dependency-free process-group or platform API, or provide a fallback when pgrep is absent.
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
| processes = [ | ||
| subprocess.Popen(ttyd_argv, cwd=cwd, stdin=subprocess.DEVNULL, stdout=logs[0], | ||
| stderr=subprocess.STDOUT, start_new_session=unix), | ||
| subprocess.Popen(browser_argv, cwd=directory, stdin=subprocess.DEVNULL, stdout=logs[1], | ||
| stderr=subprocess.STDOUT, start_new_session=unix), |
There was a problem hiding this comment.
If ttyd starts successfully but the browser Popen fails—for example because the selected executable becomes unavailable or Windows rejects its launch—the list construction raises before entering the later try/finally, leaving ttyd and its shell tree running and the log handles open. Start both processes inside a guarded block that terminates any process already launched on partial failure.
Useful? React with 👍 / 👎.
| browser_argv = [browser, "--headless=new", "--no-first-run", "--no-default-browser-check", | ||
| "--use-gl=angle", "--use-angle=swiftshader", "--enable-unsafe-swiftshader", | ||
| "--disable-background-networking", "--remote-debugging-address=127.0.0.1", | ||
| f"--remote-debugging-port={debug_port}", f"--user-data-dir={directory / 'profile'}", |
There was a problem hiding this comment.
When SESSION is relative, the browser receives a relative --user-data-dir=SESSION/profile while its working directory is already SESSION, so it creates SESSION/SESSION/profile. close() only removes SESSION/profile, leaving the real browser profile and cache behind; resolve the session directory first or pass a profile path relative to the browser's chosen cwd.
Useful? React with 👍 / 👎.
Address the fresh review on PR obra#2214 after the obra#2275 integration. Drew approved fixing the two reproduced bugs and keeping the specified auto/on/off verification semantics. Cache accepted narration by normalized text plus effective engine, voice, and synthesis model. Resolve voice defaults before rendering, invalidate entries without settings, and retain requested ASR checks on cache hits. Exclude rejected clips as before. Treat manual subtitle offsets as start-time overrides. Only assembly offsets JSON selects scenes in the cut, including when manual timing overrides are also supplied. Empty narrated cuts write an empty SRT without crashing. Make the gated Unix example pass --verify on and document the actual local ASR modes. Auto remains permissive if ASR is unavailable; on remains strict. Validation: observed the new cache and subtitle regressions fail before the fixes; all 23 focused narration and subtitle-text tests now pass. Synthesis, duration probing, and ASR are mocked. No media inspection or live ASR was performed; Drew retains final video acceptance.
1 participant
Who is submitting this PR? (required)
gpt-6-astraat medium reasoning, with an initialgpt-5.6-lunaworker. Scope reduction, the terminal recorder rewrite, and test cleanup: Claude Fable 5.1 (claude-fable-5-1).0.153.4through Paseo0.8.0-beta.1on macOS for the media work; Claude Code for the recorder rewrite and cleanup. Native instruction evaluations: Claude Code2.1.267on Windows.superpowers@superpowers-dev6.3.0,stream-deck-agents-codex@drew-local,deep-research-work0.1.15,plugin-management0.1.0. Claude Code session: superpowers, superpowers-chrome, episodic-memory, decision-log, linear, primeradiant-ops, cloud-build, iterative-development, elements-of-style. The Windows evaluations loaded the candidate Superpowers plugin and its bootstrap.What problem are you trying to solve?
Drew asked what Windows support looked like for #2214 and requested native Git Bash and PowerShell support. Actual Windows runs of the imported skill failed: FFmpeg's glob input is unsupported on Windows, the subtitle filter misreads
C:\paths, text files were read in the cp1252 locale so UTF-8 and BOM input broke, browser discovery only knew Mac and Linux paths, and the narration verifier spawnedpython3, which does not exist on Windows, so--verify onsilently passed without transcribing. The terminal recipe depends on tmux, which Windows does not have, and the docs had no PowerShell or Git Bash commands.Before the doc changes, a native PowerShell agent had to infer the recorder interface from source and broke on special-path quoting and BOM-bearing JSON. A Git Bash agent passed
/c/...to native Python and got aC:\c\...file-not-found error.What does this PR change?
--verify onnow transcribes cached clips too and fails when the transcriber is unavailable. Two small helpers,browser_tools.pyandmedia_paths.py, are shared by the tools.examples/film-terminal.py, keeps the Unix route's shape:servestarts ttyd on the chosen shell and a headless Chrome or Edge page showing it, keeps both alive in a background task, and logs the terminal output.run,key,watch, andcloseare one-shot CDP calls against that browser, so the shell and its state persist across tool calls. The installed prompt reports each command's status through the window title, invisible in the picture, andrunprints it. Frames are screenshots at 5 fps in a fixed 1600×900 viewport. Cleanup istaskkill /T.SKILL.md,assembling.md,narrating.md,recording-a-terminal.md, andrecording-motion.mdgive copyable PowerShell and Git Bash sequences.docs/superpowers/specs/.Is this change appropriate for the core library?
It extends the general-purpose movie skill being imported by #2214 to the Windows shells Superpowers already supports. It preserves the five CLI interfaces and the Unix tmux recipe. The recorder needs ttyd and a script-local
websocket-clientdependency; no global install dependency or paid service is introduced. Those workflow dependencies belong in the same conversation as #2214's existing FFmpeg and browser requirements.What alternatives did you consider?
uv run --scriptcommands.Does this PR contain multiple unrelated changes?
No. The tool fixes, the recorder, the instructions, and their tests all serve the one Windows workflow.
Existing PRs
movie Windowsand"proving-it-works"found no other Windows implementation. Hits docs: add Freebuff integration guide #2174 and feat: add diagnosing-superpowers skill — evidence-based session diagnosis, scrubbed bug-report bundles #2236 concern other features.Environment tested
claude-sonnet-5, low effortclaude-sonnet-5, low effortgpt-6-astra,gpt-5.6-luna,claude-fable-5-1Native host: Windows 11 build 26200, x64, Python 3.12.14, uv 0.12.12, FFmpeg 9.0.1 with libass, ttyd 1.7.7, PowerShell 5.1.26100.9168, PowerShell 7.6.6, Git Bash 5.3.9. No WSL, administrator rights, cloud key, or machine settings changes were needed; the final Windows run was made as an ordinary user with a Medium integrity token.
New harness support (required if this PR adds a new harness)
Not applicable.
Evaluation
Four instruction sessions with one fixed Windows scenario, same product code, model, and caches; only the docs changed between baseline and candidate. They ran against the earlier recorder design; the recorder's command surface has since been simplified and its documentation rewritten, and those sessions were not repeated.
/c/...to native Python; failed before repair. Stopped at the gap.Transcripts are retained locally and are not in this PR.
Rigor
superpowers:writing-skillsfor the doc changes with the four before/after sessions above.Validation of the current head:
8a31ffc3with three complete native movies, one per shell: all five tools, contact-sheet and hard-caption inspection, final rendered narration transcription, and source-audio checks. The media code is unchanged since, apart from the card renderer's cleanup path, which the Windows browser suite covers.gdigrabcapture produced readable application pixels. Full-desktop capture returned zero with only wallpaper and remains unverified.servespends it on a bare Enter before installing the prompt.Reproduction entry point:
tests/proving-it-works-with-a-movie/README.md.Human review
Drew asked for this draft so he can do that review. The unchecked box is deliberate.