Fix default branch resolution for Git clones - #123
MichaelVasco wants to merge 26 commits into
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#123 "Fix default branch resolution for Git clones"
head: 322066f author: MichaelVasco ci: none reported
Verdict: The underlying bug is real and worth fixing, but this head does not compile. ebuild/deps/manager.py contains a corrupted edit that leaves invalid Python at line 156, so the ebuild.deps package cannot be imported at all and the two tests the PR adds cannot run. Separately, even with the syntax repaired the fix would not take effect: setup() still computes effective_branch = ... or "master", so _git_clone never receives the None the new code path exists to handle.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Critical | ebuild/deps/manager.py:156-158 |
Syntax error — the module does not parse. A copy of the "Clone to cache" comment was pasted with its # stripped, followed by two de-indented duplicates of the lines above it. python3 -m py_compile ebuild/deps/manager.py → SyntaxError: invalid syntax at line 156. ruff check reports the same plus Unexpected indentation at line 160. ebuild.deps.manager is unimportable, so anything reaching DepsManager — including tests/unit/test_deps_manager.py, added by this PR — fails at import. |
Revert lines 156-158 to the single original pair and keep only the intended change: effective_branch = repo_cfg.get("branch") (no or "master"). |
| 2 | Critical | ebuild/deps/manager.py:359-364 |
Second syntax error, same cause. Inside _git_clone the new if branch: body is indented 4 spaces against a method body at 8, and the following cmd.extend([url, str(dest)]) is at column 0, outside the method. |
Re-indent both statements to the method's 8-space body level. |
| 3 | High | ebuild/deps/manager.py:156 |
The fix is unreachable from the only caller. _git_clone's signature widens to Optional[str] and the body learns to omit --branch, but the mangled block still ends in or "master", so setup() passes the literal "master" on every path. With the syntax errors fixed and nothing else changed, behaviour is identical to master and the PR's stated purpose is not achieved. |
Drop the or "master" fallback so effective_branch is None when the config names no branch, letting git clone take the remote's own default branch (HEAD). That is the actual "default branch resolution". |
| 4 | High | ebuild/deps/manager.py:161, :368 |
The already-cloned path is untouched and still hardcodes master. setup() calls self._checkout_branch(dest, effective_branch) when dest.exists(), and _checkout_branch runs git checkout <branch> unconditionally. On a repo whose default branch is main, a cached clone gets git checkout master → fails. The failure is also silent: the subprocess.run at :369-377 captures output and never inspects returncode, so a failed checkout returns as success and the caller proceeds against the wrong tree. |
Make _checkout_branch a no-op when branch is None, and check returncode on both the fetch and the checkout, raising RuntimeError with stderr as _git_clone already does at :365-367. |
| 5 | Medium | tests/unit/test_deps_manager.py:7-47 |
Both tests call the private DepsManager._git_clone directly with an explicit branch argument, so they assert the helper's behaviour and never exercise setup() — the function that actually holds the defect in finding 3. Once the syntax is repaired these two tests pass while the bug the PR is named after is still live. That is worse than no test, because it reads as coverage. |
Add a test that drives DepsManager.setup(repo_name, ...) with no branch in the config and asserts --branch is absent from the command subprocess.run received. Keep the two helper tests as-is; they are fine, just not sufficient. |
| 6 | Medium | PR body | The PR body is the unmodified repository template: no summary, no change list, every "Type of Change" and "Testing" box unchecked, no linked issue. .github/CONTRIBUTING.md and the STANDARDS.md process requirements both expect a filled template, and master at 52e1f94 has just tightened the linked-issue policy. Nothing here states what was run. |
Fill in the template. State the failing scenario (a dependency repo whose default branch is not master), and what was run to verify — the current "Unit tests pass" checkbox is unchecked, which at least does not overclaim. |
| 7 | Low | PR body / template | The template's own checkbox labels are mojibake in the rendered body (eat, ix, efactor, est, uild) — the leading characters of feat, fix, refactor, test, build have been eaten, which is a \f/\r/\t/\b escape-interpretation bug in .github/PULL_REQUEST_TEMPLATE.md as it reaches this repo. Not this author's doing. |
Org-level issue against the shared .github template, not a change for this PR. |
Architecture conformance
Conforms. §21 places ebuild in Tier 1 — Foundation; ebuild/deps/manager.py is dependency acquisition for eos and eboot, which is Tier-1-internal and does not reach up a tier. §5.1 is not engaged: the diff adds no import, link or manifest entry, and eBuild understands the complete graph but is not a runtime dependency still holds — this code runs at developer/CI time only.
Worth noting the intent lines up with §9.2, "Actionable diagnostics with remediation guidance", and with STANDARDS.md §"Release model", which states every repo's line of development is master. That last point is the interesting wrinkle: for the EmbeddedOS org's own repos master is correct by policy, so the hardcode is defensible for eos and eboot. It breaks for any third-party or forked dependency URL, which repo_cfg["url"] explicitly allows. Say that in the PR body — it is the justification the change needs.
Proposed changes
Smallest sequence that keeps the tree building:
- Repair
ebuild/deps/manager.py:154-158back to:# Clone to cache effective_url = repo_cfg.get("url") or self._default_url(repo_name) effective_branch = repo_cfg.get("branch")
- Repair
_git_cloneindentation:(@staticmethod def _git_clone(url: str, dest: Path, branch: Optional[str], shallow: bool) -> None: cmd = ["git", "clone"] if shallow: cmd.extend(["--depth", "1"]) if branch: cmd.extend(["--branch", branch]) cmd.extend([url, str(dest)])
Optionalis already imported at:18; no import change needed.) - Guard the cached path —
if effective_branch: self._checkout_branch(dest, effective_branch)at:161, and add thereturncodechecks inside_checkout_branch. - Add the
setup()-level test described in finding 5. - Run
python3 -m py_compile ebuild/deps/manager.py,ruff check ., andpython -m pytest tests/unit/test_deps_manager.py -v, and paste the output into the PR body.
Not checked
- pytest — NOT RUN. pytest is not importable on this host, and it would fail at collection regardless while finding 1 stands. The two new tests are unverified; I read them, I did not run them.
- mypy — NOT RUN. Not installed. The
str→Optional[str]signature widening is the kind of change mypy would have an opinion about at the call site; unverified. - CI — no checks reported on
fix/default-branch-resolution. GitHub shows no check runs for this branch at all, which is why a non-parsing file reached review.mergeableandmergeStateStatusare bothUNKNOWNin the PR metadata. Hadci.yml'sLint (ruff)step run on this branch it would have caught finding 1 in seconds. - I did not test against a real remote whose default branch is
main; the failure mode in findings 3 and 4 is read from the code, not reproduced.
Verified locally: python3 -m py_compile ebuild/deps/manager.py and ruff 0.16.5 check against a git archive export of head 322066fc — both report the syntax errors quoted in findings 1 and 2.
Automated architecture review of 322066fc031f — 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.
srpatcha
left a comment
There was a problem hiding this comment.
Thanks for looking at the clone path — resolving the remote's default branch instead of assuming master is a real gap. Unfortunately the branch currently does not import: ebuild/deps/manager.py:156 has the text Clone to cache without its #, and lines 157-158 and 364-366 lost their indentation, so Python raises SyntaxError and pytest fails at collection for every test that touches ebuild.deps. It looks like a paste went wrong — please restore the # Clone to cache comment and the indentation, and run python -m pytest tests/ebuild/test_deps_manager.py locally before pushing.
Once it parses, there is a logic gap: manager.py:158 still does repo_cfg.get("branch") or "master", so _git_clone() is never called with None from clone_repo(); only your new unit tests hit that path. If the intent is to clone the remote's default branch when none is configured, that line (and _checkout_branch() at :163) need to handle the missing-branch case as well — and that is a user-visible behaviour change worth a CHANGELOG.md line.
Smaller points: please add the tests to the existing tests/ebuild/test_deps_manager.py (with the SPDX header), use a fix(ebuild): ... commit subject that says what problem this solves, and sign off the commits (git commit -s). Happy to re-review as soon as it's pushed.
fix(ebuild): resolve repository default branches
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#123 "Fix default branch resolution for Git clones"
head: 314adad author: MichaelVasco ci: fail
Verdict: Follow-up review of two new commits (f0995a1, 314adad) since head 322066fc. One of the six open findings is resolved, one is half-resolved, four are still open — and the headline problem is unchanged: the file still does not parse, and once the indentation is repaired the clone still runs git clone --branch master, because the "master" default was never in manager.py at all. It is in ebuild/deps/__init__.py:32.
Follow-up on the previous review
| Prior # | Sev | Status | Evidence |
|---|---|---|---|
| 1 | Critical | Partially addressed | The stray un-commented Clone to cache line is gone in f0995a1, but the block it belonged to is still at column 0 inside a method body. python3 -m py_compile ebuild/deps/manager.py at 314adadf → IndentationError: unexpected indent (manager.py, line 166). Still open. |
| 2 | Critical | Untouched | Neither new commit touches _git_clone. git diff 322066fc 314adadf shows changes to manager.py:128-168 only. ruff 0.16.5 at this head: Expected an indented block after 'if' statement at 362:19, Unexpected indentation at 367:1. Still open. |
| 3 | High | Addressed textually, not in effect | or "master" is gone (manager.py:155). The behaviour is unchanged — see finding 1 below. Still open. |
| 4 | High | Half addressed | Guard added: manager.py:161-162 now reads if effective_branch: self._checkout_branch(...). That half is resolved in 314adad. The silent-failure half is untouched — _checkout_branch at :372-384 still discards returncode on both the fetch --all and the checkout. Still open as finding 2 below. |
| 5 | Medium | Untouched | tests/unit/test_deps_manager.py is byte-identical to 322066fc. Still open as finding 3 below. |
| 6 | Medium | Untouched, and now red | PR body is still the unmodified template; updatedAt 2026-09-14T15:51:32Z, no body edit. Since the last review master merged #130 (52e1f94, 76970c9) adding .github/workflows/linked-issue.yml, and policy / Policy / Linked Issue now fails on this head. Still open as finding 5 below. |
| 7 | Low | Resolved upstream, not here | 52e1f94 repaired the mojibake in .github/PULL_REQUEST_TEMPLATE.md on master. This branch is 2 commits behind and still carries the broken copy. Nothing for the author to do beyond rebasing; no longer a finding. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Critical | ebuild/deps/manager.py:153-164, :362-365 |
The module still does not parse. Two independent indentation faults survive at this head. (a) The setup() block from # Clone to cache through return dest sits at column 0 inside a method whose body is at 8 — py_compile → IndentationError: unexpected indent at line 166. (b) _git_clone's if branch: body is at 4 and cmd.extend([url, str(dest)]) is at 0, both against an 8-space method body. ebuild.deps.manager is unimportable, so ebuild setup, ebuild deps *, eos_project_generator.py:429 and the two tests this PR adds all fail at import. Four commits over four days have not produced a tree that compiles. |
Re-indent both blocks to the enclosing method's 8-space body level. No logic change is needed; the intended logic is already correct. |
| 2 | High | ebuild/deps/__init__.py:32,37,43; ebuild/deps/manager.py:155 |
The PR does not do what it is named after. Removing or "master" from manager.py moved the hardcode, it did not remove it. DEFAULT_CONFIG pins "branch": "master" for eos, eboot and efirmware, and load_config() (manager.py:78-84) injects that default into any config.yaml missing the key — so repo_cfg.get("branch") returns "master" for every repo setup() documents itself as handling ("eos" or "eboot"). Verified: with the two indentation faults repaired and nothing else changed, DepsManager().setup("eos") issues ['git','clone','--depth','1','--branch','master', 'https://github.com/embeddedos-org/eos.git', ...]. The None path the new code exists to serve is reachable only for a repo name absent from DEFAULT_CONFIG. |
Drop the "branch" key from the three DEFAULT_CONFIG entries, or set it to None, so an unconfigured repo genuinely resolves to the remote's HEAD. Note this is a behaviour change for existing ~/.ebuild/config.yaml files, which already have branch: master written into them by a previous save_config() — a migration is needed, or the fix is inert for every existing install. State that in the PR body. |
| 3 | High | ebuild/deps/manager.py:372-384 |
A failed branch checkout is reported as success. Both subprocess.run calls in _checkout_branch capture output and never inspect returncode. A git checkout <branch> against a branch the remote does not have returns non-zero; setup() then returns dest as if the switch happened, and the caller builds against the wrong tree. _git_clone at :367-369 already does this correctly, five lines away. Pre-existing on master, not introduced here — but it is on the path this PR is repairing, and the PR's own premise is that a repo's branch may not be master, which is precisely when this fires. |
Check returncode on both calls and raise RuntimeError(f"...: {result.stderr.strip()}"), matching _git_clone. |
| 4 | Medium | ebuild/deps/manager.py:295 |
status() still returns repo_cfg.get("branch", "master"). Once finding 2 is fixed, ebuild deps status will report branch: master for a repo that was cloned from a remote whose default is main — a field asserting a configured branch the tool no longer uses. The adjacent git_branch field (:303) reports the truth, so the two disagree in the same output. |
repo_cfg.get("branch"), and render None as (remote default) at the CLI, not as master. |
| 5 | Medium | tests/unit/test_deps_manager.py:7-47 |
The tests pass while the bug is live — unchanged since the last review. Both tests call the private DepsManager._git_clone with an explicit branch, so neither reaches setup(), which is where finding 2 lives. Verified: against the indentation-repaired tree both tests pass (PASS test_git_clone_without_branch_does_not_pass_branch, PASS test_git_clone_with_branch_passes_branch) in the same interpreter session in which setup("eos") issued --branch master. Coverage that green-lights the defect it was written for is worse than no coverage. |
Add a test that drives DepsManager.setup("eos") against a config.yaml with no branch and asserts --branch is absent from the command subprocess.run received. That test fails at this head, which is the point. |
| 6 | Medium | ebuild/deps/manager.py:131 |
The docstring now says branch: ... Falls back to config; if unset, Git uses the remote default branch. That is false for "eos" and "eboot" — the two values the same docstring names at :129 — because of finding 2. The PR has made the documentation wrong in the act of describing the fix. |
Land finding 2 first; the docstring is correct only after it. |
| 7 | Medium | PR body | Still the unmodified repository template: no summary, no change list, no linked issue, every box unchecked. policy / Policy / Linked Issue fails on this head as a direct result (run 34864916171). This is now a red check, not only a process gap. |
Fill in the template and reference an issue. State the failing scenario (a dependency URL whose default branch is not master — which repo_cfg["url"] explicitly permits) and what was actually run. |
| 8 | Low | commits f0995a1, 314adad |
Both commit subjects are Update manager.py. .github/PULL_REQUEST_TEMPLATE.md and STANDARDS.md (Conventional Commits 1.0.0) require <type>(<scope>): <description>. |
Squash and rewrite as e.g. fix(deps): use the remote default branch when none is configured. |
Architecture conformance
Conforms. §21 places ebuild in Tier 1 — Foundation; ebuild/deps/manager.py acquires the eos / eBoot / eFirmware sources for the SDK, which is Tier-1-internal. §5.1 is not engaged: the diff adds no import, link line or manifest entry, and "eBuild understands the complete graph but is not a runtime dependency" still holds — this code runs at developer and CI time only. Tier placement of the new test is correct: tests/unit/ already exists and pytest.ini sets testpaths = tests, and the same-basename file at tests/ebuild/test_deps_manager.py does not collide because tests/unit/ carries an __init__.py and tests/ebuild/ does not, giving the two files distinct module names.
Two design-side observations, neither a defect in this PR:
- §9.2 requires "Reproducible lockfiles/manifests for production builds," and
ebuild depsresolves the platform sources by a mutable branch name with--depth 1and no recorded digest. This PR is an argument about which mutable default is right; the design never says a mutable ref is the wrong answer here at all. Proposal appended for 2026-09. STANDARDS.md"Release model" states every org repo's line of development ismaster, so the hardcode is defensible for the three repos inDEFAULT_CONFIGand breaks only for a third-party or forkedurl. That is the justification the PR body needs and does not make.
Proposed changes
Smallest sequence that leaves a working tree at every step:
- Re-indent only.
manager.py:153-164to 8 spaces (and theif dest.exists():body to 12), and_git_clone'sif branch:body to 12 withcmd.extend([url, str(dest)])at 8. Confirm withpython3 -m py_compile ebuild/deps/manager.py && ruff check .. Nothing else in the same commit — this is the commit that makes the branch reviewable again. - Remove the real default. Drop
"branch": "master"from the threeDEFAULT_CONFIGentries inebuild/deps/__init__.py, and decide what happens to an existing~/.ebuild/config.yamlthat already records it. Say which in the PR body. status()—repo_cfg.get("branch")at:295, and render theNonecase at the CLI._checkout_branch—returncodechecks on thefetchand thecheckout, raising withstderr, matching_git_clone.- The missing test —
setup()-level, per finding 5. It must fail before step 2 and pass after. - PR body — fill the template, link an issue (clears the red policy check), and paste the output of
ruff check .andpython -m pytest tests/unit/test_deps_manager.py -v. - Rebase onto
masterto pick up52e1f94, which repairs the PR template this branch still carries.
Not checked
- pytest — NOT RUN.
python3 -m pytestreportsNo module named pyteston this host. The two new tests were executed by importing the module and calling the two functions directly against an indentation-repaired scratch copy — that is what the PASS lines in finding 5 are. The suite as a whole (pytest tests/) was not run, and no statement here covers it. - mypy — NOT RUN. Not installed. The
str→Optional[str]widening at:358is unverified against the call sites. - Full CI — NOT RUN and cannot be run by the author.
CI — ebuild,CodeQLandSimulation Testare allconclusion: action_requiredon this head (verified viarepos/embeddedos-org/ebuild/commits/314adadf/check-runsandactions/runs), i.e. awaiting maintainer approval for a fork contribution.ci.ymlat this head does runruff check .and would have caught finding 1 in seconds on any of the four pushes. The only check that executed is thepull_request_target-triggered linked-issue policy, which needs no approval — and it failed. This is the gap already proposed on 2026-09-03 ("§28's evidence states are unreachable for an external contributor"); no new proposal appended for it. - Not reproduced against a real remote whose default branch is
main. Findings 2 and 3 are read from the code and, for finding 2, confirmed by intercepting thesubprocess.runargv — not by performing a clone. efirmwareis inDEFAULT_CONFIGbut not insetup()'s docstring; whethersetup("efirmware")is a supported call was not investigated.
Verified on this run, against a git archive export of head 314adadf:
python3 -m py_compile ebuild/deps/manager.py→IndentationError: unexpected indent (manager.py, line 166), exit 1.ruff 0.16.5 check ebuild/deps/manager.py→ threeinvalid-syntaxdiagnostics at166:1,362:19,367:1.- With an indentation-only repair applied to a scratch copy:
DepsManager().setup("eos")issuesgit clone --depth 1 --branch master ...;status()["branch"]is'master'; both new tests pass. origin/master'sebuild/deps/manager.pycompiles cleanly — the broken tree exists only on this branch.
Blocked / stale
Blocked on the author. reviewDecision: CHANGES_REQUESTED, mergeable: UNKNOWN, 4 commits ahead / 2 behind master, and the head still does not parse after two follow-up commits. It is also blocked on a maintainer for CI approval — but that is not what is holding it, because the defect is reproducible locally in one command. What unblocks it: step 1 above.
Automated architecture review of 314adadfe639 — 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.
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#123 "Fix default branch resolution for Git clones"
head: 93eb697 author: MichaelVasco ci: fail
Verdict: Follow-up on one new commit (93eb697) since head 314adadf. It deletes a
single comment line. Nothing from the previous review is resolved — all eight findings
are still open, including the Critical one: ebuild/deps/manager.py still does not
parse. This is the fifth commit on the branch and the third in a row that leaves the
module unimportable.
Follow-up on the previous review
git diff 314adadf 93eb697b is one hunk, one line: -# Clone to cache / + (blank) at
manager.py:153. No other file changed.
| Prior # | Sev | Status | Evidence at this head |
|---|---|---|---|
| 1 | Critical | Untouched | python3 -m py_compile ebuild/deps/manager.py → IndentationError: unexpected indent (manager.py, line 166), exit 1. ruff 0.16.5 check → three invalid-syntax diagnostics at 166:1, 362:19, 367:1. The setup() block at :154-164 is still at column 0 inside an 8-space method body; _git_clone's if branch: body is still at 4 with cmd.extend([url, str(dest)]) at 0. |
| 2 | High | Untouched | ebuild/deps/__init__.py:32,37,44 still carry "branch": "master". manager.py:155 reads repo_cfg.get("branch"), which load_config() has already populated with "master" for eos, eboot and efirmware. |
| 3 | High | Untouched | manager.py:371-384 — both subprocess.run calls in _checkout_branch still discard returncode. |
| 4 | Medium | Untouched | manager.py:295 still repo_cfg.get("branch", "master"). |
| 5 | Medium | Untouched | git diff 314adadf 93eb697b -- tests/unit/test_deps_manager.py is empty. Byte-identical for the third review running. |
| 6 | Medium | Untouched | manager.py:131 still asserts the remote default is used when no branch is configured. False while finding 2 stands. |
| 7 | Medium | Untouched | PR body is still the unmodified template. policy / Policy / Linked Issue is completed / failure on this head (run 34865960264) — the only check that executed. |
| 8 | Low | Untouched, and repeated | 93eb697's subject is again Update manager.py — the third commit on this branch with that subject. |
Findings
Carried forward unchanged from ebuild-123-314adadf.md; the recommended fixes there
still apply verbatim. Line numbers refreshed to this head.
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Critical (P0) | ebuild/deps/manager.py:154-166, :362-367 |
The module does not parse; ebuild.deps.manager is unimportable, so ebuild setup, ebuild deps *, eos_project_generator.py:429 and the two tests this PR adds all fail at import. |
Re-indent only. :154-157 to 8 spaces, :159 to 8 with its body at 12, :363 to 12, :365 to 8. No logic change — the intended logic is already correct. |
| 2 | High (P1) | ebuild/deps/__init__.py:32,37,44; manager.py:155 |
The PR does not do what it is named after: the "master" default lives in DEFAULT_CONFIG, not in manager.py. repo_cfg.get("branch") returns "master" for every repo setup() documents. |
Drop or None the three "branch" keys — but see the new note below, the comment above them argues the opposite, so the PR body has to answer it. Existing ~/.ebuild/config.yaml files already record branch: master, so a migration is needed or the fix is inert for every existing install. |
| 3 | High (P1) | ebuild/deps/manager.py:371-384 |
A failed git fetch --all / git checkout <branch> is reported as success; setup() returns dest and the caller builds the wrong tree. _git_clone at :367-369 does this correctly five lines away. |
Check returncode on both calls and raise RuntimeError(f"...: {result.stderr.strip()}"). |
| 4 | Medium (P2) | ebuild/deps/manager.py:295 |
status() reports branch: master for a repo cloned from a remote whose default is main, disagreeing with the adjacent git_branch field at :303 in the same output. |
repo_cfg.get("branch"), rendered as (remote default) at the CLI. |
| 5 | Medium (P2) | tests/unit/test_deps_manager.py:7-47 |
Both tests call the private _git_clone with an explicit branch, so neither reaches setup() — where the defect is. They pass while the bug is live. |
Add a setup()-level test with no branch in the config asserting --branch is absent from the argv subprocess.run receives. It must fail at this head. |
| 6 | Medium (P2) | ebuild/deps/manager.py:131 |
The docstring describes behaviour the code does not have, for the two repo names the same docstring lists at :129. |
Correct only after finding 2 lands. |
| 7 | Medium (P2) | PR body | Unmodified template: no summary, no change list, no linked issue, every box unchecked. Directly causes the one red check. | Fill the template, reference an issue, state the failing scenario and what was run. |
| 8 | Low (P3) | commits f0995a1, 314adad, 93eb697 |
Three commit subjects of Update manager.py. STANDARDS.md requires Conventional Commits 1.0.0. |
Squash and rewrite as e.g. fix(deps): use the remote default branch when none is configured. |
New this review — not a new finding, but it changes what finding 2 requires of the
author. ebuild/deps/__init__.py:27-29, directly above the three "branch": "master"
entries, is a comment recording the decision deliberately:
# Both repos default to master; neither has a main. Cloning the branch named here is
# whatebuild setupdoes first, so "main" failed it for every new developer before
# any build ran.
So finding 2 is not a leftover — it is a documented choice, made after an outage it
names. Removing it is still the right change for a third-party or forked url, which
repo_cfg["url"] explicitly permits, but the PR now has to say why the recorded reason
no longer applies (it does not: for the three org repos STANDARDS.md fixes the line of
development at master, so the correct fix is to let a non-default url resolve to
the remote's HEAD, not to unset the default for eos/eboot/efirmware). That
comment must be updated in the same commit, or it becomes the next wrong document.
Architecture conformance
Conforms, unchanged from the previous two reviews and re-checked against the diff at
this head. §21 places ebuild in Tier 1 — Foundation; ebuild/deps/manager.py acquires
the eos/eBoot/eFirmware sources for the SDK, which is Tier-1-internal. §5.1 is not
engaged: the one-line diff adds no import, link line or manifest entry, and "eBuild
understands the complete graph but is not a runtime dependency" still holds — this code
runs at developer and CI time only.
The §9.2 reproducibility gap this PR sits on top of (the SDK resolves platform sources by
mutable branch name, --depth 1, no recorded digest) was already proposed on 2026-09-14
in .ai/autoreview/proposals/2026-09.md. No new proposal appended — nothing in this
commit reveals a further gap in the master design.
Proposed changes
Unchanged from ebuild-123-314adadf.md, steps 1–7. Step 1 is the only one that matters
right now and is one edit:
- Re-indent
manager.py:154-166and:362-367to the enclosing method's body level.
Confirm withpython3 -m py_compile ebuild/deps/manager.py && ruff check .. Commit
nothing else — this is the commit that makes the branch reviewable.
A note on method, offered because four days and five commits have not produced a tree
that compiles: every commit on this branch is a single-line change with the subject
Update manager.py, which is what the GitHub web editor produces. Indentation repair is
not work the web editor is good at. Clone the branch, run ruff check . locally, push
once.
Not checked
- pytest — NOT RUN for this repo at this head.
python3 -m pytestreports
No module named pytestfor the system interpreter, and collection would fail at
import regardless while finding 1 stands. The two new tests remain unexecuted by me. - mypy — NOT RUN. Not installed. The
str→Optional[str]widening at:358is
unverified against call sites. - Full CI — NOT RUN and not runnable by the author.
repos/embeddedos-org/ebuild/commits/93eb697b/check-runs
returns exactly one check run:policy / Policy / Linked Issue,completed,
failure.CI — ebuild,CodeQLandSimulation Testproduced no run at all on this
head — a fork PR awaiting maintainer approval.ci.ymlrunsruff check .and would
have caught finding 1 in seconds on any of the five pushes. Already covered by the
2026-09-03 proposal on §28's evidence states being unreachable for external
contributors; no new proposal. - Not reproduced against a real remote whose default branch is
main. Findings 2 and
3 are read from the code at this head; the argv interception that confirmed finding 2
was done at the previous head against an indentation-repaired scratch copy and was not
repeated, because the file is byte-identical in the relevant region. efirmwareis inDEFAULT_CONFIGbut not insetup()'s docstring; still not
investigated.
Verified on this run, against a git archive export of 93eb697b (Linux, CPython
3.12, ruff 0.16.5):
python3 -m py_compile ebuild/deps/manager.py→IndentationError: unexpected indent (manager.py, line 166), exit 1.ruff check ebuild/deps/manager.py→invalid-syntaxat166:1,362:19,367:1.git diff --stat 314adadf 93eb697b→ebuild/deps/manager.py | 2 +-, one file.git rev-list --left-right --count origin/master...93eb697b→ 2 behind, 5 ahead.gh api .../commits/93eb697b/check-runs→ one check run,policy / Policy / Linked Issue,failure.
Blocked / stale
Stale, and blocked on the author. reviewDecision: CHANGES_REQUESTED,
mergeStateStatus: BLOCKED, mergeable: MERGEABLE, 5 ahead / 2 behind master. One
commit in four days, resolving none of eight open findings, on a head that does not
compile. It is secondarily blocked on a maintainer for fork-CI approval, but that is not
what is holding it — the Critical defect reproduces locally in one command.
What unblocks it: step 1 above.
Automated architecture review of 93eb697b40e9 — 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.
fix(ebuild): resolve repository default branches
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#123 "Fix default branch resolution for Git clones"
head: 91c6952 author: MichaelVasco ci: fail — policy / Policy / Linked Issue is the only check that ran, and it failed
Verdict: Follow-up on five new commits (420f75b, c935cfc, 2178ca8, 59a5b96, 91c6952)
since head 93eb697b. Two of eight findings are addressed textually. The Critical one is not, and
the file is now materially more broken than at any previous review: _checkout_branch's body has
been pasted four times, line 371 is the literal text def @staticmethod, and there is a dangling
) -> None: at :397. python3 -m py_compile → IndentationError: unexpected indent at line 165.
This is the tenth commit on the branch and the fourth consecutive review at which
ebuild.deps.manager cannot be imported.
Because three reviews of "re-indent lines X–Y" have not landed, this one carries the repaired code
instead. I applied it to a scratch export, and it compiles — and with it applied, the PR still does
not do what it is named after: setup("eos") issues git clone --depth 1 --branch master.
Follow-up on the previous review
| Prior # | Sev | Status | Evidence at this head |
|---|---|---|---|
| 1 | Critical (P0) | Untouched, and worse | python3 -m py_compile ebuild/deps/manager.py → IndentationError: unexpected indent (manager.py, line 165), exit 1. The setup() block at :154-163 is still at column 0 inside an 8-space method body. _git_clone at :361-364 still has its if branch: body at 4 and cmd.extend([url, str(dest)]) at 0. New damage in these five commits: :370 @staticmethod at column 0; :371 the literal line def @staticmethod, which is not valid Python under any indentation; :372-459 the _checkout_branch body repeated four times (grep -c "current = DepsManager._git_current_branch(repo_dir)" → 4); :397 a stray ) -> None: terminating a raise RuntimeError( call. The file grew from 406 to 485 lines. origin/master's copy compiles cleanly, so the breakage exists only on this branch. |
| 2 | High (P1) | Untouched | git diff 93eb697b..91c69521 touches ebuild/deps/manager.py only. ebuild/deps/__init__.py:32,37,44 still carry "branch": "master". Re-verified by execution, not by reading — see below. |
| 3 | High (P1) | Addressed in text, unreachable in fact | One of the four pasted copies of _checkout_branch does now check returncode on both the fetch --all and the checkout and raises RuntimeError with stderr, which is exactly the fix. It is inside a function definition Python cannot parse, and the fourth copy (:448-459) is the old body with both subprocess.run results still discarded. Credit for the intent; nothing executes. Re-verify once finding 1 is closed. |
| 4 | Medium (P2) | Addressed in text, inert in fact | :294 is now repo_cfg.get("branch") — correct, and the right change. It has no effect while finding 2 stands: on the repaired copy status() still reports branch: 'master' for eos and eboot, because load_config() has already written the DEFAULT_CONFIG value into the config. Measured, not inferred. |
| 5 | Medium (P2) | Untouched | tests/unit/test_deps_manager.py byte-identical for the fourth review running. And now executed: on the repaired copy both tests pass (2 passed in 0.04s) in the same interpreter in which setup("eos") issues --branch master. The previous reviews could not run pytest; this one could. The tests green-light the defect they were written for. |
| 6 | Medium (P2) | Untouched | :131's docstring still asserts the remote default is used when no branch is configured. False for eos and eboot while finding 2 stands. |
| 7 | Medium (P2) | Untouched | PR body is still the unmodified template (1369 bytes, every box unchecked, mojibake labels intact, no linked issue). policy / Policy / Linked Issue completed / failure is the only check run on this head. |
| 8 | Low (P3) | Untouched, and compounding | All five new commits are subject Update manager.py. That is nine of the ten commits on this branch. STANDARDS.md requires Conventional Commits 1.0.0. |
Findings
Carried forward; severities and recommended fixes unchanged from ebuild-123-93eb697b.md except
where noted. Rather than restate them, the actionable content of this review is the repair below.
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Critical (P0) | ebuild/deps/manager.py:154-163, :361-364, :370-459 |
The module does not parse. ebuild setup, ebuild deps *, eos_project_generator.py:429 and the two tests this PR adds all fail at import. Ten commits over five days have not produced a tree that compiles. |
The exact repair, verified below. Three edits, no logic change. |
| 2 | High (P1) | ebuild/deps/__init__.py:32,37,44 |
The "master" default is in DEFAULT_CONFIG, not in manager.py, so the PR's stated purpose is still not achieved. Re-verified by execution this run. |
Unchanged — and note __init__.py:27-29 records the decision deliberately after a named outage, so the right change is to let a non-default url resolve to the remote's HEAD, not to unset the default for the three org repos. The PR body has to say which. |
| 3 | High (P1) | ebuild/deps/manager.py:448-459 |
The surviving old copy still discards both returncodes. |
Keep only the checked version; see the repair. |
| 4–8 | As tabulated above. | Unchanged. |
The repair, and what it proves
Applied to a git archive export of 91c69521. Three edits, no logic invented — every line below
already exists somewhere on this branch, in the wrong place or duplicated.
(a) setup(), replacing :154-168 — re-indent to the method's 8-space body:
effective_url = repo_cfg.get("url") or self._default_url(repo_name)
effective_branch = repo_cfg.get("branch")
dest = self.cache_dir / repo_name
if dest.exists():
# Already cloned — optionally switch branch
if effective_branch:
self._checkout_branch(dest, effective_branch)
self.save_config()
return dest
self.cache_dir.mkdir(parents=True, exist_ok=True)
self._git_clone(effective_url, dest, effective_branch, shallow)
self.save_config()
return dest(b) _git_clone, replacing :361-364:
if branch:
cmd.extend(["--branch", branch])
cmd.extend([url, str(dest)])(c) _checkout_branch — delete :370 through :459 entirely (the stray @staticmethod, the
def @staticmethod line, all four pasted bodies) and put back one copy:
@staticmethod
def _checkout_branch(repo_dir: Path, branch: Optional[str]) -> None:
if not branch:
return
current = DepsManager._git_current_branch(repo_dir)
if current == branch:
return
fetch = subprocess.run(
["git", "-C", str(repo_dir), "fetch", "--all"],
capture_output=True,
text=True,
)
if fetch.returncode != 0:
raise RuntimeError(f"Failed to fetch {repo_dir}: {fetch.stderr.strip()}")
checkout = subprocess.run(
["git", "-C", str(repo_dir), "checkout", branch],
capture_output=True,
text=True,
)
if checkout.returncode != 0:
raise RuntimeError(f"Failed to checkout {branch}: {checkout.stderr.strip()}")That closes findings 1 and 3 together. It is not a substitute for finding 2.
With exactly those three edits and nothing else:
python3 -m py_compile ebuild/deps/manager.py -> OK
pytest tests/unit/test_deps_manager.py -q -> 2 passed in 0.04s
DepsManager().setup("eos") -> git clone --depth 1 --branch master
https://github.com/embeddedos-org/eos.git
status()['eos']['branch'] -> 'master'
Those last two lines are the point of this review. The branch can be made to compile without moving
the defect it is named after one inch, and the tests it adds will report success while it does. The
PR is not finished when it parses.
Verification performed for this review
The user's ebuild checkout has a dirty working tree (TASKS.md, ebuild/cli/integration.py,
tests/ebuild/test_integration_initramfs_security.py modified; the sync step reported it as
SKIP dirty tree (4 files)). It was not touched — no worktree was added to it, nothing was
checked out, stashed or reset. Everything below ran against git archive 91c69521 | tar -x into a
temporary directory outside the repository. Nothing was committed or pushed. 93eb697b..91c69521
was not present locally, so the PR head was fetched into refs/autoreview/scratch123 read-only.
| Check | Result |
|---|---|
python3 -m py_compile ebuild/deps/manager.py at 91c69521 |
FAIL — IndentationError: unexpected indent (manager.py, line 165), exit 1 |
ast.parse for a precise location |
IndentationError: unexpected indent at line 165 col 8, text self.cache_dir.mkdir(parents=True, exist_ok=True) |
Duplicate count of the _checkout_branch body |
4 (grep -c "current = DepsManager._git_current_branch(repo_dir)") |
grep -n "def @staticmethod" |
371:def @staticmethod — present, literally |
| File length | 485 lines (406 at 93eb697b) |
origin/master's ebuild/deps/manager.py |
compiles — the breakage is only on this branch |
git rev-list --left-right --count origin/master...91c69521 |
2 behind, 10 ahead |
| Commit subjects on the branch | 9 of 10 are Update manager.py; the tenth is Create test_deps_manager.py |
Repaired copy — python3 -m py_compile |
OK |
Repaired copy — pytest tests/unit/test_deps_manager.py -q |
2 passed, 0.04s. (The previous three reviews recorded pytest as NOT RUN; it is available in this run's environment, so the two tests are now executed rather than read.) |
Repaired copy — setup("eos") with subprocess.run intercepted |
['git','clone','--depth','1','--branch','master','https://github.com/embeddedos-org/eos.git', …] — finding 2 confirmed live by execution |
Repaired copy — status() |
branch: 'master' for both eos and eboot — finding 4's textual fix is correct and inert |
m._config["repos"]["eos"]["branch"] as load_config() leaves it |
'master' — injected from DEFAULT_CONFIG, exactly as finding 2 describes |
gh api .../commits/91c69521/check-runs |
one check run: policy / Policy / Linked Issue, completed, failure. CI — ebuild, CodeQL, Simulation Test produced no run at all. |
| PR metadata | mergeable: MERGEABLE, mergeStateStatus: BLOCKED, reviewDecision: CHANGES_REQUESTED, body 1369 bytes and still the unmodified template |
Architecture conformance
Conforms, unchanged from the previous three reviews and re-checked against this head's diff. §21
places ebuild in Tier 1 — Foundation; ebuild/deps/manager.py acquires the eos/eBoot/eFirmware
sources for the SDK, which is Tier-1-internal. §21.1 is not engaged — no repository boundary moves.
§5.1 is not engaged either: the diff adds no import, link line or manifest entry, and "eBuild
understands the complete graph but is not a runtime dependency" still holds, since this code runs at
developer and CI time only.
The §9.2 gap this PR sits on top of — the SDK resolving platform sources by mutable branch name with
--depth 1 and no recorded digest, against §9.2's "Reproducible lockfiles/manifests for production
builds" — was already proposed on 2026-09-14 in .ai/autoreview/proposals/2026-09.md. No new
proposal appended: nothing in these five commits reveals a further gap in the master design. The
defect here is not a design gap, it is an editor.
Proposed changes
1. Apply (a), (b), (c) above in ONE commit. Confirm with
python3 -m py_compile ebuild/deps/manager.py && ruff check .
Commit nothing else. This is the commit that makes the branch reviewable.
2. Decide finding 2 and say so in the body: __init__.py:27-29 records the
"master" default as a deliberate choice made after an outage. The correct
fix is to let a non-default `url` resolve to the remote's HEAD, not to
unset the default for eos/eboot/efirmware. Update that comment in the
same commit, or it becomes the next wrong document. Existing
~/.ebuild/config.yaml files already record branch: master — say what
happens to them.
3. The setup()-level test (finding 5). It must FAIL before step 2 and pass
after. The two existing tests must stay; they are fine, just not sufficient.
4. Docstring at :131 (finding 6) — correct only after step 2.
5. PR body (finding 7): fill the template, link an issue. That clears the one
red check. Paste the output of py_compile, ruff and pytest.
6. Squash and rewrite the ten commits as one Conventional Commit (finding 8).
7. Rebase onto master to pick up 52e1f94, which repairs the PR template this
branch still carries.
A note on method, offered because five days and ten commits have not produced a tree that
compiles, and this head is worse than the first one. Every commit on this branch is subject
Update manager.py, which is what the GitHub web editor produces, and the new damage — a body pasted
four times, a def @staticmethod line — is the signature of repeated paste-into-textarea. This file
cannot be repaired that way. Clone the branch, make the three edits in an editor, run
python3 -m py_compile ebuild/deps/manager.py before pushing, push once. The repair above is
copy-pasteable and verified.
No fix PR opened. The blocking finding is Critical, and the brief's autofix rule is High only.
More decisively: the defect exists only on this PR's branch — origin/master compiles — and the
brief forbids touching a branch belonging to an existing PR, so a branch cut from the default branch,
which is what fix-start.sh produces, would have nothing to patch. The repair is therefore delivered
as text, verified, rather than as a pull request.
Blocked / stale
Blocked on the author, and regressing. reviewDecision: CHANGES_REQUESTED,
mergeStateStatus: BLOCKED, 10 ahead / 2 behind master, and the head does not compile after five
more commits. It is secondarily blocked on a maintainer for fork-CI approval — CI — ebuild,
CodeQL and Simulation Test have produced no run on any head of this branch — but that is not what
is holding it: ci.yml runs ruff check . and would have caught this in seconds on any of the ten
pushes, and the defect reproduces locally in one command without any CI at all.
What unblocks it: step 1.
Not checked
ruff— NOT RUN. Noruffon this host in this run (the previous reviews had 0.16.5). The
syntax findings are from CPython 3.12'spy_compileandast.parse, which is sufficient for
finding 1 but means no statement here covers style or lint diagnostics.- mypy — NOT RUN. Not installed. The
str→Optional[str]widening at:357and:372is
unverified against call sites. - The full suite — NOT RUN. Only
tests/unit/test_deps_manager.pywas executed, against the
repaired scratch copy.pytest tests/as a whole was not run, and nothing here covers it. - Full CI — NOT RUN and not runnable by the author. One check run exists on this head. Already
covered by the 2026-09-03 proposal on §28's evidence states being unreachable for external
contributors; no new proposal. - Not reproduced against a real remote whose default branch is
main. Findings 2 and 3 are read
from the code and, for finding 2, confirmed by intercepting thesubprocess.runargv — not by
performing a clone. - The repaired copy is mine, not the author's. It is offered as evidence and as a starting point;
it has not been run against a realgit clone, and_checkout_branchin particular was exercised
only throughpy_compile, never called. efirmwareis inDEFAULT_CONFIGbut not insetup()'s docstring; still not investigated, for
the fourth review running.- The user's
ebuildworking tree was deliberately not inspected beyondgit status --short, and
its four modified files play no part in anything above.
Automated architecture review of 91c695212e4f — 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.
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#123 "Fix default branch resolution for Git clones"
head: e3446d2 author: MichaelVasco ci: fail — policy / Policy / Linked Issue is still the only check that runs, and it still fails
Verdict: Follow-up on one new commit (e3446d2) since head 91c69521. It deletes the
def @staticmethod line — real progress on one piece of the damage — and in the same hunk
pastes a sixth copy of the git checkout block, so the file still does not parse and the
duplication got worse, not better. python3 -m py_compile → IndentationError at line 165,
exit 1. This is the eleventh commit on the branch and the fifth consecutive review at which
ebuild.deps.manager cannot be imported. Findings 1–8 from ebuild-123-91c69521.md are all
still open.
Follow-up on the previous review
git diff 91c69521 e3446d2d is one file, two hunks, 10 insertions / 2 deletions.
| Prior # | Sev | Status | Evidence at this head |
|---|---|---|---|
| 1 | Critical (P0) | Partially addressed — still broken | python3 -m py_compile ebuild/deps/manager.py → IndentationError: unexpected indent (manager.py, line 165), exit 1. ruff 0.16.5 reports 6 invalid-syntax diagnostics: 165:1, 361:19, 366:1, 405:15, 405:18, 405:23. Fixed by e3446d2: the literal def @staticmethod line at :371 is gone. Still present: setup()'s block at :154-163 at column 0 inside an 8-space method body; _git_clone's if branch: body at 4 with cmd.extend([url, str(dest)]) at 0 (:361-366); the stray ) -> None: now at :405; the _checkout_branch body still pasted 4× (grep -c "current = DepsManager._git_current_branch(repo_dir)" → 4). File grew 485 → 493 lines. |
| 2 | High (P1) | Untouched | git diff 91c69521 e3446d2d touches ebuild/deps/manager.py only. ebuild/deps/__init__.py:31,36,43 still carry "branch": "master" for eos, eboot and efirmware; :27-29 still carries the comment that records that choice deliberately. manager.py:155 still reads repo_cfg.get("branch"), which load_config() has already populated from DEFAULT_CONFIG. |
| 3 | High (P1) | Untouched | manager.py:455-466 — the surviving old copy of _checkout_branch still fires fetch --all and checkout and discards both returncodes. |
| 4 | Medium (P2) | Addressed in text, still inert | :294 is repo_cfg.get("branch"). Correct, and still has no effect while finding 2 stands. |
| 5 | Medium (P2) | Untouched | git diff 91c69521 e3446d2d -- tests/unit/test_deps_manager.py is empty. Byte-identical for the fifth review running. |
| 6 | Medium (P2) | Untouched | :131 still reads "Falls back to config; if unset, Git uses the remote default branch." False for eos, eboot and efirmware while finding 2 stands. |
| 7 | Medium (P2) | Untouched | PR body is still the unmodified template — every box unchecked, mojibake labels (\feat, \fix, \r\nefactor) intact, no linked issue, ## Changes still two empty bullets. policy / Policy / Linked Issue completed / failure is the only check run on this head. |
| 8 | Low (P3) | Untouched, and compounding | e3446d2's subject is Update manager.py — ten of the eleven commits on this branch. STANDARDS.md requires Conventional Commits 1.0.0. |
Findings
Carried forward from ebuild-123-91c69521.md with line numbers refreshed to this head, plus
one new finding introduced by e3446d2 itself.
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Critical (P0) | ebuild/deps/manager.py:154-163, :361-366, :370-466, :405 |
The module does not parse. ebuild setup, ebuild deps *, eos_project_generator.py:429 and the two tests this PR adds all fail at import — confirmed by running pytest, below. Eleven commits over six days have not produced a tree that compiles. |
The verified three-edit repair in ebuild-123-91c69521.md §"The repair, and what it proves" still applies verbatim, with (c) now deleting :370-466. No logic change — the intended logic is already on the branch, in the wrong places. |
| 9 | High (P1) | ebuild/deps/manager.py:383-393 |
New in e3446d2. The commit inserts a second, identical checkout = subprocess.run([... "checkout", branch]) / if checkout.returncode != 0: raise block immediately above the one already at :394-404. Once finding 1 is repaired this runs git checkout <branch> twice per call. The second is a no-op on the happy path, but it doubles the work in the one path that touches the network-adjacent working tree, and it guarantees the two copies drift the next time one is edited. |
Delete :383-393. It is a duplicate of the block directly below it. |
| 2 | High (P1) | ebuild/deps/__init__.py:31,36,43; manager.py:155 |
The PR still does not do what it is named after: the "master" default lives in DEFAULT_CONFIG, not in manager.py. |
Unchanged. __init__.py:27-29 records the default as a deliberate choice made after a named onboarding outage, so the correct fix is to let a non-default url resolve to the remote's HEAD, not to unset the default for the three org repos — for which STANDARDS.md fixes the line of development at master anyway. The PR body has to say which, and that comment must be updated in the same commit. Existing ~/.ebuild/config.yaml files already record branch: master, so say what happens to them or the fix is inert for every existing install. |
| 3 | High (P1) | ebuild/deps/manager.py:455-466 |
The surviving old copy still discards both returncodes: a failed git fetch --all / git checkout is reported as success and the caller builds the wrong tree. _git_clone at :367-369 does this correctly a hundred lines away. |
Deleted by repair (c); keep only the checked copy. |
| 4 | Medium (P2) | ebuild/deps/manager.py:294 |
status() reports the configured branch, which is master for every repo DEFAULT_CONFIG names, disagreeing with the adjacent git_branch field in the same output. Textual fix landed; blocked behind finding 2. |
Unchanged — render None as (remote default) at the CLI. |
| 5 | Medium (P2) | tests/unit/test_deps_manager.py:7-47 |
Both tests call the private _git_clone with an explicit branch, so neither reaches setup(), where the defect is. Measured at 91c69521: on a repaired copy they pass in the same interpreter in which setup("eos") issues --branch master. |
Add a setup()-level test with no branch in the config asserting --branch is absent from the argv subprocess.run receives. It must fail before finding 2 is fixed. |
| 6 | Medium (P2) | ebuild/deps/manager.py:131 |
The docstring describes behaviour the code does not have, for the repo names the same docstring lists at :129. |
Correct only after finding 2 lands. |
| 7 | Medium (P2) | PR body | Unmodified template. Directly causes the one red check. | Fill it, link an issue, paste the output of py_compile, ruff and pytest. |
| 8 | Low (P3) | commits 322066f…e3446d2 |
Ten of eleven commit subjects are Update manager.py. |
Squash and rewrite as one Conventional Commit, e.g. fix(deps): resolve the remote default branch for non-default clone URLs. |
Architecture conformance
Conforms, unchanged from the previous four reviews and re-checked against this head's diff.
§21 places ebuild in Tier 1 — Foundation; ebuild/deps/manager.py acquires the
eos/eBoot/eFirmware sources for the SDK, which is Tier-1-internal. §21.1 is not engaged —
no repository boundary moves. §5.1 is not engaged: the diff adds no import, link line or
manifest entry, and "eBuild understands the complete graph but is not a runtime dependency"
holds, since this code runs at developer and CI time only.
The §9.2 gap this PR sits on top of — the SDK resolving platform sources by mutable branch name
with --depth 1 and no recorded digest, against §9.2's "Reproducible lockfiles/manifests for
production builds" — was proposed on 2026-09-14 in .ai/autoreview/proposals/2026-09.md.
No new proposal appended: nothing in this commit reveals a further gap in the master design.
Proposed changes
1. Apply repair (a)+(b)+(c) from ebuild-123-91c69521.md in ONE commit, and in
the same commit delete manager.py:383-393 (finding 9 — the duplicate
checkout block this commit just added). Confirm with:
python3 -m py_compile ebuild/deps/manager.py && ruff check .
Commit nothing else. This is the commit that makes the branch reviewable.
2. Decide finding 2 and say so in the body. Update __init__.py:27-29 in the
same commit or it becomes the next wrong document.
3. Add the setup()-level test (finding 5). It must FAIL before step 2 and pass
after.
4. Docstring at :131 (finding 6) — correct only after step 2.
5. PR body (finding 7): fill the template, link an issue. That clears the one
red check.
6. Squash and rewrite the eleven commits as one Conventional Commit (finding 8).
7. Rebase onto master (2 behind) to pick up 52e1f94, which repairs the PR
template this branch still carries.
A note on method, repeated because six days and eleven commits have not produced a tree that
compiles. This commit removed one bad line and added a pasted duplicate eight lines long —
the signature of editing in a browser textarea. The repair in the previous review is
copy-pasteable and was verified to compile. Clone the branch, make the edits in an editor, run
python3 -m py_compile ebuild/deps/manager.py before pushing, push once.
No fix PR opened. The blocking finding is Critical and the brief's autofix rule is High
only; finding 9 is High but exists only on this PR's branch, which the brief forbids touching.
origin/master's copy of manager.py compiles cleanly — a branch cut from the default branch,
which is what fix-start.sh produces, would have nothing to patch. The repair is delivered as
text.
Verification performed for this review
The user's ebuild checkout has a dirty working tree (TASKS.md, ebuild/cli/integration.py,
tests/ebuild/test_integration_initramfs_security.py modified, smart-sensor/ untracked). It was
not touched — no worktree added, nothing checked out, stashed or reset. Everything below ran
against git archive e3446d2d | tar -x into a temporary directory outside the repository.
| Check | Result |
|---|---|
python3 -m py_compile ebuild/deps/manager.py (CPython 3.12) |
FAIL — IndentationError: unexpected indent (manager.py, line 165), exit 1 |
ast.parse for a precise location |
unexpected indent at line 165 col 8, text self.cache_dir.mkdir(parents=True, exist_ok=True) |
ruff 0.16.5 check ebuild/deps/manager.py |
FAIL — 6 errors, all invalid-syntax: 165:1, 361:19, 366:1, 405:15, 405:18, 405:23 |
pytest 9.1.1 tests/unit/test_deps_manager.py -q |
FAIL — 1 collection error: from ebuild.deps.manager import DepsManager → IndentationError at manager.py:165. 0 tests ran |
grep -c "current = DepsManager._git_current_branch(repo_dir)" |
4 — unchanged from 91c69521 |
grep -c '"checkout", branch' |
5 at this head (4 at 91c69521) — finding 9 |
grep -n "def @staticmethod" |
no match — the line e3446d2 deleted |
| File length | 493 lines (485 at 91c69521, 406 at 93eb697b) |
git diff --stat 91c69521 e3446d2d |
ebuild/deps/manager.py | 12 ++++++++++--, one file |
git diff 91c69521 e3446d2d -- tests/unit/test_deps_manager.py |
empty |
git rev-list --left-right --count origin/master...e3446d2d |
2 behind, 11 ahead |
gh api .../commits/e3446d2d/check-runs |
one check run: policy / Policy / Linked Issue, completed, failure |
| PR metadata | mergeable: MERGEABLE, mergeStateStatus: BLOCKED, reviewDecision: CHANGES_REQUESTED, body still the unmodified template |
Blocked / stale
Blocked on the author. reviewDecision: CHANGES_REQUESTED, mergeStateStatus: BLOCKED,
11 ahead / 2 behind master, and the head does not compile after eleven commits. It is
secondarily blocked on a maintainer for fork-CI approval — CI — ebuild, CodeQL and
Simulation Test have produced no run on any head of this branch — but that is not what is
holding it: ci.yml runs ruff check . and would have caught this in seconds on any of the
eleven pushes, and the defect reproduces locally in one command with no CI at all.
What unblocks it: step 1.
Not checked
- mypy — NOT RUN. Not installed on this host. The
str→Optional[str]widening at:357
and:371is unverified against call sites. - The full pytest suite — NOT RUN. Only
tests/unit/test_deps_manager.pywas collected, and
it errored at import.pytest tests/as a whole was not run and nothing here covers it. - Finding 2 was not re-confirmed by execution at this head. It was confirmed at
91c69521
by interceptingsubprocess.run's argv on a repaired copy (git clone --depth 1 --branch master …).manager.py:155and all ofebuild/deps/__init__.pyare byte-identical between
the two heads, so the earlier measurement still stands, but it was not repeated. - No repaired copy was built this run. The repair is carried forward from the previous
review's verified text; it was not re-applied or re-compiled at this head, and finding 9's
"runs checkout twice" consequence is read from the source, not executed. - Full CI — NOT RUN and not runnable by the author. One check run exists on this head.
Already covered by the 2026-09-03 proposal on §28's evidence states being unreachable for
external contributors; no new proposal. - Not reproduced against a real remote whose default branch is
main. No clone was
performed by this review. efirmwareis inDEFAULT_CONFIGbut not insetup()'s docstring at:129, and the
comment at:27-29still says "Both repos" while three are listed. Still not investigated,
for the fifth review running.- The user's
ebuildworking tree was deliberately not inspected beyondgit status --short, and its four modified files play no part in anything above.
Automated architecture review of e3446d2d6258 — 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.
docs(ebuild): document default branch resolution fix
Summary
Type of Change
efactor — Code restructuring without behavior change
Changes
Testing
Pre-Submission Checklist
Related Issues
Screenshots / Logs
Additional Notes