Repository navigation
EDH-328 add Passepartout fixtures - #1
Conversation
EDH-328 Passepartout: Add planner fixture harness
Sourcehttps://linear.app/edhorsagents/document/prd-zero-sense-multi-experiment-bench-edf4de601568 Branch: Commit: What to buildTurn the Passepartout layout planner experiment into a fixture-driven planning slice. The experiment should verify landscape, portrait, square, and border cases without invoking ffmpeg, EXIF, pixel processing, or the target repo build. Acceptance criteria
Verification
TDD noteA native inline Zero test attempt was discovered by PR statusImplementation is complete locally, but Graphite PR submission is blocked by EDH-335 because this checkout has no Git remote configured and Graphite cannot infer the GitHub owner/name. |
|
Warning Review limit reached
Your plan currently allows 4 reviews/hour. Refill in 11 minutes and 49 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds pure dimension helpers and four exported C fixture functions (landscape_fixture, portrait_fixture, square_fixture, border_fixture) plus an exported C main() that returns success when all fixtures pass. Adds a Node ESM integration test that runs zero graph/check/build, verifies fixture signatures and checker success, builds WASM, instantiates it, and asserts exported fixture functions and main() return 1. Adds a shell wrapper to run the Node test and wires it into scripts/test.sh. Updates README and branch notes to document the fixture cases and the harness validation flow. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Linear Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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: e4ea58000a
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/passepartout-planner-fixtures.test.mjs`:
- Around line 17-28: The test currently only checks return types via
functionsByName and that type-checking (check/zeroJson) passed; add a value
evaluation step that actually runs each fixture ("landscape_fixture",
"portrait_fixture", "square_fixture", "border_fixture") and asserts the returned
value is true. Locate where functionsByName and zeroJson are used, use the
project's test runner/eval API (or invoke the function via the runner/eval mode
or generate a probe) to execute each fixture by name (e.g., via the same
mechanism that produces check results) and assert that the result.value === true
(or equivalent success flag) for each fixture instead of relying solely on
returnType/type-check assertions.
- Line 7: The test currently falls back to a machine-specific hardcoded path via
the zeroBin constant; remove that hardcoded fallback and instead either (a)
require a ZERO_BIN env var and throw a clear, descriptive error when it's unset,
or (b) compute a repo-relative binary path (e.g., derive the repo root from
import.meta.url or process.cwd() and resolve a ./bin/zero path) so the test can
run on other machines; update any references to zeroBin accordingly and ensure
error messages mention ZERO_BIN so failures are explicit.
🪄 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
Run ID: aa67d072-dc32-47bc-9a6a-c8c6e2218f73
📒 Files selected for processing (6)
docs/branch-knowledge.mdexperiments/passepartout-layout-planner/README.mdexperiments/passepartout-layout-planner/src/main.0scripts/test-passepartout-planner-fixtures.shscripts/test.shtests/passepartout-planner-fixtures.test.mjs
📜 Review details
🔇 Additional comments (5)
experiments/passepartout-layout-planner/src/main.0 (1)
58-89: LGTM!experiments/passepartout-layout-planner/README.md (1)
9-17: LGTM!scripts/test-passepartout-planner-fixtures.sh (1)
1-6: LGTM!scripts/test.sh (1)
9-10: LGTM!docs/branch-knowledge.md (1)
16-20: LGTM!Also applies to: 29-30, 36-36
e4ea580 to
fab5a58
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/passepartout-planner-fixtures.test.mjs (1)
18-49:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFixture outcome checks are still disconnected from Zero fixture execution.
The value assertions are computed from a JS reimplementation (
fixturePasses) rather than executing fixture functions from the Zero target, so fixture regressions inmain.0can go undetected.Also applies to: 54-60
🤖 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/passepartout-planner-fixtures.test.mjs` around lines 18 - 49, The test currently computes expected fixture outcomes with local JS helpers (selectExportSpec, contentBox) inside fixturePasses instead of invoking the actual Zero-target fixture functions; change the test to import and call the exported functions from the compiled Zero module (the main.0 exports used by the test) and assert on their real return values rather than the local reimplementation — update fixturePasses to call the Zero exports for "landscape_fixture", "portrait_fixture", "square_fixture" and "border_fixture" (and for the border case call the Zero contentBox/export-spec function chain) so the assertions validate the actual Zero outputs rather than the JS duplicates.
🤖 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 `@scripts/test-passepartout-planner-fixtures.sh`:
- Line 7: The test invocation uses a cwd-dependent path ("node
tests/passepartout-planner-fixtures.test.mjs"); change it to invoke Node with a
script-relative path by computing the script directory and calling Node with the
test file located relative to that directory (so the command in
scripts/test-passepartout-planner-fixtures.sh uses the script's dir to build the
path to tests/passepartout-planner-fixtures.test.mjs instead of a plain
"tests/..." relative path).
In `@scripts/test.sh`:
- Line 10: Replace the cwd-dependent invocation of the fixture wrapper in
scripts/test.sh ("sh scripts/test-passepartout-planner-fixtures.sh") with a
script-relative call that resolves the directory of scripts/test.sh and invokes
test-passepartout-planner-fixtures.sh from that directory; locate the line
invoking test-passepartout-planner-fixtures.sh and change it to compute the
script's directory (via the script's $0/dirname equivalent) and call the fixture
wrapper from that resolved path so the script works regardless of the current
working directory.
In `@tests/passepartout-planner-fixtures.test.mjs`:
- Around line 14-16: The zeroJson helper currently calls execFileSync(zeroBin,
args, { cwd: repoRoot, encoding: "utf8" }) with no timeout, so add a timeout
option to that call to prevent CI hangs; update the options object passed to
execFileSync inside function zeroJson to include a sensible timeout (e.g.,
30_000 ms or use a shared constant) so the subprocess will be killed if it
exceeds the limit, and ensure any tests expecting longer runs are adjusted
accordingly (refer to zeroJson and zeroBin to locate the call).
---
Duplicate comments:
In `@tests/passepartout-planner-fixtures.test.mjs`:
- Around line 18-49: The test currently computes expected fixture outcomes with
local JS helpers (selectExportSpec, contentBox) inside fixturePasses instead of
invoking the actual Zero-target fixture functions; change the test to import and
call the exported functions from the compiled Zero module (the main.0 exports
used by the test) and assert on their real return values rather than the local
reimplementation — update fixturePasses to call the Zero exports for
"landscape_fixture", "portrait_fixture", "square_fixture" and "border_fixture"
(and for the border case call the Zero contentBox/export-spec function chain) so
the assertions validate the actual Zero outputs rather than the JS duplicates.
🪄 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
Run ID: 0d3f8d03-d325-4f11-a6a8-22e8d17b4fb4
📒 Files selected for processing (6)
docs/branch-knowledge.mdexperiments/passepartout-layout-planner/README.mdexperiments/passepartout-layout-planner/src/main.0scripts/test-passepartout-planner-fixtures.shscripts/test.shtests/passepartout-planner-fixtures.test.mjs
📜 Review details
🔇 Additional comments (3)
experiments/passepartout-layout-planner/src/main.0 (1)
58-65: LGTM!Also applies to: 67-75, 76-84, 85-89, 91-95
experiments/passepartout-layout-planner/README.md (1)
9-17: LGTM!docs/branch-knowledge.md (1)
16-20: LGTM!Also applies to: 29-31, 36-37
fab5a58 to
5ed2479
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/passepartout-planner-fixtures.test.mjs (1)
41-56:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHarness still does not assert fixture outcomes (
true) at runtime.This only checks function signatures and checker health; it can pass even if fixture logic regresses and returns
false. Add a value-level execution/probe step forlandscape_fixture,portrait_fixture,square_fixture, andborder_fixture.🤖 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/passepartout-planner-fixtures.test.mjs` around lines 41 - 56, The test currently only checks signatures and build diagnostics; add a runtime execution/probe after building the graph to ensure the fixtures return true by actually invoking each fixture function: use the project's runtime invocation helper (e.g., zeroRun/zeroInvoke or equivalent) to call "landscape_fixture", "portrait_fixture", "square_fixture", and "border_fixture" against the same target and assert each invocation returns a truthy/true value; place these assertions after you construct graph/functionsByName and before the final build assertions so regressions in fixture logic fail the test.
🤖 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/passepartout-planner-fixtures.test.mjs`:
- Around line 36-38: The catch block in zeroBuildResult currently assumes
error.stdout exists and does JSON.parse(error.stdout.toString()), which can
throw if stdout is undefined; update the zeroBuildResult catch handler to first
check that error && error.stdout is defined (or use optional chaining like
error?.stdout), only call toString() and JSON.parse when present, and otherwise
rethrow the original error (or throw a new Error containing the original
error.message) so the original spawn/timeout failure isn't masked.
---
Duplicate comments:
In `@tests/passepartout-planner-fixtures.test.mjs`:
- Around line 41-56: The test currently only checks signatures and build
diagnostics; add a runtime execution/probe after building the graph to ensure
the fixtures return true by actually invoking each fixture function: use the
project's runtime invocation helper (e.g., zeroRun/zeroInvoke or equivalent) to
call "landscape_fixture", "portrait_fixture", "square_fixture", and
"border_fixture" against the same target and assert each invocation returns a
truthy/true value; place these assertions after you construct
graph/functionsByName and before the final build assertions so regressions in
fixture logic fail the test.
🪄 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
Run ID: 42f4efdd-852b-46eb-9647-a64e29f20035
📒 Files selected for processing (6)
docs/branch-knowledge.mdexperiments/passepartout-layout-planner/README.mdexperiments/passepartout-layout-planner/src/main.0scripts/test-passepartout-planner-fixtures.shscripts/test.shtests/passepartout-planner-fixtures.test.mjs
📜 Review details
🔇 Additional comments (5)
experiments/passepartout-layout-planner/src/main.0 (1)
58-95: LGTM!experiments/passepartout-layout-planner/README.md (1)
9-19: LGTM!scripts/test-passepartout-planner-fixtures.sh (1)
1-10: LGTM!scripts/test.sh (1)
4-6: LGTM!Also applies to: 12-12
docs/branch-knowledge.md (1)
16-20: LGTM!Also applies to: 29-31, 37-38
5ed2479 to
a7d38e3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 `@experiments/passepartout-layout-planner/src/main.0`:
- Around line 33-38: The content_max_w and content_max_h functions can return
negative values when border_px is too large or negative; update both functions
to first validate border_px is non-negative (reject or panic/return an error
when border_px < 0) and then compute inner = max_w(input_w,input_h) - (border_px
* 2) (and analogous for max_h) and clamp the result to a minimum of 0 (e.g.,
return max(0, inner)); apply the same change to both content_max_w and
content_max_h to ensure no negative content dimensions propagate.
🪄 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
Run ID: 7dfe3b4d-fe3b-46a3-9859-ed193add859a
📒 Files selected for processing (6)
docs/branch-knowledge.mdexperiments/passepartout-layout-planner/README.mdexperiments/passepartout-layout-planner/src/main.0scripts/test-passepartout-planner-fixtures.shscripts/test.shtests/passepartout-planner-fixtures.test.mjs
📜 Review details
🧰 Additional context used
🪛 Shellcheck (0.11.0)
scripts/test-passepartout-planner-fixtures.sh
[warning] 4-4: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 5-5: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
scripts/test.sh
[warning] 4-4: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
🔇 Additional comments (6)
experiments/passepartout-layout-planner/src/main.0 (1)
1-31: LGTM!Also applies to: 41-92
scripts/test-passepartout-planner-fixtures.sh (1)
1-10: LGTM!scripts/test.sh (1)
4-6: LGTM!Also applies to: 12-12
tests/passepartout-planner-fixtures.test.mjs (1)
1-60: LGTM!docs/branch-knowledge.md (1)
16-20: LGTM!Also applies to: 29-31, 37-37
experiments/passepartout-layout-planner/README.md (1)
3-3: LGTM!Also applies to: 9-17
a7d38e3 to
eb22b69
Compare
Dismissed after all actionable threads were addressed and resolved; checks are green.
eb22b69 to
67229f9
Compare
67229f9 to
6ee3c59
Compare
Merge activity
|
1 participant
Summary by CodeRabbit
Tests
Documentation