refactor: move executable output layout out of Ninja backend

[vivek3210]

Issue

executable_output_path() was owned by the Ninja backend even though CLI packaging, footprint reporting, and native test execution consume the generated artifact path. This couples backend-neutral behavior to one backend and makes future backend support harder.

Fixes #142

Approach

• Add ebuild.build.layout as the backend-neutral owner of executable suffix and output-path calculation.
• Import the helper directly from ebuild.build.layout in CLI consumers.
• Re-export executable_output_path from ninja_backend to preserve existing imports.
• Preserve the rationale for the Windows .exe suffix in the neutral helper documentation.
• Update tests and Windows-path monkeypatches to use the neutral owner.
• Add a regression assertion that the legacy import is an identity-preserving re-export.

Validation

• Focused pytest on the four affected test modules: 119 passed, 1 skipped on Windows CPython 3.14.
• Full pytest on the current PR head: 669 passed, 6 skipped, 9 failed on Windows CPython 3.14. All nine failures are pre-existing tests/unit/test_index_sync.py failures caused by missing PackageRecipe.to_dict.
• Independent Linux review run: 670 passed, 4 skipped, 10 failed, including the platform-sensitive build-directory failure; this is recorded separately in TASKS.md.
• git diff --check: passed.
• Editor diagnostics: no errors in changed files.

Considerations

The full-suite failures are outside this change and were reproduced independently of the layout refactor. The compatibility export remains available from ebuild.build.ninja_backend; new production consumers use ebuild.build.layout. Ruff was not available in the active interpreter, so the repository Ruff command could not be executed locally.

[Copilot]

🟡 Changes recommended

Update CLI imports to use the neutral module and reconcile the inconsistent full-suite validation record.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Moves executable output-path calculation into a backend-neutral layout module while preserving Ninja compatibility.

Changes:

• Adds ebuild.build.layout.executable_output_path.
• Retains the Ninja re-export and updates related tests.
• Records validation evidence in TASKS.md.

File summaries

File	Summary
tests/unit/test_package_efw.py	Uses the neutral path helper.
tests/unit/test_ninja_backend.py	Verifies the compatibility re-export.
tests/unit/test_golden_path_commands.py	Updates Windows suffix patching.
tests/unit/test_footprint.py	Updates Windows suffix patching.
TASKS.md	Records validation evidence requiring reconciliation.
ebuild/build/ninja_backend.py	Preserves the legacy export; CLI coupling remains.
ebuild/build/layout.py	Adds the backend-neutral path helper.

Review details

Suppressed comments (1)

TASKS.md:56

• The validation record is inconsistent: the adjacent full-suite entry reports 560 passed, 6 skipped, exit code 0, while the PR description reports 669 passed, 6 skipped, and 9 pre-existing failures in tests/unit/test_index_sync.py. Please reconcile the task evidence with the run that corresponds to this branch so the review does not rely on a misleading full-suite result.

  and `tests/unit/test_ninja_backend.py`) reports **119 passed, 1 skipped**;
  the compatibility test asserts both import paths are identical and the
  Windows suffix path is exercised.

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[srpatcha]

Review — ebuild#141 "refactor: move executable output layout out of Ninja backend"

head: 300f26e author: vivek3210 ci: fail

