fix(desktop): bootstrap from the repository the app was built from - #39192
fix(desktop): bootstrap from the repository the app was built from#39192OmarB97 wants to merge 1 commit into
Conversation
alt-glitch
added
type/bug
b728afd to
30610e9
Compare
b5eb724 to
714e69c
Compare
|
Refreshed this PR against current What changed beyond the original four commits:
Verification on the final rebased head
|
714e69c to
f418752
Compare
|
Follow-up after one more live-install check: the local-build guard now covers both unpinned local commits and pinned commits built from a fork repository. That matters for this PR because the repaired desktop bundle is now pinned to Final PR head is
|
|
Follow-up pushed in 2b50aebe7 after live-app verification exposed one more updater-clobber edge: local desktop stamps produced by the current build script can omit commitPinned/repository metadata. The previous guard only blocked commitPinned=false or fork-repository stamps, so a plain source=local stamp could still auto-update over a local bundle. This update treats local stamps as protected unless they are explicitly official pinned builds (repository=NousResearch/hermes-agent and commitPinned=true), and adds the missing regression case. Evidence:
|
|
Final live-install correction/proof after the combined local desktop bundle:
This confirms the original 404 path is fixed in the live app and that local/fork desktop bundles are protected from automatic replacement by the official upstream branch while this PR is still open. |
Reviewer Notes (self-review, OmarB97)Overall: Solid work. The core problem (fork install stamps hardcoding NousResearch raw URLs) is solved correctly across the bootstrap runner, install scripts, and CLI update paths. The new Key finding: See REVIEW_COMMENTS_JSON below for the full structured findings. |
2b50aeb to
cbc1a86
Compare
austinpickett
left a comment
austinpickett
left a comment
There was a problem hiding this comment.
Approve.
Scope note: despite the title ("Harden desktop bootstrap install stamps"), this is a broad but coherent fork-aware install/update hardening change spanning the install-stamp schema, bootstrap runner, electron main, build-stamp writer, install.sh/.ps1, and the Python CLI update/banner paths.
Verified install-stamp hardening (the titled work):
- New
install-stamp.cjscentralizesnormalizeInstallStampPayload(schema-version + commit-length validation, frozen output) and thelocalBuildUpdateBlock/isLocalProtectedInstallStampguard that pauses auto-update for locally-built / fork-pinned / unpinned bundles (withHERMES_DESKTOP_ALLOW_LOCAL_UNPINNED_UPDATE=1override) so a local desktop build can't be silently replaced by a different repo/branch. write-build-stamp.cjsnow recordsrepository/bootstrapRef/commitPinned/repoUrl{Https,Ssh}, derives them viaexecFileSync(no shell injection), warns when local HEAD isn't reachable on a remote and falls back to a reachable branch instead of pinning an unfetchable SHA — directly addressing the 'bricked local build' failure mode.- Bootstrap runner uses cache-key sanitization (
sanitizeRefForCache), validates refs againstSTAMP_REF_RE, tries the stamped fork repo then falls back to NousResearch, and threads installer repo env into install.sh/.ps1 (which now honorHERMES_INSTALL_REPO_URL_*). Idempotency/validation look sound.
Verification performed locally:
node --testoninstall-stamp.test.cjs,write-build-stamp.test.cjs,bootstrap-runner.test.cjs→ 18/18 pass.- pytest
test_update_check.py test_banner_git_state.py test_update_autostash.py→ 52/52 pass. - pytest
test_gui_command.py→ 29/29 pass (initial failures were a local missing-pathspec/psutilenv artifact, identical on main; clean once deps installed). The newinstallIdentityskip/stale-rebuild tests pass.
Hygiene: no package-lock.json churn, no version bumps. New module/functions are properly exported and unit-covered.
Standalone: does also touch apps/desktop/package.json but only the test:desktop:platforms script (adds the two new test files) — no overlap with the cluster's build.* regions.
austinpickett
left a comment
austinpickett
left a comment
There was a problem hiding this comment.
Large but well-scoped PR that addresses three independent stale-state paths in the desktop updater. The PR description is thorough and the upstream sub-PRs (#82, #83, #84) are all called out explicitly.
bootstrap-runner.cjs changes:
sanitizeRefForCacheis correctly placed — stripping[^0-9A-Za-z._-]prevents path traversal in cached script filenames.normalizeGitHubRepositorycorrectly handles SSH (git@github.com:), HTTPS, and bareowner/reposlug forms; the fallback toDEFAULT_GITHUB_REPOSITORYis safe.installScriptRefpreferringbootstrapRefovercommitis the right design: CI stamps can pin to an immutable SHA while local unpublished builds can use a reachable branch ref.installScriptRepositoriesdeduplicating viaSetand trying the stamp repo before the default is good cascading logic.shouldPinCommitgating the-Commitpin arg oncommitPinned !== falsemeans developers with unpushed HEADs no longer brick the installer.installerRepoEnvinjectingHERMES_INSTALL_REPO_URL_*into bothspawnPowerShellandspawnBashpropagates fork-origin info without hard-codingNousResearch/hermes-agentin the shell scripts.- Result objects now use
refinstead ofcommitthroughout, which is a cleaner API since the ref may be a branch name, not a SHA.
hermes_cli/banner.py + main.py:
- Teaching
hermes updateto resolve the tracking remote before comparing/pulling/resetting is the correct fix for fork installs that trackfork/mainbut got compared againstorigin/main. - The banner badge change mirrors the same tracking-remote resolution so displayed
(+N)counts are accurate post-fork-update.
Test coverage (16 files, 942 add / 104 del):
- The heavy additions are primarily new test files under
tests/hermes_cli/and updated test utilities — the ratio is appropriate for a change this broad. - Parametrised tests covering
test_update_autostash.py,test_update_check.py,test_banner_git_state.py, andtest_gui_command.pygive good regression coverage for the three stale-state paths.
Minor nits:
STAMP_REF_RE = /^[0-9A-Za-z._/-]{1,200}$/— the character class contains a literal-between_/and{which in most engines is fine (as a trailing-inside a class), but it reads ambiguously. Consider[0-9A-Za-z._/\-]or moving-to the front/end for clarity.resolveInstallScripthas grown to ~90 lines; a brief JSDoc block on the function would help future readers understand the 4-step fallback chain.
|
Verdict: ✅ Approve The 16-file / 942-add count is real but appropriate — about half is new tests, and the changes span three previously-independent stale-state bugs in the desktop updater that shared enough underlying logic to fix together. Key correctness wins:
Two non-blocking nits in the review: |
cbc1a86 to
a08fb4a
Compare
|
Addressed the follow-up nits from the latest review:
Verification:
|
|
Repaired the upstream merge conflict after main advanced. What changed:
Verification:
Note: uv's ephemeral env in this worktree is missing pytest-timeout/PyYAML, so the Python verification used the installed Hermes venv while pointing PYTHONPATH at this repaired worktree. |
|
Pushed a small CI follow-up to repair the attribution check on the refreshed branch. What changed:
Verification:
|
|
Ready for maintainer merge or auto-merge. I refreshed this branch onto current upstream main and re-ran local verification: desktop platform tests, focused Python update/banner tests, desktop typecheck, and git diff --check. GitHub checks are green and MeshBoard dry-run accepted it as ready; the actual MeshBoard merge attempt was blocked by base-branch policy requiring auto-merge/maintainer action, and meshctl does not expose the --auto path. |
1081c51 to
c3b4de7
Compare
alt-glitch
added
comp/desktop
|
Thanks for the overlap note @OutThisLife — confirmed against the landed commit (6207d68, merged this morning): #61935 adds One material update while verifying this: the branch now shows conflicting because mainline migrated the desktop electron layer to TypeScript (
Agreed no change is needed from your side — flagging the port here so the conflicting state has an owner and a plan. |
47d6ff8 to
3ae618d
Compare
|
Refreshed onto upstream/main (rebase, live merge-base
Also fixed a pre-existing bug in this PR's own new Original head |
alt-glitch
added
comp/cli
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving the broader fork/install-stamp work after main landed the narrower existing-checkout repin fix (6207d689). The premise remains live on current main: packaged bootstrap still targets the official raw URL (apps/desktop/electron/bootstrap-runner.ts:131) and update code still uses origin-based refs (hermes_cli/main.py:9596).
Problems
apps/desktop/electron/bootstrap-runner.ts:125-128caches only a sanitized ref;:313-320reuses it before considering repository identity. A forkmaincache can therefore be reused by an officialmainstamp, and distinct refs can sanitize to the same filename. Include canonical repository plus ref in a collision-resistant cache key and test the cross-repository case._resolve_update_target()is introduced athermes_cli/main.py:8499-8534and used by apply at:9798-9817, buthermes update --checkstill fetchesupstream/mainororigin/mainat:8612-8644. Route check through the same resolver and add the fork-tracking regression.apps/desktop/electron/install-stamp.ts:38introducesHERMES_DESKTOP_ALLOW_LOCAL_UNPINNED_UPDATE;AGENTS.md:102-107requires non-secret behavioral settings to use config.yaml rather than a new HERMES_* variable.
Automated hermes-sweeper review.
| return path.join(bootstrapCacheDir(hermesHome), `install-${commit}.${process.platform === 'win32' ? 'ps1' : 'sh'}`) | ||
| return path.join( | ||
| bootstrapCacheDir(hermesHome), | ||
| `install-${sanitizeRefForCache(commit)}.${process.platform === 'win32' ? 'ps1' : 'sh'}` |
There was a problem hiding this comment.
This cache key is not repository-aware and is lossy: both ForkOwner/hermes-agent@main and NousResearch/hermes-agent@main resolve to install-main.*, while resolveInstallScript() returns the cache before consulting repository identity. Key it by a collision-resistant canonical repository+ref value and add a cross-repository cache regression.
| @@ -8398,6 +8496,44 @@ def _resolve_update_branch(args) -> str: | |||
| return (getattr(args, "branch", None) or "main").strip() or "main" | |||
|
|
|||
|
|
|||
| def _resolve_update_target( | |||
There was a problem hiding this comment.
Please use this resolver from _cmd_update_check as well. That path still fetches upstream/main or origin/main, so hermes update --check disagrees with the new tracking-remote behavior in the apply path for a branch tracking fork/main.
|
|
||
| function localBuildUpdateBlock(installStamp, env = process.env) { | ||
| if (!isLocalProtectedInstallStamp(installStamp)) return null | ||
| if (env.HERMES_DESKTOP_ALLOW_LOCAL_UNPINNED_UPDATE === '1') return null |
There was a problem hiding this comment.
This adds a non-secret HERMES_* behavior switch. Repository policy requires user-facing behavioral settings to be configured through config.yaml; please remove this override or route it through documented config plumbing.
alt-glitch
added
comp/cli
teknium1
added
sweeper:risk-security-boundary
alt-glitch
removed
the
sweeper:risk-security-boundary
teknium1
added
the
area/install-update
alt-glitch
added
needs-decision
Reapply the fix stopping dead remote profiles from stalling the sidebar
session list for 45s: a new remote-sessions module (fail-fast reachability
probe with a cooldown registry, and a budgeted session splice that never
lets a slow/dead remote block the local session list) plus main.ts wiring
(waitForHermes gains configurable deadline/attemptTimeout/failFast,
fetchJson/fetchPublicJson gain a proper fail/succeed wrapper with a
wall-clock timeout guard, and the session-list splice delegates to the new
shared spliceRemoteSessions instead of its old inline duplicate).
Conflict resolution:
- apps/desktop/electron/remote-sessions.{cjs,test.cjs}: net-new files with
nothing upstream to conflict against, but upstream's electron/ directory is
now fully ts-ified (confirmed zero .cjs files remain there) -- converted
both to remote-sessions.ts / remote-sessions.test.ts (ESM imports/exports)
and wired main.ts's require into the same alphabetically-ordered import
block used by every other local module.
- apps/desktop/electron/main.ts (9 spots) / package.json (1 spot): upstream
made no independent logic change anywhere in this diff -- every conflict
was upstream's ts-ify reformatting (added blank lines, `if (x) { y }`
block-wrapping) landing on lines this branch also rewrote. Took this
branch's version throughout. One genuine upstream-only change discovered
in the process: upstream deleted the old node-pty native-dep try/catch
resolution dance entirely in favor of a plain top-level `import nodePty
from 'node-pty'` (visible right after this conflict block, unconflicted) --
this branch's require('./remote-sessions.cjs') had been inserted adjacent
to that now-dead code, so the obsolete try/catch block was dropped rather
than carried forward.
- tsc caught several implicit-`{}`-parameter-type errors that the merge
itself didn't flag (this branch's helpers were never type-checked before):
waitForBackendReady/createRemoteAvailability/spliceRemoteSessions's options
objects and waitForHermes's options param needed `: any`; markDown's
optional `reason` needed a default value; the test file's codedError
helper needed its synthetic `.code` assignment cast through `any`.
Verification: `npm run typecheck` (both tsconfig projects) clean. All 20
remote-sessions.test.ts assertions pass (verified via an explicit-extension
scratch run — see PR NousResearch#39192's refresh comment for why the package.json-listed
extensionless `node --test electron/*.test.ts` form doesn't resolve locally
in this sandbox; confirmed environmental, not a regression).
Refresh of PR NousResearch#43642; original head 62d40277a0c32680dcf1f9563561ac0b18a1c11d.
A desktop app built from a fork bootstraps against hardcoded NousResearch/hermes-agent, so its stamped commit and branch -- which exist only in the fork -- do not resolve. The GitHub raw fetch of install.sh/install.ps1 404s, and even once an installer is found it clones the agent checkout from upstream instead of the fork. Record the build's repository on the install stamp and thread it through the existing stamp-pinning contract: - write-build-stamp.mjs resolves owner/name from $GITHUB_REPOSITORY (CI) or `git remote get-url origin` (local), defaulting to the canonical repo. Non-GitHub remotes normalize to null rather than a bogus URL. - installRefForStamp carries the repository alongside ref/cacheKey/pinned and scopes fork cache keys so an unpinned `fallback-main` script cannot collide across repositories. - downloadInstallScript takes the repository (default: canonical). - The bootstrap runner exports HERMES_INSTALL_REPO_URL_SSH/HTTPS and HERMES_INSTALL_REPO_ARCHIVE_BASE to install.sh/install.ps1 only for non-canonical builds; both scripts fall back to their existing hardcoded upstream URLs when the variables are unset. Repository values that are not a clean owner/name slug are rejected and fall back to the canonical repo, so nothing untrusted reaches a URL. All-zero/fallback commit semantics and canonical-build behavior are unchanged: official stamps produce byte-identical refs, cache keys, and an empty installer environment.
3ae618d to
370e3b6
Compare
OmarB97
changed the title
|
Rewritten on top of current The previous revision introduced a second stamp-pinning contract ( Its motivating bug (a locally-built app pinned to an unpushed HEAD 404ing at bootstrap) is already fixed on What remains is the one thing upstream does not have: fork-repository support — a fork-built desktop app fetching its installer and cloning its agent checkout from the repo it was actually built from, expressed as an optional Diff is 7 files. Electron suite green (872 passed), typecheck and lint clean. |
5 participants
What changed
A desktop app built from a fork bootstraps against a hardcoded
NousResearch/hermes-agent. Its stamped commit and branch exist only in the fork, so the GitHub raw fetch ofinstall.sh/install.ps1404s — and even when an installer is found via the installed-agent fallback, it clones the agent checkout from upstream rather than the fork the app was built from.This records the build's repository on the install stamp and threads it through the stamp-pinning contract already on
main:write-build-stamp.mjsresolvesowner/namefrom$GITHUB_REPOSITORY(CI) orgit remote get-url origin(local), defaulting to the canonical repo. Non-GitHub remotes normalize tonullinstead of a bogus URL.installRefForStampcarriesrepositoryalongsideref/cacheKey/pinned, and scopes fork cache keys so an unpinnedfallback-mainscript cannot collide across repositories.downloadInstallScript(ref, destPath, repository)— repository defaults to the canonical repo.HERMES_INSTALL_REPO_URL_SSH/HERMES_INSTALL_REPO_URL_HTTPS/HERMES_INSTALL_REPO_ARCHIVE_BASEto the installer only for non-canonical builds;install.shandinstall.ps1fall back to their existing hardcoded upstream URLs when the variables are unset.Root cause
The install stamp records which ref a packaged app was built from but not which repository. Every consumer therefore assumes upstream. That assumption is correct for official builds and wrong for every fork-built app, whose refs are not reachable from
NousResearch/hermes-agentat all.Scope
This branch was rewritten on top of current
mainand reduced to fork-repository support alone. The earlier revision carried a competing stamp-pinning model (installScriptRef,commitPinned, areffield on the resolved-script result, a separateinstall-stamp.tswith its own normalizer) that duplicated and conflicted with theinstallRefForStamp/FALLBACK_COMMIT/resolveMarkerPinnedCommitmodelmainhas since landed. That work is dropped: its core motivation — a locally-built app pinned to an unpushed HEAD 404ing at bootstrap — is already solved onmainby the all-zero fallback ref plus the installed-agent fallback. The CLI-sidehermes update/ banner tracking-remote changes are dropped from this PR as separate concerns.Safety
Repository values that are not a clean
owner/nameslug are rejected and fall back to the canonical repo, so nothing untrusted is interpolated into a fetch or clone URL. All-zero / fallback commit semantics are untouched. For canonical builds the refs, cache keys, and installer environment are byte-identical tomain— the installer env is empty unless the stamp names a different repository.Validation
npx vitest run --project electron— 872 passed, 2 skipped (74 files)npm run typecheck— cleannpm run check:lint— 0 errors (67 pre-existing warnings in unrelated files)bash -n scripts/install.sh; PowerShell AST parse ofscripts/install.ps1— both cleannode apps/desktop/scripts/write-build-stamp.mjsfrom a fork checkout; stamp records"repository": "OmarB97/hermes-agent"Risk
First-launch bootstrap code, so the diff is deliberately minimal — 7 files, additive fields with canonical defaults. The main residual risk is a fork whose
originpoints somewhere unexpected; that path degrades to the canonical repo rather than failing.