refactor: deepen prepare-image architecture - #67
Conversation
📝 WalkthroughWalkthroughThe PR moves image preparation into a staged batch workflow and adds a dedicated image engine. The engine handles probing, orientation, layout, color conversion, rendering, and sRGB metadata. The CLI prints committed paths. Integration tests cover batching, rollback, collisions, rendering, metadata, color conversion, and bundled tools. Bun scripts, CI checks, project guidance, and product documentation are updated. Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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: 5
🤖 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 `@src/domain/image_engine.ts`:
- Around line 518-529: Update getSourceColorProperties to preserve conversion
when only some color tags are available: retain validated primaries and transfer
values, default an unset range to “pc” for still images, and infer “gbr” for RGB
pixel formats when color_space is unknown or absent. Return null only when the
required color information cannot be safely resolved, rather than requiring all
four tags to be explicitly present.
- Around line 225-229: Update the image preparation flow around
inspectSourceDimensions, computePrepareImageLayout, and buildPrepareImageFilter
so EXIF orientations 5–8 are normalized before cropping and scaling, using the
matching orientation transform while preserving the existing rotation handling.
Ensure crop dimensions are based on the transformed frame geometry and avoid
applying swapped dimensions to an unrotated 0:v stream.
In `@src/domain/prepare_image.ts`:
- Around line 84-86: Remove the unreachable empty-string guard from planImages,
since it receives the resolved output path and empty --out values are already
rejected by the CLI directory validation. Keep the existing file-path validation
behavior for resolved paths.
In `@tests/helpers/cli.ts`:
- Around line 15-18: The CLI test helper’s Bun invocation must work with a
stripped PATH. Update the process setup around Bun.spawn to invoke the
prepare-image entrypoint through the absolute process.execPath and ensure the
child environment supplies absolute ffmpeg and ffprobe paths, using the resolved
ffmpeg-static and existing bundled ffprobe symbols while preserving
caller-provided environment overrides.
In `@tests/integration/prepare_image.integration.test.ts`:
- Around line 192-200: The hasExifOrientationTag and hasSrgbColorSpaceTag
helpers must stop scanning the entire JPEG buffer, which can match compressed
image data. Reuse the APP1/Exif segment-walking approach from image_engine.ts:
traverse markers from SOI, extract the Exif payload from APP1 segments, and
search the tag patterns only within that payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3480f66b-08fa-4c11-9dc0-0b0a3707cf8d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc.github/workflows/ci.yml.gitignore.oxfmtrc.jsonAGENTS.mdCONTEXT.mdREADME.mddocs/agents/domain.mddocs/agents/issue-tracker.mddocs/plans/bundle-ffmpeg-binaries.mddocs/plans/decisions-log.mddocs/plans/prepare-image-directory-input.mddocs/plans/typescript-7-migration.mddocs/plans/v1-prepare-image-cli.mddocs/plans/v1-prepare-image-layout.mddocs/plans/v1-product-split-cleanup.mddocs/v1_split_plan.mdindex.tspackage.jsonsrc/cli/prepare_image.tssrc/domain/image_engine.tssrc/domain/media_process.tssrc/domain/output_path.tssrc/domain/prepare_image.tssrc/domain/prepare_image_layout.tstests/helpers/cli.tstests/integration/prepare_image.integration.test.tstests/output_path.test.tstests/prepare_image_layout.test.ts
💤 Files with no reviewable changes (15)
- tests/output_path.test.ts
- docs/plans/v1-prepare-image-cli.md
- docs/plans/v1-product-split-cleanup.md
- .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc
- tests/prepare_image_layout.test.ts
- docs/plans/typescript-7-migration.md
- docs/plans/v1-prepare-image-layout.md
- index.ts
- docs/v1_split_plan.md
- docs/plans/prepare-image-directory-input.md
- src/domain/output_path.ts
- docs/plans/bundle-ffmpeg-binaries.md
- src/domain/prepare_image_layout.ts
- src/domain/media_process.ts
- .gitignore
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
docs/agents/issue-tracker.md
📄 CodeRabbit inference engine (AGENTS.md)
Track executable work in Linear Issues/Subissues and consult
docs/agents/issue-tracker.mdfor issue-tracker guidance.
Files:
docs/agents/issue-tracker.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Run TypeScript typechecking withbunx tscand maintain type-correct TypeScript code.
Use type-aware Oxlint and Oxfmt verification as part of the quality checks.
The v1 acceptance test must continue to pass, including the command and image-engine acceptance coverage.
Support PNG, JPEG, and TIFF input files and directories containing those image types.
Place each input image on a white canvas and export a high-quality baseline sRGB JPEG suitable for manual Instagram upload.
Preserve atomic output allocation, commit, and rollback behavior in the image-preparation workflow.
Support a--border-pxinteger option, including zero, and the documented--helpcommand.
Use systemffmpegandffprobewhen available while maintaining independently working bundled adapters when they are absent fromPATH.
Files:
tests/helpers/cli.tssrc/domain/prepare_image.tssrc/domain/image_engine.tssrc/cli/prepare_image.tstests/integration/prepare_image.integration.test.ts
docs/plans/decisions-log.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/plans/decisions-log.md: Record durable architecture or product decisions immediately indocs/plans/decisions-log.md.
Readdocs/plans/decisions-log.mdbefore domain-sensitive work.
Files:
docs/plans/decisions-log.md
CONTEXT.md
📄 CodeRabbit inference engine (AGENTS.md)
Read
CONTEXT.mdbefore domain-sensitive work.
Files:
CONTEXT.md
src/domain/prepare_image.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep source discovery, staging, atomic output allocation, commit, and rollback in
src/domain/prepare_image.ts.
Files:
src/domain/prepare_image.ts
src/domain/image_engine.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep FFmpeg/ffprobe adapters, orientation handling, layout, white-canvas rendering, and JPEG export in
src/domain/image_engine.ts.
Files:
src/domain/image_engine.ts
src/cli/prepare_image.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep argv parsing and stdout/stderr adaptation in
src/cli/prepare_image.ts.
Files:
src/cli/prepare_image.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: edhor1608/passepartout
Timestamp: 2026-07-26T08:39:46.672Z
Learning: Use `bun install --frozen-lockfile` for installation and `bun run check` before pull requests.
Learnt from: CR
Repo: edhor1608/passepartout
Timestamp: 2026-07-26T08:39:46.672Z
Learning: Keep executable work in Linear and prefer explicit code plus local comments over new slice Markdown.
📚 Learning: 2026-05-21T12:23:11.207Z
Learnt from: edhor1608
Repo: edhor1608/passepartout PR: 63
File: docs/agents/domain.md:34-34
Timestamp: 2026-05-21T12:23:11.207Z
Learning: In this repository (edhor1608/passepartout), the internal agent skill `grill-with-docs` is already established and understood by agents in the codebase. Code review should not require adding redundant explanations or links to `grill-with-docs` in agent-facing docs (e.g., `docs/agents/**/*.md`); only document it if there’s a concrete reason (e.g., introducing a new behavior or dependency that isn’t already understood).
Applied to files:
docs/agents/domain.mddocs/agents/issue-tracker.md
🪛 LanguageTool
AGENTS.md
[grammar] ~41-~41: Use a hyphen to join words.
Context: ...very acceptance test with an explicit 60 second per-test timeout. - `bun run test...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (14)
src/domain/image_engine.ts (1)
283-324: LGTM!Also applies to: 540-605
src/domain/prepare_image.ts (1)
33-75: LGTM!Also applies to: 94-108, 132-147
src/cli/prepare_image.ts (1)
1-1: LGTM!Also applies to: 75-78
tests/integration/prepare_image.integration.test.ts (1)
56-108: LGTM!Also applies to: 287-353, 418-462, 499-583, 644-679
package.json (2)
9-10: LGTM!Also applies to: 12-18
8-8: 📐 Maintainability & Code QualityVerify the type-aware Oxlint quality-gate contract at the package script.
The executable command must enable type-aware analysis before the documentation and durable verification record make that claim.
package.json#L8-L8: verify the supported Oxlint configuration/CLI option and enable type-aware mode if absent.AGENTS.md#L27-L28: retain the type-aware quality-gate claim only after the package command/configuration is confirmed.README.md#L72-L73: keep the user-facing quality-gate description aligned with the actual check.docs/plans/decisions-log.md#L1060-L1061: record the verification result only after the same gate is proven.As per coding guidelines, quality checks must use type-aware Oxlint and Oxfmt verification.
Source: Coding guidelines
.github/workflows/ci.yml (1)
27-28: LGTM!.oxfmtrc.json (1)
10-11: LGTM!AGENTS.md (1)
15-15: LGTM!Also applies to: 41-49, 50-53, 54-66
CONTEXT.md (1)
7-35: LGTM!Also applies to: 47-47, 67-75
README.md (1)
15-21: LGTM!Also applies to: 60-61, 75-85
docs/agents/domain.md (1)
12-14: LGTM!Also applies to: 23-24
docs/agents/issue-tracker.md (1)
9-9: LGTM!Also applies to: 27-28, 37-37
docs/plans/decisions-log.md (1)
947-1055: LGTM!Also applies to: 1057-1059, 1062-1068
| if (rotation === 90 || rotation === 270 || isExifOrientationSwapped(readExifOrientation(path))) { | ||
| return { color, height: width, width: height }; | ||
| } | ||
|
|
||
| return { color, height, width }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any rotation handling in the filter chain and for orientation coverage in tests.
rg -nP 'transpose|autorotate|hflip|vflip|orientation' src tests
# Inspect the exif-orientation integration test to see which orientation value is exercised.
rg -nP -B10 -A30 'insertExifOrientation\(' tests/integration/prepare_image.integration.test.tsRepository: edhor1608/passepartout
Length of output: 3930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== image_engine outline =="
ast-grep outline src/domain/image_engine.ts --match image --view expanded || true
echo "== relevant image_engine sections =="
sed -n '200,360p' src/domain/image_engine.ts
echo "== render methods =="
rg -n "renderReadyToUploadImage|inspectSourceDimensions|computePrepareImageLayout|ffmpeg|filters|transpose|exif_orientation|hflip|vflip|scale|crop" src/domain/image_engine.ts
echo "== integration createFixture and pixel probe area =="
sed -n '1,210p' tests/integration/prepare_image.integration.test.ts
sed -n '570,610p' tests/integration/prepare_image.integration.test.tsRepository: edhor1608/passepartout
Length of output: 11699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== image_engine render/layout implementation =="
sed -n '120,175p' src/domain/image_engine.ts
sed -n '330,530p' src/domain/image_engine.ts
echo "== all ffmpeg filter usage =="
rg -n "filter|transpose|exif_orientation|format=|setsar|scale=|crop=" src/domain/image_engine.ts
echo "== deterministic behavior model for orientation mismatch =="
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Source:
probe_width: int
probe_height: int
`@dataclass`
class Layout:
inner_width: int
inner_height: int
crop_w: int
crop_h: int
crop_x: int
crop_y: int
crop_expr: str
def build_layout(probe_width, probe_height):
# mimic landscape branch for small border and enough source pixels
target_max = 1080
ratio_w = 4
ratio_h = 5
for output_width in range(target_max, ratio_w - 1, -ratio_w):
output_height = (output_width / ratio_w) * ratio_h
if output_height > target_max:
continue
inner_width = output_width
inner_height = output_height
if inner_width <= probe_width or inner_height <= probe_height:
crop_w = min(inner_width, probe_width)
crop_h = min(inner_height, probe_height)
crop_x = max(0, (probe_width - crop_w)//2)
crop_y = max(0, (probe_height - crop_h)//2)
return Layout(inner_width, inner_height, crop_w, crop_h, crop_x, crop_y, f"{crop_w}:{crop_h}:{crop_x}:{crop_y}")
return None
cases = [
("unswapped 40x60 probe/source", 40, 60),
("swapped EXIF 6: probe thinks 60x40, frame is still 40x60", 60, 40),
]
for name, sw, sh in cases:
layout = build_layout(sw, sh)
invalid_crop = layout.crop_w > sh or layout.crop_h > sw
print(f"{name}: layout crop={layout.crop_expr}, inner={layout.inner_width}x{layout.inner_height}, invalid_crop={invalid_crop}")
PY
echo "== image fixtures around created images =="
rg -n "raw-orientation|oriented|createFixture|createHorizontalPatternFixture" tests src -g '*.jpg' -g '*.jpeg' || trueRepository: edhor1608/passepartout
Length of output: 8031
Rotate EXIF swap orientations before cropping the frame.
inspectSourceDimensions returns swapped width/height for EXIF 5–8, which makes computePrepareImageLayout choose landscape layouts and crop=W:H values based on swapped frame geometry. buildPrepareImageFilter then crops the still-unrotated 0:v; orientations 6 and 7 can produce crop dimensions larger than the real frame and ffmpeg fails, while 5 and 8 crop the wrong area before scaling. Apply the matching transpose/hflip/vflip/exif_orientation filter before the crop/scale chain, or stop swapping these dimensions.
🤖 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 `@src/domain/image_engine.ts` around lines 225 - 229, Update the image
preparation flow around inspectSourceDimensions, computePrepareImageLayout, and
buildPrepareImageFilter so EXIF orientations 5–8 are normalized before cropping
and scaling, using the matching orientation transform while preserving the
existing rotation handling. Ensure crop dimensions are based on the transformed
frame geometry and avoid applying swapped dimensions to an unrotated 0:v stream.
| function getSourceColorProperties(stream: Record<string, unknown>): SourceColorProperties | null { | ||
| const space = getSupportedString(stream, "color_space", SUPPORTED_COLOR_SPACES); | ||
| const primaries = getSupportedString(stream, "color_primaries", SUPPORTED_COLOR_PRIMARIES); | ||
| const transfer = getSupportedString(stream, "color_transfer", SUPPORTED_COLOR_TRANSFERS); | ||
| const range = getSupportedString(stream, "color_range", SUPPORTED_COLOR_RANGES); | ||
|
|
||
| if (!space || !primaries || !transfer || !range) { | ||
| return null; | ||
| } | ||
|
|
||
| return { primaries, range, space, transfer }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy lift
All-or-nothing color handling silently skips conversion for partially tagged sources.
getSourceColorProperties returns null unless all four tags are present and in the allowlist. Still images frequently carry color_primaries/color_transfer while color_space is unknown (RGB PNG/TIFF) or color_range is unset — in those cases wide-gamut input is passed through untouched and exported as if it were sRGB. Consider defaulting missing components (e.g. range → pc for still images, space → gbr for RGB pix_fmts) instead of dropping the whole conversion.
🤖 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 `@src/domain/image_engine.ts` around lines 518 - 529, Update
getSourceColorProperties to preserve conversion when only some color tags are
available: retain validated primaries and transfer values, default an unset
range to “pc” for still images, and infer “gbr” for RGB pixel formats when
color_space is unknown or absent. Return null only when the required color
information cannot be safely resolved, rather than requiring all four tags to be
explicitly present.
| const proc = Bun.spawn({ | ||
| cmd: ["bun", "src/cli/prepare_image.ts", ...args], | ||
| cmd: [process.execPath, "run", "--silent", "prepare-image", ...args], | ||
| cwd: REPO_ROOT, | ||
| env: options?.env ? { ...process.env, ...options.env } : undefined, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the prepare-image script definition and any shell wrappers it relies on.
fd -H -t f 'package.json' -d 2 --exec jq '.scripts'Repository: edhor1608/passepartout
Length of output: 595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files of interest =="
git ls-files | rg '(^package\.json$|tests/helpers/cli\.ts$|src/cli/prepare_image\.ts$|package-lock\.json$|bun\.lockb$)' || true
echo
echo "== package.json scripts and deps =="
if [ -f package.json ]; then
jq '{scripts, packages, dependencies, devDependencies}' package.json
fi
echo
echo "== tests/helpers/cli.ts =="
cat -n tests/helpers/cli.ts
echo
echo "== prepare-image related test usage =="
rg -n --hidden --glob '!node_modules' --glob '!dist' 'prepare-image|PATH|process\.execPath|spawn' tests package.json . -g '!node_modules' || trueRepository: edhor1608/passepartout
Length of output: 7848
🌐 Web query:
Bun npm package scripts run command PATH lookup bun run execute bun binary PATH
💡 Result:
When you execute a command using bun run, Bun modifies the PATH for that specific process to ensure local project binaries are available [1][2]. Specifically, Bun injects the appropriate node_modules/.bin directories (including recursive lookups for nested projects) into the PATH environment variable for the duration of the script's execution [2]. The resolution order for bun run is as follows [3][4][5]: 1. package.json scripts: If a script with the specified name exists in your package.json, it is executed [3][4]. 2. Source files: If a matching file is found (e.g., index.js), Bun attempts to run it [3][4]. 3. Binaries from project packages: Bun checks for binaries located in node_modules/.bin [3][4]. 4. System commands: Finally, for bun run only, Bun falls back to checking the system PATH [3][4]. When executing these commands, Bun does not necessarily require the bun binary itself to be in the PATH to function correctly, as it manages the environment and path lookups internally [2]. However, Bun performs these actions by spawning a subshell [3][6]. On Linux and macOS, it selects from bash, sh, or zsh, while on Windows, it uses the integrated Bun Shell [3][6]. It is important to note that global binaries installed via bun add -g or bun install -g require the Bun installation directory (e.g., ~/.bun/bin) to be manually added to your system PATH for them to be executable globally from a standard shell [7]. If you encounter issues where scripts cannot find installed binaries, ensuring that the global bin directory is present in your shell's PATH is the standard solution [7].
Citations:
- 1: https://github.com/oven-sh/bun/blob/5a0b93523134ca0043ee7ae84f01003aa4b7ff69/src/cli/filter_run.zig
- 2: https://github.com/oven-sh/bun/blob/6618e7f7/src/runtime/cli/filter_run.rs
- 3: https://bun.sh/docs/runtime
- 4: Fix bun run folder oven-sh/bun#15117
- 5: https://github.com/oven-sh/bun/blob/7e57e529/test/cli/install/bun-run.test.ts
- 6: https://bun.com/docs/runtime
- 7: Fix
binnot adding to path withbun installoven-sh/bun#4962
🌐 Web query:
Bun source BunPackageScriptRunner exec PATH script commands bun run script "execve"
💡 Result:
Bun’s mechanism for executing scripts via bun run involves several layers of path resolution and process spawning, designed to prioritize package-defined scripts, local binaries, and system commands [1][2][3]. Process Spawning Mechanism At the core, Bun uses posix_spawn (and its extensions) on POSIX platforms to avoid the overhead of fork() and exec() [4]. When executing commands, Bun performs manual resolution for the command path and argv[0] before the system call to ensure security (preventing null-byte injection) and to support its custom resolution logic [4]. Path Resolution and Execution 1. Resolution Order: When bun run <name> is invoked, Bun follows a specific priority: - Package.json scripts: Defined scripts are executed first [1][2][3]. - Source files: Files (e.g., index.js) [1][2][3]. - Binaries in node_modules/.bin: Local project binaries [1][2][3]. - System commands: Resolved via the system PATH (applicable to bun run) [1][2][3]. 2. Subshell Execution: For scripts defined in package.json, Bun executes the command in a subshell [5][2]. On Linux and macOS, it sequentially checks for bash, sh, and zsh, using the first one found [5][3]. On Windows, Bun uses its internal "Bun Shell" to provide cross-platform, bash-like syntax support [5][2]. 3. ENOEXEC Handling: When an attempted execution returns ENOEXEC (commonly occurring with scripts missing a shebang), Bun mimics Node.js/libuv behavior by retrying the execution by wrapping the command in a shell (e.g., /bin/sh <script>), ensuring compatibility for non-executable script files [6]. 4. Environment Manipulation: Bun may modify the PATH environment variable during execution—for example, when using the --bun flag or during lifecycle script execution—to ensure that nested Bun processes or internal shims are correctly prioritized [7][8]. Specifically, when running scripts or utilizing shims, Bun may prepend directories (such as BUN_NODE_DIR) to PATH to ensure the correct Bun binary is invoked [7]. Key Source Code References - Process spawning logic and POSIX integration are found in src/spawn_sys/spawn_process.rs and src/bun.js/api/bun/spawn.zig [4][6][9]. - Lifecycle script management, including shell bin selection and environment setup, is handled in src/install/lifecycle_script_runner.zig [10]. - Command resolution logic for bun run is implemented in src/cli/run_command.zig [8].
Citations:
- 1: Fix bun run folder oven-sh/bun#15117
- 2: https://github.com/oven-sh/bun/blob/1498d7b7/docs/runtime/index.mdx
- 3: https://bun.com/docs/runtime
- 4: https://deepwiki.com/oven-sh/bun/5.4-subprocess-management
- 5: https://bun.sh/docs/runtime
- 6: spawn: retry via /bin/sh when exec returns ENOEXEC oven-sh/bun#31717
- 7: install: stop nested --bun from self-referencing its node shim oven-sh/bun#30713
- 8: https://github.com/oven-sh/bun/blob/b0a6feca57bf5c2a9ec2ef9773499cab7d904b30/src/cli/run_command.zig
- 9: https://github.com/oven-sh/bun/blob/7e57e529/src/bun.js/api/bun/spawn.zig
- 10: https://github.com/oven-sh/bun/blob/88417471/src/install/lifecycle_script_runner.zig
🌐 Web query:
site:github.com/oven-sh/bun filter_run.rs PATH .exec() shell subprocess bun run package script
💡 Result:
The file src/runtime/cli/filter_run.rs is a component of the Bun runtime codebase responsible for managing and executing concurrent tasks, specifically when using flags like --filter with bun run [1][2][3]. In this context,.exec (or the internal execution logic within this file) handles spawning shell subprocesses to run package scripts defined in a project's configuration [1][4]. The implementation manages process creation, environment configuration, and shell-specific commands [1][4]. Key technical details include: 1. Shell Invocation: When executing commands, it constructs an argument vector (argv) that uses the system shell to run the task [1][4]. On Unix systems, this typically involves invoking the shell with the -c flag to execute the command string, while on other platforms, it may use exec or platform-specific mechanisms [1][4]. 2. Environment Configuration: The runner ensures that the PATH environment variable is correctly configured for the subprocess [1][4]. It uses a scoped mechanism to temporarily modify the PATH for the execution of the package script and restores the original environment afterward, ensuring that local package binaries are correctly resolved [1][4]. 3. Execution Mechanism: The logic utilizes a spawn_process function that takes the configured options, the command arguments, and the environment pointers to initiate the subprocess [1][4]. This is part of a larger system that facilitates the concurrent execution and display of output for package scripts [5][3]. This code is part of the core implementation for Bun's command-line interface, specifically handling the orchestration of parallel or filtered command execution within a workspace or project [3].
Citations:
- 1: https://github.com/oven-sh/bun/blob/6618e7f7/src/runtime/cli/filter_run.rs
- 2: https://github.com/oven-sh/bun/tree/main/src/runtime/cli
- 3: Deduplicate package manager and CLI command helpers oven-sh/bun#32022
- 4: https://github.com/oven-sh/bun/blob/1498d7b7/src/runtime/cli/filter_run.rs
- 5: Cap bun --filter frame to terminal height oven-sh/bun#28802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== prepare source =="
sed -n '1,220p' src/cli/prepare_image.ts
echo
echo "== image-engine shell/fallback setup =="
sed -n '1,260p' src/domain/image_engine.tsRepository: edhor1608/passepartout
Length of output: 7873
Use absolute runtime binaries when running prepare-image in stripped-PATH tests.
prepare-image maps to bun run src/cli/prepare_image.ts, and that script later resolves ffmpeg/ffprobe before falling back to bundled paths. With PATH: dirname(process.execPath), the top-level prepare-image script cannot be invoked via bun run, and command lookups must use absolute paths such as process.execPath, the resolved ffmpeg-static path, and an existing bundled ffprobe path.
🤖 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 `@tests/helpers/cli.ts` around lines 15 - 18, The CLI test helper’s Bun
invocation must work with a stripped PATH. Update the process setup around
Bun.spawn to invoke the prepare-image entrypoint through the absolute
process.execPath and ensure the child environment supplies absolute ffmpeg and
ffprobe paths, using the resolved ffmpeg-static and existing bundled ffprobe
symbols while preserving caller-provided environment overrides.
| function hasExifOrientationTag(path: string): boolean { | ||
| return readFileSync(path).includes(Buffer.from([0x01, 0x12, 0x00, 0x03])); | ||
| } | ||
|
|
||
| function hasSrgbColorSpaceTag(path: string): boolean { | ||
| return readFileSync(path).includes( | ||
| Buffer.from([0xa0, 0x01, 0x00, 0x03, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01]), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scanning the whole JPEG for tag byte patterns can produce false positives in entropy-coded data.
Both helpers search the entire file, including compressed scan data, so an incidental byte match makes hasExifOrientationTag(...) === false on Line 621 flaky. Restrict the search to the APP1 segment (walk 0xFF 0xE1 markers from SOI and slice Exif\0\0 payload) rather than the full buffer — the file already has that walking logic in src/domain/image_engine.ts to model it on.
🤖 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 `@tests/integration/prepare_image.integration.test.ts` around lines 192 - 200,
The hasExifOrientationTag and hasSrgbColorSpaceTag helpers must stop scanning
the entire JPEG buffer, which can match compressed image data. Reuse the
APP1/Exif segment-walking approach from image_engine.ts: traverse markers from
SOI, extract the Exif payload from APP1 segments, and search the tag patterns
only within that payload.
Orientation tests only checked output dimensions, which cannot distinguish a correct rotation from a wrong one -- EXIF 5-8 all swap width and height. Adds a quadrant-marker fixture and per-corner assertions for orientations 5, 6, 7 and 8, with expectations derived from the EXIF spec rather than from an ffmpeg reference run. Verified by mutation: removing the dimension swap in inspectSourceDimensions fails all five orientation tests. Also removes the unreachable empty-path guard in planImages (callers pass an already-resolved absolute path; the CLI rejects an empty --out while parsing argv) and caps the collision-suffix search in commitWithoutOverwrite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFTLY9M1HfSALwHBMTEyPi
|
Addressed review feedback in the latest commit. EXIF orientation 5–8 crop (major, Rather than leave that on an argument, the test now proves it. Unreachable guard (major, Partial color tags (trivial, Also capped the collision-suffix search in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/prepare_image.integration.test.ts`:
- Around line 690-744: Add an explicit 60-second timeout to each generated
orientation test in the loop over ORIENTATION_CORNER_EXPECTATIONS by supplying
the timeout argument to test(). Keep the existing test body and expectations
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80ced43c-0a87-45b2-9d2e-c764f8b6f412
📒 Files selected for processing (2)
src/domain/prepare_image.tstests/integration/prepare_image.integration.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript with Bun and verify it throughbunx tsc.
Run type-aware Oxlint and Oxfmt verification as part of the quality checks.
Files:
tests/integration/prepare_image.integration.test.tssrc/domain/prepare_image.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Acceptance tests must use an explicit 60-second per-test timeout; integration tests cover the v1 command and image engine.
Files:
tests/integration/prepare_image.integration.test.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: The production command must accept PNG, JPEG, and TIFF images or directories of those images, place each image on a white canvas, and export high-quality baseline sRGB JPEGs.
Output allocation and commits must be atomic, with rollback support for failed image preparation.
Files:
tests/integration/prepare_image.integration.test.tssrc/domain/prepare_image.ts
src/domain/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep image-preparation workflow logic, including source discovery, staging, atomic output allocation, commit, and rollback, in the domain layer; keep concrete image-engine behavior in the image engine module.
Files:
src/domain/prepare_image.ts
🔇 Additional comments (2)
src/domain/prepare_image.ts (1)
33-35: LGTM!Also applies to: 87-89, 134-157
tests/integration/prepare_image.integration.test.ts (1)
56-142: LGTM!
AGENTS.md requires an explicit 60 second per-test timeout for acceptance tests, but the file capped every test at 30s. `setDefaultTimeout` overrides the runner's `--timeout` flag, so the `bun test --timeout 60000` in the test script never took effect -- verified with a minimal repro. Fixing it at the file level covers all 26 tests uniformly; adding a third `test()` argument would have applied the limit to 5 of them and left the file inconsistent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFTLY9M1HfSALwHBMTEyPi
|
Fixed, but one level up from the suggestion. The diagnosis was off: these tests never ran under Bun's 5s default. Line 16 called The finding still stands, just bigger than reported: the So instead of adding a third
|
main enabled `exactOptionalPropertyTypes` (#68) plus `typescript/strict-boolean-expressions` and `typescript/no-non-null-assertion` (#69) while this PR was open. Both sides had rewritten `src/cli/prepare_image.ts`, but not as competing implementations: main adapted the old architecture's expressions to the new rules, while this branch had already moved directory enumeration and output allocation into the domain behind `prepareImages()`. The resolution keeps this branch's thin CLI and takes only main's strict idioms. The new rules also reach code outside the conflict: - `image_engine.ts`: three truthiness checks made explicit. All are equivalent -- `getSupportedString` returns an allowlisted value or null and never "", and `sourceCrop` is an object or undefined. - `prepare_image.ts`: `?.isDirectory()` / `?.isFile()` compared against `true`, since optional chaining yields `boolean | undefined`. - `tests/helpers/cli.ts`: `env: undefined` is no longer assignable to an optional property, so the key is now omitted entirely when no override is passed. No behavior change; all 26 acceptance tests pass unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFTLY9M1HfSALwHBMTEyPi Entire-Checkpoint: 4448d79cc6d0
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/prepare_image.ts (1)
35-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsafe
--border-pxvalues.
Number.isIntegeraccepts values aboveNumber.MAX_SAFE_INTEGER, so a decimal input can be accepted as a rounded pixel value. UseNumber.isSafeInteger(parsedBorderPx)to reject values that JavaScript cannot represent exactly.Proposed fix
- if (!/^\d+$/.test(value) || !Number.isInteger(parsedBorderPx)) { + if (!/^\d+$/.test(value) || !Number.isSafeInteger(parsedBorderPx)) {🤖 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 `@src/cli/prepare_image.ts` around lines 35 - 39, Update the validation in the --border-px parsing flow to use Number.isSafeInteger(parsedBorderPx) instead of Number.isInteger, while preserving the existing digit-format check and invalid-value error behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/cli/prepare_image.ts`:
- Around line 35-39: Update the validation in the --border-px parsing flow to
use Number.isSafeInteger(parsedBorderPx) instead of Number.isInteger, while
preserving the existing digit-format check and invalid-value error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 188eafca-c1ef-4e93-ba22-fd0fee1a5c6b
📒 Files selected for processing (5)
src/cli/prepare_image.tssrc/domain/image_engine.tssrc/domain/prepare_image.tstests/helpers/cli.tstests/integration/prepare_image.integration.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain TypeScript correctness and pass typechecking withbunx tsc.
Use type-aware Oxlint-compatible TypeScript code and pass the repository lint check.
Keep formatting compliant with Oxfmt.
Image processing must support PNG, JPEG, and TIFF inputs, place images on a white canvas, and export high-quality baseline sRGB JPEGs.
Files:
tests/helpers/cli.tssrc/cli/prepare_image.tssrc/domain/image_engine.tssrc/domain/prepare_image.tstests/integration/prepare_image.integration.test.ts
src/cli/prepare_image.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep argv parsing and stdout/stderr interaction in
src/cli/prepare_image.ts; treat it as the CLI adapter.
Files:
src/cli/prepare_image.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Use explicit code and local comments rather than adding new slice Markdown for executable work.
FFmpeg and ffprobe adapters must work with preferred system binaries and independently with bundled fallback binaries when the tools are absent fromPATH.
Files:
src/cli/prepare_image.tssrc/domain/image_engine.tssrc/domain/prepare_image.ts
src/domain/image_engine.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep FFmpeg/ffprobe adapters, orientation handling, layout, white-canvas rendering, and JPEG export in
src/domain/image_engine.ts.
Files:
src/domain/image_engine.ts
src/domain/prepare_image.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep source discovery, staging, atomic output allocation, commit, and rollback in
src/domain/prepare_image.ts.
Files:
src/domain/prepare_image.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Acceptance tests must enforce the v1 command behavior, use explicit 60-second per-test timeouts, and cover the command and image-engine integration paths.
Files:
tests/integration/prepare_image.integration.test.ts
🔇 Additional comments (4)
src/domain/image_engine.ts (1)
490-504: LGTM!Also applies to: 524-528
src/domain/prepare_image.ts (1)
84-93: LGTM!tests/helpers/cli.ts (1)
15-24: LGTM!tests/integration/prepare_image.integration.test.ts (1)
16-19: LGTM!
1 participant
What changed
Why
The previous workflow spread product invariants across several shallow modules and exposed internal layout helpers as architecture boundaries. That made atomic output behavior, image-engine guarantees, documentation, and tests harder to evolve coherently. The new boundaries group behavior around the two stable responsibilities: preparing outputs and rendering images.
Impact
The public command remains unchanged: prepare-image still accepts PNG, JPEG, TIFF, or directories and writes high-quality baseline sRGB JPEGs. Internally, failure handling and concurrent output allocation are now explicit and covered end to end.
Validation
Note
Refactor prepare-image pipeline into a dedicated image engine with batch support
prepareImageAPI in prepare_image.ts withprepareImages, which supports file or directory inputs, stages outputs, and commits atomically with collision-safe suffixed filenames.prepareImages; it now prints every written output path.photo-1.jpg) rather than overwriting.Macroscope summarized e9269b7.
Summary by CodeRabbit
.jpgfiles without overwriting existing files.bun run checkand expanded integration coverage.