Verdict: A clean, correctly-scoped move of executable_output_path() / _exe_suffix() from ebuild/build/ninja_backend.py into a new backend-neutral ebuild/build/layout.py, re-exported for compatibility. The code change is sound and introduces no test regressions — I ran the suite at this head and at master (76970c97) and the failure set is byte-identical. The blocking problems are not in the diff: one required check fails on a missing Fixes #N, and the whole test matrix is red from lint drift that already exists on master.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High (P1)	PR body	Required check policy / Policy / Linked Issue fails: "Pull request #141 must close at least one same-repository issue; no closing issues were recognized." This one is yours to fix and blocks merge on its own.	Add Fixes #<n> (or Closes/Resolves) to the PR body, referencing the same-repo issue for T-005. If no issue exists, open one.
2	High (P1)	n/a — pre-existing on master	All 10 Test (…) jobs and the CI Gate aggregate fail. Not caused by this PR. Root cause is 4 ruff errors in files this diff does not touch: tests/ebuild/test_build_dir_resolution.py:31 (F811 duplicate import shutil), tests/unit/test_package_recipe.py:117 (W292 no trailing newline), tests/unit/test_ci_gate.py:214,215 (E402). Verified all four present at master 76970c97; verified ruff check over the six files this PR changes reports All checks passed.	Nothing to change here. Already addressed by open PRs #122 ("ci: fix the lint findings that stop CI before any test runs") and #132. Rebase once one of those lands. No duplicate fix PR opened for this reason.
3	Medium (P2)	ebuild/cli/commands.py:29-33	The refactor is incomplete for its own stated purpose. The PR body's motivation is that "CLI packaging, footprint reporting, and test execution consume the generated artifact path", yet commands.py — the sole production consumer, and the one that is not the Ninja backend — still does from ebuild.build.ninja_backend import (… executable_output_path …). Only tests were repointed. The TASKS.md evidence added at line 20 states "consumers/tests use the backend-neutral owner"; that is true of tests and false of the consumer. Behaviour is unaffected (I verified commands.executable_output_path is layout.executable_output_path), so this is coupling, not a bug.	Move executable_output_path out of the ninja_backend import tuple in commands.py and add from ebuild.build.layout import executable_output_path. Then correct the TASKS.md wording, or narrow it to "tests".
4	Medium (P2)	TASKS.md:26-28 (context, now contradicted)	TASKS.md carries two suite claims that cannot both hold. The new T-005 block is inserted directly above the pre-existing line "Suite result … 560 passed, 6 skipped, exit code 0. Supersedes any other count quoted for T-003 or T-004 elsewhere in this repo", while this PR's own body reports the full suite as "669 passed, 6 skipped, 9 failed". A reader of TASKS.md alone is told the suite is green. It is not. Separately the body undercounts: I measured 10 failed, 670 passed, 4 skipped at this head — 9 in tests/unit/test_index_sync.py ('PackageRecipe' object has no attribute 'to_dict') plus tests/ebuild/test_build_dir_resolution.py::test_end_to_end_build_from_outside_produces_the_binary. All 10 reproduce identically on master, so "pre-existing and unrelated" is confirmed — the count is what is wrong, not the attribution.	Update the TASKS.md "Suite result" line so it no longer asserts exit code 0, and state the known-failing set by name. Correct "9 failed" to 10 in the body, or state the platform and selection the 9 was measured under.
5	Low (P3)	ebuild/build/layout.py:12-33	The move dropped the only record of why this function exists. The removed ninja_backend docstrings explained that "gcc on Windows appends .exe when -o names no extension, so an edge declaring app produced app.exe on disk: ninja never saw its own output, treated the target as dirty and relinked on every build", and warned that "a consumer that rebuilds this path independently instead of calling this function can silently drop the suffix and go looking for a binary the edge never produced". The replacement docstring says only "including .exe on Windows". That rationale is what stops the next contributor re-inlining build_dir / name, and T-003/T-004 in TASKS.md are that exact bug.	Carry the two rationale paragraphs across into layout.py, rewritten to be backend-neutral ("the compiler driver", not "the Ninja edge").
6	Low (P3)	PR body	Evidence quality. (a) The body claims flake8 ebuild/build/layout.py: passed — the repo's lint gate is ruff over the whole tree, and a single-file flake8 run does not exercise it. I ran the real gate on the changed files and it passes, so there is no defect behind this, only a claim that does not match the check it implies. (b) Three words lost their leading character — inja_backend, ests/unit/test_index_sync.py, lake8 — and ## ConsiderationsThe is missing a line break. The body is the durable record of this change.	Re-run ruff check . and quote that, and repair the four text defects.

Architecture conformance

Conforms. Master design §21 places ebuild in Tier 1 – Foundation; the change is entirely intra-repo and adds no cross-repo dependency, so §21.1's split policy is not engaged — no new repository is created for a named subsystem, which is the outcome that policy wants.

§5.1 holds. The new edge is ebuild/build/ninja_backend.py:17 → ebuild/build/layout.py, i.e. a backend-specific module depending on a backend-neutral one. Per .ai/architect.md that is downward and inward, the direction required. layout.py imports only sys and pathlib, so it depends on nothing in the repo and cannot create a cycle — verified by importing both modules in isolation. Nothing in the diff points up a tier, and §5.1's "eBuild … is not a runtime dependency" is untouched: this is build-host code only.

§9.2's first SDK rule — "one source of truth for CLI, VS Code and EoStudio" — is the rule this PR serves, and it moves toward it. Finding 3 is the reason it does not arrive: the single source of truth now exists, but the CLI still reaches it through the Ninja backend rather than through it directly.

.ai/architect.md "do not restructure and change behaviour in the same commit" is respected. The diff is a pure move plus re-export; I confirmed the moved function is byte-equivalent in behaviour and that ninja_backend retains no dangling reference to the removed _exe_suffix (no occurrence remains anywhere outside layout.py and three test monkeypatches).

Proposed changes

Smallest sequence that keeps everything building:

1. Add Fixes #<n> to the PR body. This clears finding 1 and is the only required check in your control.
2. In ebuild/cli/commands.py, drop executable_output_path, from the ninja_backend import tuple at line 29 and add from ebuild.build.layout import executable_output_path. One line moved; commands.py then has no reason to know a Ninja backend exists for path purposes. Re-run tests/unit/test_golden_path_commands.py tests/unit/test_footprint.py tests/unit/test_package_efw.py.
3. Keep the ninja_backend re-export — test_executable_path_is_reexported_for_backend_compatibility is a good test and the export is still legitimately used by _write_ninja at ninja_backend.py:224.
4. Move the rationale paragraphs into layout.py's docstring (finding 5).
5. Fix the TASKS.md "Suite result" line so it no longer claims exit code 0 (finding 4).
6. Rebase after #122 or #132 lands. Until then the matrix stays red for reasons outside this diff.

Not checked

