Repository navigation
fix(update): gate the fork-upstream prompt for --yes and non-tty runs - #97052
Conversation
_sync_with_upstream_if_needed called bare input() with no assume_yes parameter and no tty check, so a fork checkout without an upstream remote wedged hermes update forever in any non-interactive context (CI, cron, the desktop updater hand-off): stdin stays open, EOFError never fires. Thread assume_yes and the gateway input_fn into the helper and skip the prompt as a decline under assume_yes or a non-tty stdio pair, without writing the decline marker or touching git remotes, so interactive runs still get asked later. Both call sites forward the interaction state; the config-migration and stash-restore prompts already carry this gate. Closes #60240 (prompt half). Supersedes #78678, #92448, #92410. Co-authored-by: BlackishGreen33 <BlackishGreen33@users.noreply.github.com> Co-authored-by: salch-cred <salch-cred@users.noreply.github.com> Co-authored-by: jackulau <jackulau@users.noreply.github.com>
૮ >ﻌ< ა ci reviewran on b27b6eb — docs(update): --yes help states the fork-upstream prompt is
|
monerostar
left a comment
There was a problem hiding this comment.
Ubuntu 26.04 on a 5800X box (kernel 7.0.0-30-generic). This host runs hermes update from cron, so the hang class is live here.
Piped stdin (isatty False/False) against main 8c098e9:
_sync_with_upstream_if_needed still calls input("Add official repo as 'upstream' remote? [Y/n]: ").
That is the unattended hang.
Same probe on this PR (1451f88):
input() not called, remotes untouched, skip marker not written.
Printed: Skipping upstream setup (non-interactive run).
PR tests: 7 passed in 0.27s (Python 3.11.15 against the PR tree).
Looked at #92448 too. It also skips when stdin is unattended, but then treats that as "n" and calls _mark_skip_upstream_prompt(), so the next interactive update never asks. This PR returns without the marker, and also forwards --yes plus the dashboard input_fn. Prefer this one.
Looks good.
|
monerostar's independent check confirms what I found when closing #92448: my unattended decline persisted the skip marker (via _mark_skip_upstream_prompt), permanently silencing the prompt for interactive users. This PR's non-persisting skip plus the --yes/input_fn threading is the right call. Glad it's the one carrying the fix. |
helix4u
left a comment
There was a problem hiding this comment.
The diagnosis is right, and the overall shape is good: threading assume_yes and input_fn into this prompt fixes the Desktop updater path and ordinary non-TTY automation without changing interactive behavior.
I do see one correctness blocker before merge, though.
On a genuine fork with no upstream remote, where local HEAD matches the user's origin/main but that fork is behind official main, hermes update --yes now does this:
- Fetches the user's fork.
- Finds zero commits between
HEADandorigin/main. - Skips upstream setup because
assume_yes=True. - Never checks official
main. - Falls through to
Already up to date!and exits successfully.
That means an unattended fork can remain stale indefinitely while every update reports success. Once #68959 removes the false-fork cases, genuine forks are the main population left on this path, so this outcome becomes more important.
The --yes help currently says "Assume yes for interactive prompts," so I think the cleanest behavior is to accept the upstream prompt, add the official remote, and continue the existing sync flow, as #78678 did. If permanently adding a remote is not desired, a one-off fetch from the official URL would also preserve the update behavior without changing the configured remotes. At minimum, the helper needs to report that upstream was not checked so the caller cannot claim the checkout is up to date.
Please add a caller-level regression test around _cmd_update_impl, not only direct helper tests: genuine fork, no upstream, HEAD == origin/main, --yes, and official main ahead. The assertion should prove that the update either checks official main or does not report successful Already up to date.
Two smaller scope notes:
- The gateway callback defaults to
"n"on timeout, and that response flows into_mark_skip_upstream_prompt(). An unanswered gateway prompt can therefore permanently silence later interactive prompts, which is the same persistence problem called out in #92448. - This does not fully supersede #92410. A hidden console that still reports TTY/TTY, runs without
--yes, and has nobody present can still block here, and #92410 also covers the separate stash-restore prompt. It is fine to reject that timeout machinery as outside this PR, but the description should call that an intentionally unresolved case rather than complete supersession.
Everything else looks focused and well-tested, and required CI is green. Fixing the successful-stale-fork path is the main thing I would hold the merge on.
…to-date path Review follow-up on #97052 (helix4u): a fork with no upstream remote whose HEAD matches origin/main used to print plain "Already up to date!" under --yes even though official main was never consulted, so an unattended stale fork looked current. _sync_with_upstream_if_needed now returns whether the official upstream was actually checked, and the commit_count == 0 completion line says "Up to date with your fork (official repo not checked)." when it was not. Skip-as-decline semantics are unchanged: no prompt, no remote mutation, no decline marker. Caller-level regression test added for the fork + no-upstream + --yes + HEAD==origin/main path; helper tests now pin the return contract.
…not accepted The old text read as if --yes answers yes to every prompt. It accepts the config-migration and stash-restore prompts but skips the fork-upstream prompt without adding a remote (#97052 review); say so.
|
Thanks for the close read. You're right about the scenario: someone forks the repo, their machine matches their own fork, but the official repo has moved on. With no upstream remote and On making Agreed on both scope points. The PR body now claims to supersede only the prompt-hang part of #92410 and names the gap we knowingly leave (a console that looks interactive, no |
…to-date path Review follow-up on #97052 (helix4u): a fork with no upstream remote whose HEAD matches origin/main used to print plain "Already up to date!" under --yes even though official main was never consulted, so an unattended stale fork looked current. _sync_with_upstream_if_needed now returns whether the official upstream was actually checked, and the commit_count == 0 completion line says "Up to date with your fork (official repo not checked)." when it was not. Skip-as-decline semantics are unchanged: no prompt, no remote mutation, no decline marker. Caller-level regression test added for the fork + no-upstream + --yes + HEAD==origin/main path; helper tests now pin the return contract.
The pre-NousResearch#97052 prompt wedge fires AFTER the updater has already moved the checkout (stash cleanup, reset, bootstrap refresh all precede the upstream-remote prompt), so checkout movement cannot distinguish a wedged updater from a working one. Two CI rides confirmed the check never fires. The 35-minute wait bound already caps these legs.
…to-date path Review follow-up on NousResearch#97052 (helix4u): a fork with no upstream remote whose HEAD matches origin/main used to print plain "Already up to date!" under --yes even though official main was never consulted, so an unattended stale fork looked current. _sync_with_upstream_if_needed now returns whether the official upstream was actually checked, and the commit_count == 0 completion line says "Up to date with your fork (official repo not checked)." when it was not. Skip-as-decline semantics are unchanged: no prompt, no remote mutation, no decline marker. Caller-level regression test added for the fork + no-upstream + --yes + HEAD==origin/main path; helper tests now pin the return contract.
…not accepted The old text read as if --yes answers yes to every prompt. It accepts the config-migration and stash-restore prompts but skips the fork-upstream prompt without adding a remote (NousResearch#97052 review); say so.
Fixes the prompt half of #60240. Supersedes #78678 and #92448, and the fork-upstream prompt-hang portion of #92410.
What happens
On a fork checkout of main with no
upstreamremote,hermes updateasks "Add official repo as 'upstream' remote? [Y/n]:" through a bareinput(). In any non-interactive context (CI, cron, the desktop updater hand-off) stdin is open but nobody answers, so the call blocks forever:--yesnever reaches this prompt and the EOFError fallback only fires when stdin is closed. Observed as a 90-minute hang in a Windows open-app-update E2E leg, with the update marker never cleaned and the updater killed at teardown.The fix
_sync_with_upstream_if_needednow takesassume_yesand the gatewayinput_fn, and both call sites in_cmd_update_implforward them. Underassume_yes, or when there is noinput_fnand stdio is not a TTY pair, the prompt is skipped as a decline without writing the decline marker and without touching git remotes, so interactive users still get asked on their next update. Gateway updates route the question through the existing IPC prompt callback with a default of no. Interactive terminal behavior is unchanged.This is the same gate the file already applies to its config-migration and stash-restore prompts; this prompt was the one that never got it.
--yesdeliberately declines rather than adds the remote. Adding a remote is a repo mutation, and--yesin this command means "don't block on prompts", not "make git changes I didn't ask for". The add-on-yes alternative (what #78678 implemented) was considered and rejected on that ground.Relationship to the superseded PRs
#78678 (@BlackishGreen33) had the right structure (thread
assume_yes+input_fn, non-persisting skip) but made--yesadd the remote, and also bundled the_is_forkcase-sensitivity fix that #68959 covers on its own. #92448 (@salch-cred) had the right diagnosis but its unattended decline flows into the branch that persists the skip marker, permanently silencing the prompt for interactive users, and its TTY guard alone cannot cover the hidden-console case that--yesthreading does. #92410 (@jackulau) bounds every prompt with reader threads; withassume_yesand the TTY gate in place the residual population (interactive-looking console, no--yes, nobody present) does not justify the thread machinery for this prompt. That case is intentionally left unresolved here rather than fully superseded, and #92410's timeout bounding of the other prompts is likewise out of scope. All three authors are credited on the commit.The
_is_forkURL-normalization half of #60240 stays with #68959, which already covers more URL forms.Tests
tests/hermes_cli/test_update_upstream_prompt_noninteractive.pypins the gate:assume_yesand every non-TTY stdio combination return without callinginput(), without touching remotes and without persisting the decline; the gateway path routes throughinput_fnand keeps its decline persistence; interactive accept and decline keep today's behavior. The suite fails on main (5/7, including TypeError on the new signature) and passes with the fix. Neighboring suitestest_cmd_update.py(38) andtest_update_yes_flag.py+test_update_autostash.py(52) pass; one assertion intest_cmd_update.pyupdated for the new call signature.Review follow-up (@helix4u)
On a genuine fork with no
upstreamremote whoseHEADmatches the user's ownorigin/mainwhile official main is ahead, the skip left the update printing plain "Already up to date!" even though the official repo was never consulted._sync_with_upstream_if_needednow returns whether official upstream was actually checked, and that completion line reads "Up to date with your fork (official repo not checked)." when it was not. Skip semantics are unchanged: no prompt, no remote mutation, no decline marker. A caller-level regression test through_cmd_update_implpins the path, and the--yeshelp text now states that the fork-upstream prompt is skipped rather than accepted.End-to-end verification
Verified on the end-to-end path the hang was found on: a Windows desktop-updater hand-off leg (install old build, click Update now in the app, detached updater runs
hermes update --yes --gateway --force --branch mainwith an open unattended stdin, on a checkout that fork-detection classifies as a fork with no upstream remote). With a pre-fix baseline the leg wedges atAdd official repo as 'upstream' remote? [Y/n]:until an external kill, and the update marker is never cleaned. With the fix in the running code the same leg logsSkipping upstream setup (non-interactive run)., the update exits 0 on the first attempt, the marker is cleaned, and the app relaunches. One caveat for anyone watching similar CI: an install whose baseline predates this fix still wedges once, because its first update runs the old code in memory; the fix applies from the next update on.