Skip to content

fix: repair unreachable submodule refs#1772

Closed
dutchiono wants to merge 1 commit intomilady-ai:developfrom
dutchiono:codex/fix-missing-submodule-refs
Closed

fix: repair unreachable submodule refs#1772
dutchiono wants to merge 1 commit intomilady-ai:developfrom
dutchiono:codex/fix-missing-submodule-refs

Conversation

@dutchiono
Copy link
Copy Markdown
Collaborator

Summary

  • repoint steward-fi to a reachable commit on its configured remote
  • repoint test/contracts/lib/openzeppelin-contracts to a reachable commit on its configured remote

Why

Fresh Windows installs were failing during submodule initialization because the superproject referenced two submodule SHAs that the remotes no longer served.

Validation

  • confirmed the updated submodule pointers resolve locally to:
    • steward-fi: 3a6b97b138353d30860507fe5c66f058bd7d759a
    • test/contracts/lib/openzeppelin-contracts: 9cfdccd35350f7bcc585cf2ede08cd04e7f0ec10
  • note: this PR is metadata-only and has no meaningful code-level regression test to add

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

The comment is pending your approval. Here's a summary of the review:


PR #1772 — Decision: REQUEST CHANGES

Reclassification: This is a Windows compat multi-fix PR, not "repair unreachable submodule refs." The actual submodule SHA updates are 2 lines; the other 598 lines cover .cmd shims, plugin checkout readiness, a bunx tsup fallback, workspace-dep resolution improvements, and a BUN version bump. A new contributor submitting this under a minimal title warrants scrutiny.

All 12 universal invariants intact.