• Windows and macOS behaviour. Everything below ran on Linux, CPython 3.12.14. The .exe path is exercised only through monkeypatch.setattr(layout, "_exe_suffix", …); no real Windows link was performed by me. The author's "119 passed, 1 skipped" was measured on a Windows host — I measured 118 passed, 2 skipped on Linux for the same four files, which is consistent with one POSIX/Windows skip swapping places, but I did not reproduce their host.
• End-to-end build. tests/ebuild/test_build_dir_resolution.py::test_end_to_end_build_from_outside_produces_the_binary fails in my environment at both master and this head; I did not determine whether it needs a real compiler/ninja toolchain or is genuinely broken. Either way it is not this PR's.
• The repo's actual CI workflow was not executed locally. I read the failing job logs from run 34849845797 and reproduced the ruff result independently; I did not reproduce the matrix.
• Coverage of the moved function was not measured, and I did not audit the other three open PRs (#119, #124, #132) for overlap with this diff beyond confirming they target different defects.

---

Automated architecture review of 300f26ea222a — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[vivek3210]

@copilot review

[Copilot]

🟡 Changes recommended

Remove the unintended full-file line-ending churn in commands.py.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[srpatcha]

Review — ebuild#141 "refactor: move executable output layout out of Ninja backend"

head: 8392522 author: vivek3210 ci: fail

Verdict: Every finding from the previous review that was yours to fix is resolved —
the linked issue, the CLI import, the TASKS.md contradiction, the lost rationale and
the PR-body evidence, all verified below. One new problem arrived with them: the merge
commit 59138be rewrote all 2930 lines of ebuild/cli/commands.py from CRLF to LF. That
change is in neither of its parents, is not mentioned anywhere in the PR, and I verified
it turns a clean auto-merge with open PR #135 into a conflict.

Follow-up on the previous review

Prior #	Sev	Status	Evidence at this head
1	High	Resolved in the body edit	Body now carries Fixes #142 (issue #142 open, same repo). policy / Policy / Linked Issue → pass (run 34878719024).
2	High	Still open, still not yours	All 8 completed Test (…) jobs fail on the same 4 pre-existing ruff errors, unchanged: tests/ebuild/test_build_dir_resolution.py:31 F811, tests/unit/test_package_recipe.py:117 W292, tests/unit/test_ci_gate.py:214,215 E402. Read from the ubuntu-22.04 / 3.10 job log (Found 4 errors, exit 1) and reproduced locally: ruff check . at this head → the same 4, all in files this diff does not touch; ruff check over the 8 changed files → All checks passed!. PRs #122 and #132 both still open and both address it. Nothing to change here; no duplicate fix PR opened.
3	Medium	Resolved in 0558e0c	commands.py:29 is now from ebuild.build.layout import executable_output_path, removed from the ninja_backend tuple at :30-33. Verified no production module still reaches the symbol through the backend: the only remaining ninja_backend import of it is tests/unit/test_ninja_backend.py:28, which is the compatibility test and is meant to.
4	Medium	Resolved in 8392522	TASKS.md no longer claims exit code 0. It now records the 560 passed line as superseded, states 669/6/9 for the Windows host and 670/4/10 for the Linux review run, and names PackageRecipe.to_dict as the cause. Both counts match what I measure.
5	Low	Resolved in 8392522	The rationale is back and correctly made backend-neutral: layout.py:15-17 ("Compiler drivers on Windows append .exe …") and :25-27 ("Consumers must use this helper rather than rebuilding build_dir / target_name …"). python -m doctest ebuild/build/layout.py → 2 passed.
6	Low	Resolved in the body edit	The flake8 claim is gone, replaced by "Ruff was not available in the active interpreter, so the repository Ruff command could not be executed locally" — which is the honest statement. The four mojibake defects (inja_backend, ests/unit/…, lake8, ## ConsiderationsThe) are all repaired.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High (P1)	commit 59138be, ebuild/cli/commands.py (whole file)	The merge commit silently converted the entire file's line endings, and it breaks another open PR. 59138be has parents 0558e0c and 300f26ea; ebuild/cli/commands.py has all 2930 lines CRLF-terminated in both parents and 0 in the merge result. A merge commit is not allowed to introduce content that is in neither side — this one rewrote 2930 lines. Consequences, both verified: (a) git merge-tree 300f26ea <pr135-head> auto-merges commands.py cleanly, git merge-tree 8392522e <pr135-head> reports CONFLICT (content): Merge conflict in ebuild/cli/commands.py, so landing this head forces PR #135 through a 2930-line conflict for a change it never made; (b) the one real edit in commands.py — moving executable_output_path between two import statements — is now invisible in the GitHub diff behind 2930 lines of churn, and git blame for the whole file points at this PR. Nothing in the body, the commit message or TASKS.md mentions it.	Restore the file's line endings and redo the merge without them: git checkout 300f26ea -- ebuild/cli/commands.py, reapply the two-line import move, amend. Confirm with git diff --stat origin/master...HEAD -- ebuild/cli/commands.py → 2 +-, and git show <merge> --stat naming no file neither parent changed. Do not normalise the repo's line endings as part of this PR: only 7 of 186 .py files on master are CRLF and the repo has no .gitattributes, so that is a real cleanup — but it is its own PR with its own .gitattributes, not a side effect of a merge resolution.
2	Low (P3)	commit 59138be	The merge subject is Merge remote-tracking branch 'fork/fix/backend-neutral-executable-layout' into fix/backend-neutral-executable-layout — a self-merge of the same branch name from two remotes, and the second such merge on this branch after 300f26e. It is not a Conventional Commits subject (STANDARDS.md), and it is the commit that carried finding 1. Rebasing instead of merging avoids both.	Rebase the branch onto origin/master and drop both merge commits. The PR then contains the two refactor: / docs: commits it is actually about.

Nothing else in the new commits is a defect. 0558e0c is exactly the change the previous
review's step 2 asked for, and 8392522 is a documentation-only commit whose numbers I
independently reproduced.

Architecture conformance

Conforms, and the one place it fell short last time is now closed.

§21 places ebuild in Tier 1 — Foundation. The change is entirely intra-repo and adds
no cross-repo dependency, so §21.1's split policy is not engaged — no repository was
created to give a subsystem a name, which is the outcome that policy wants.

§5.1 holds and is now cleaner than at the last head. The dependency edges are
ebuild/build/ninja_backend.py:17 → ebuild/build/layout.py and
ebuild/cli/commands.py:29 → ebuild/build/layout.py: a backend-specific module and a CLI
consumer both depending on a backend-neutral one. Per .ai/architect.md that is downward
and inward, the required direction. layout.py imports only sys and pathlib, so it
depends on nothing in the repo and cannot create a cycle. The previously-flagged edge —
the CLI reaching a neutral helper through the Ninja backend — is gone; verified
ebuild.cli.commands.executable_output_path.__module__ == 'ebuild.build.layout' and
ninja_backend.executable_output_path is layout.executable_output_path.

§9.2's "one source of truth for CLI, VS Code and EoStudio" is the rule this PR serves and
it now arrives at it: there is one owner of the executable-output path, and every
production consumer imports it from there. .ai/tooling.md's "a fix belongs in the SDK,
not in the GUI's copy of the logic" is the same rule one tier up, and this is the
SDK-side precondition for it.

.ai/architect.md's "do not restructure and change behaviour in the same commit" is
respected by the code commits. Finding 1 is the exception and it is why that rule exists:
59138be restructured 2930 lines of a file whose behaviour the PR does not change.

The design gap this refactor exposed — §9.1 names no build backends, so the boundary
between backend-owned and backend-neutral is decided one refactor at a time — was already
appended to .ai/autoreview/proposals/2026-09.md on 2026-09-14. No new proposal:
finding 1 is repository hygiene (a missing .gitattributes), below the level the master
design governs.

Proposed changes

1. Undo finding 1 and rebase (findings 1 and 2 are the same repair). Re-run the focused
   four modules afterwards.
2. Nothing else in this PR. Wait for #122 or #132 to land, then rebase; the matrix stays
   red until one of them does, for reasons outside this diff.

Not checked

• Windows and macOS behaviour — NOT RUN. Everything below ran on Linux, CPython
  3.12.3. The .exe path is exercised only through monkeypatch.setattr(layout, "_exe_suffix", …); no real Windows link was performed. Test (Python 3.11, windows-2022) was still pending when this bundle was captured, so 8 of 9 completed
  jobs are the basis for "all fail", not 9 of 9.
• The repo's CI workflow — NOT RUN locally. I read the ubuntu-22.04 / 3.10 job log
  from run 34878719818 and reproduced its ruff result independently; I did not reproduce
  the matrix.
• tests/ebuild/test_build_dir_resolution.py::test_end_to_end_build_from_outside_produces_the_binary
  fails for me at both master and this head. I still have not determined whether it
  needs a real compiler/ninja toolchain. Either way it is not this PR's.
• The line-ending change's effect on Windows checkouts was not tested. Finding 1 is
  argued from the merge-conflict result and the commit's own parents, not from a Windows
  clone.
• Coverage of the moved function was not measured, and I did not audit #119, #122,
  #124 or #132 for diff overlap beyond confirming #135 is the only other open PR touching
  ebuild/cli/commands.py.

Verified on this run (Linux, CPython 3.12.3, isolated venv: pytest 9.1.1, click,
pyyaml; ruff 0.16.5; all against git archive exports, no working tree touched):

Check	Result
ruff check . @ 8392522e	4 errors, all in untouched files (finding 2, pre-existing)
ruff check <8 changed files>	All checks passed!
pytest tests/ -q @ 8392522e	10 failed, 670 passed, 4 skipped
pytest tests/ -q @ master 76970c97	10 failed, 667 passed, 4 skipped — identical failure set
pytest <4 focused modules> -q @ 8392522e	118 passed, 2 skipped
python -m doctest ebuild/build/layout.py	2 passed
import identity	ninja_backend.executable_output_path is layout.executable_output_path → True; commands.executable_output_path.__module__ → ebuild.build.layout
git diff --check origin/master 8392522e	clean
CRLF census, commands.py	0558e0c: 2930 · 300f26ea: 2930 · 59138be: 0 · 8392522e: 0
git merge-tree 300f26ea <pr135>	conflict in TASKS.md only; commands.py auto-merged
git merge-tree 8392522e <pr135>	conflict in TASKS.md and ebuild/cli/commands.py

No test regression: the failure set at this head is identical to master, name for name.

Blocked / stale

Not stale — three commits since the last review, all responsive. Blocked on CI, on
two counts: the Test (…) matrix is red from master's lint drift (finding 2, waiting on
#122 or #132), and Test (Python 3.11, windows-2022) had not reported. reviewDecision: REVIEW_REQUIRED, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, 5 ahead / 0 behind
master.

What unblocks it: finding 1, then someone landing #122 or #132.

---

Automated architecture review of 8392522ee235 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[Kartikey1306]

All ten red checks here are inherited from master, not caused by this PR —
none of the four lint findings are in files this PR touches.

Each Test (…) matrix leg runs ruff check before pytest, and master
(76970c9) fails lint with 4 findings, so no leg reaches the tests:

tests/ebuild/test_build_dir_resolution.py:31:8: F811 Redefinition of unused `shutil`
tests/ebuild/test_package_recipe.py:117:41: W292 No newline at end of file
tests/unit/test_ci_gate.py:214:1: E402 Module level import not at top of file
tests/unit/test_ci_gate.py:215:1: E402 Module level import not at top of file

CI Gate then correctly reports the nine failures. #132 (fix: repair master — lint, a dropped method, vendored drift, …) fixes all four; it is open, green and
on top of current master.

Verified by building the merge result locally rather than inferring it:

master alone	ruff check . → Found 4 errors
this PR merged with #132	ruff check . → All checks passed; pytest → 686 passed, 1 skipped (your three new tests included) — no conflicts

Nothing is needed here. Once #132 lands, a rebase or merge of master clears
all ten.

[Kartikey1306]

Update: the lint fix landed on master via #122 (c37e995, 2026-09-15 18:22Z) rather than #132, so this no longer waits on anything. I re-checked: ruff check . on c37e995 → All checks passed. A rebase or merge of master clears all ten checks here; the earlier diagnosis of the cause stands, only the fix's PR number changed.

[Kartikey1306]

Correction to my previous comment — "this no longer waits on anything" was wrong, and the same thing will happen on a rebase here: ruff now passes, so every matrix leg reaches the next step and fails there instead:

Type check (mypy)
ebuild/packages/index_sync.py:354: error: "PackageRecipe" has no attribute "to_dict"  [attr-defined]

That is master's second defect: index_sync.py:354 calls recipe.to_dict() and PackageRecipe on c37e995 has no such method (grep -c 'def to_dict' ebuild/packages/recipe.py → 0). #122 fixed only the lint half; the missing method is restored by #132 (fix: repair master — lint, a dropped method, vendored drift…), and independently by the contributor PRs #119 / #124 that add PackageRecipe.to_dict().

Verified with CI's exact invocation (mypy . --ignore-missing-imports --no-strict-optional --exclude '^(layers|core|promo)/'):

master + this PR (rebased)	Found 1 error in 1 file (the line above)
this head merged with #132	Success: no issues found in 107 source files; no conflicts

So the earlier diagnosis stood — this PR waits on #132 (or #119/#124) landing, then a rebase — and my "update" was the part that was wrong. Sorry for the noise.