Blocking issues:

  1. PR title/body don't match the diff — needs a description that covers prepare-package-dist.mjs and all the Windows compat work, not just submodule SHAs.

  2. BUN_VERSION split is unexplainedtest-electrobun-release.yml bumped to 1.3.11, three release workflows stay at 1.3.9. Is this intentional or an oversight? Must be stated explicitly.

  3. steward-fi SHA needs chain of custodyc0883fd3a6b97b from a new contributor. Needs explicit confirmation of which tag/branch 3a6b97b represents on the remote.

  4. Dead SUBMODULE_READINESS_MARKERS entryplugins/plugin-agent-orchestrator entry is now unreachable since plugins/* paths bypass the markers table. Remove it or document why it's retained.

Non-blocking concerns: getBuildCommandFallback arg splitting doesn't handle quoted arguments with spaces; ensureWindowsCmdShim undocumentedly assumes Node.js bin targets; read-tree recovery on shallow clones may fail noisily.

Security: Clear — no credentials, no access control files, no new external actions, no injection vectors.

@github-actions github-actions bot added category:bugfix Auto-managed semantic PR category trust:probationary Building trust, closer scrutiny (auto-managed) labels Apr 9, 2026
Copy link
Copy Markdown
Collaborator Author

Branch codex/fix-missing-submodule-refs has been narrowed to a gitlink-only change. The earlier Windows/setup-script files have been removed from this PR so the diff now contains only:

  • steward-fi: c0883fd007ae834360d7ee183716ce59ed8ee849 -> 3a6b97b138353d30860507fe5c66f058bd7d759a
  • test/contracts/lib/openzeppelin-contracts: 99a8f429774cb8d47de3c9a4186463d829e92956 -> 9cfdccd35350f7bcc585cf2ede08cd04e7f0ec10

Chain of custody for the target SHAs:

  • steward-fi target 3a6b97b138353d30860507fe5c66f058bd7d759a is the current origin/develop tip in the configured submodule remote
  • test/contracts/lib/openzeppelin-contracts target 9cfdccd35350f7bcc585cf2ede08cd04e7f0ec10 is the current origin/master tip in the configured submodule remote

This remains a metadata-only superproject fix for unreachable submodule refs; there is no meaningful code-path regression test to add beyond verifying the target SHAs exist on the configured remotes.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Here is my review for PR #1772:


1. Classification (derived): workflow/CI + supply-chain metadata — consistent with author's claim

2. Rubric (derived): This PR advances two submodule commit pointers; the only dimensions that matter are supply-chain integrity (can the new SHAs be trusted?), correctness of the premise (are the old SHAs actually unreachable?), and whether the wallet-sensitive steward-fi dependency warrants extra scrutiny.

3. Scope verdict: in scope

4. Universal invariants: all intact — the diff only touches gitlink entries, no TS/runtime files modified.

5. Judgment:

  • Needed? — CONCERN: Cannot verify the premise. The claim is that c0883fd0 and 99a8f429 are unreachable, but nothing in this PR (no CI output, no git ls-remote evidence) confirms that. The steward-fi submodule is on branch = develop which can be rebased, making the claim plausible — but plausible ≠ verified.
  • Better than existing? — N/A (metadata-only)
  • Duplication? — OK
  • Blast radius covered? — CONCERN: scripts/init-submodules.mjs has SUBMODULE_READINESS_MARKERS keyed on both paths. If the new commits alter the expected directory layout (e.g. remove packages/api/package.json from steward-fi, or contracts/package.json from OZ), init-submodules will silently loop on every bun install. Not checked.
  • Logic sound? — N/A
  • Complexity appropriate? — OK
  • Tested meaningfully? — CONCERN: No CI evidence that the new SHAs successfully initialize. The fix is unvalidated in CI.
  • Matches conventions? — CONCERN: PR is in DRAFT state and should not be merged as-is.
  • Plausible breakage mode: steward-fi runs as a wallet sidecar with access to tenant credentials and PGLite state (~/.milady/steward/). An attacker who gained write access to Steward-Fi/steward could advance this pointer to a commit that exfiltrates wallet credentials — and reviewers cannot audit the new commit without fetching it. This is the canonical submodule-pointer supply chain vector.

6. PR-type-specific checks:

  • steward-fi new SHA (3a6b97b) is on the develop branch of Steward-Fi/steward — CONCERN: unverified; no git ls-remote evidence provided
  • openzeppelin-contracts new SHA (9cfdccd) is on the official OZ repo — CONCERN: unverified (though risk is lower given OZ's release hygiene)
  • scripts/init-submodules.mjs readiness markers still valid against new commit tree — CONCERN: not checked
  • PR targets develop base — OK

7. Security: Concerns.

The steward-fi submodule is security-sensitive: it is run as a sidecar process managing embedded wallet credentials (packages/app-core/src/services/steward-sidecar.ts). Advancing its commit pointer without an auditable trail of what changed between c0883fd0 and 3a6b97b1 is a supply chain risk from a probationary contributor. Before this can be accepted:

  1. A maintainer with repo access to Steward-Fi/steward must confirm 3a6b97b1 is a legitimate reviewed commit on develop.
  2. The diff between old and new commits should be reviewed for behavioral changes in the API package the sidecar executes.
  3. A CI step validating reachability (git ls-remote | grep <sha>) should be added so future pointer updates are auto-validated.

8. Decision: REQUEST CHANGES

DRAFT status alone blocks merge. Beyond that, two hard blockers: (a) no independent verification the new SHAs are legitimate commits on their configured branches; (b) steward-fi is a wallet sidecar — a supply-chain-sensitive pointer advance from a probationary contributor with no behavioral diff. Required before re-review: un-draft the PR, add CI evidence that both SHAs resolve against their remotes, and link the diff between old and new steward-fi commits so a maintainer can confirm no credential-handling regression.

@github-actions github-actions bot added category:bugfix Auto-managed semantic PR category trust:probationary Building trust, closer scrutiny (auto-managed) and removed trust:probationary Building trust, closer scrutiny (auto-managed) category:bugfix Auto-managed semantic PR category labels Apr 9, 2026
@dutchiono dutchiono force-pushed the codex/fix-missing-submodule-refs branch from c41bfd1 to c89697f Compare April 9, 2026 14:42
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Now I have enough to write a complete review.


1. Classification (derived): submodule pointer update / CI-build — consistent with author's "bugfix" claim, but unsubstantiated

2. Rubric (derived): Supply-chain integrity of new SHAs + validity of the "unreachable commit" claim; no runtime code changed, so standard code-quality axes are N/A.

3. Scope verdict: in scope (submodule/build hygiene is a valid fix category)

4. Universal invariants: all intact — this diff touches only .gitmodules-tracked submodule pointers, no Milady source files.

5. Judgment:

  • Needed? — CONCERN: the claim ("remotes no longer served these SHAs") is unverified and counterintuitive. Neither old SHA has any mechanism that forces a remote to drop it; force-push to a branch tip doesn't delete commits from history — they remain reachable by SHA for at least a grace period. No CI failure log, no git fetch error, no reproduction steps are provided.
  • Better than existing? — CONCERN: can't evaluate without knowing what the new commits contain. The new SHAs could point to anything.
  • Duplication? — OK (N/A, no code involved)
  • Blast radius covered? — CONCERN: The windows-dev-smoke.yml workflow (the one ostensibly broken) does git submodule update --init --depth 1 plugins/plugin-agent-orchestrator only — it never touches steward-fi or test/contracts/lib/openzeppelin-contracts. The causal story doesn't hold. A real Windows failure from these two submodules would need to come from init-submodules.mjs during bun install, which is a different code path and not exercised by the smoke test this PR claims to fix.
  • Logic sound? — CONCERN: not demonstrated. The proof required is: (a) show the old SHAs are genuinely unreachable via git ls-remote against the configured remotes, and (b) show the new SHAs are real, trusted commits on the develop/default branch of each repo (ideally via a signed tag or merge commit ancestry trace).
  • Complexity appropriate? — OK (two-line diff, right size for pointer update)
  • Tested meaningfully? — CONCERN: author explicitly notes "no meaningful code-level regression test" — fair for a pointer change, but there's also no CI run demonstrating the fix works. The PR is still DRAFT; no passing checks are attached.
  • Matches conventions? — CONCERN: PR is DRAFT. Draft PRs are not ready for merge.
  • Plausible breakage mode: New SHA 3a6b97b1 on steward-fi or 9cfdccd3 on openzeppelin-contracts could be an attacker-controlled commit (wrong fork, force-push to a branch tip pointing elsewhere) that gets pulled into developer machines and CI via init-submodules.mjs postinstall, executing a malicious postinstall script with full local filesystem access.

6. PR-type-specific checks (submodule pointer update):

  • New SHAs reachable on the configured remote branches: CONCERN — unverified; no git ls-remote evidence provided
  • Old SHAs actually unreachable (not just "tip changed"): CONCERN — unverified; the claim is implausible without a force-push event on both upstreams simultaneously
  • Windows smoke test exercises the affected submodules: CONCERN — it does not; windows-dev-smoke.yml only initializes plugins/plugin-agent-orchestrator
  • init-submodules.mjs readiness markers still satisfied by new commits: CONCERN — unverified; no check that packages/api/package.json or contracts/package.json exist at the new SHAs
  • No new postinstall scripts introduced by updated submodule content: CONCERN — cannot verify without inspecting the target commits

7. Security: concerns

  • This is a textbook supply-chain vector for a probationary contributor: silently redirect a submodule to an attacker-controlled commit, wait for developers and CI to bun install, execute arbitrary code via the submodule's postinstall scripts.
  • steward-fi is an external DeFi project (Steward-Fi/steward.git) with no direct Milady dependency — its presence as a submodule is already unusual and worth explaining separately, but pointing it to an unverified SHA compounds the risk.
  • openzeppelin-contracts is a widely-cloned, high-value target for supply-chain attacks. Any SHA not traceable to a signed release tag or verified merge commit should be treated with suspicion.

8. Decision: REQUEST CHANGES

Required before this can move forward:

  1. Take out of DRAFT when the fix is ready. A draft PR should not be merged.
  2. Provide evidence old SHAs are unreachable — paste the actual git fetch error or git ls-remote | grep <sha> output showing the SHA is gone from the configured remote.
  3. Prove new SHAs are legitimate — for openzeppelin-contracts, show the SHA is an ancestor of a signed release tag (e.g., git log --oneline v5.x.x | grep 9cfdccd3). For steward-fi, show it's an ancestor of develop on the official remote.
  4. Confirm no malicious postinstall changes — diff the package.json files at old vs. new SHA for both submodules; confirm no new postinstall scripts were added.
  5. Add a CI step that actually validates the fix — e.g., a step in windows-dev-smoke.yml that runs node scripts/init-submodules.mjs so future regressions are caught by the same workflow.

@github-actions github-actions bot added category:bugfix Auto-managed semantic PR category trust:probationary Building trust, closer scrutiny (auto-managed) and removed trust:probationary Building trust, closer scrutiny (auto-managed) category:bugfix Auto-managed semantic PR category labels Apr 9, 2026
@dutchiono
Copy link
Copy Markdown
Collaborator Author

Consolidated into #1774. Closing this PR so the recovery work is reviewed and validated in one place.

@dutchiono dutchiono closed this Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build category:bugfix Auto-managed semantic PR category ci tests trust:probationary Building trust, closer scrutiny (auto-managed)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant