Conversation
Merges main into help50 branch
dmalan
marked this pull request as ready for review
October 5, 2024 23:51
This comment was marked as off-topic.
This comment was marked as off-topic.
Contributor
Thanks for the reminder. How did you perform the test? |
- cli.sh: only start help50 in interactive shells with a terminal, else non-interactive login shells (bash --login -c) hang on script - lib/cli: _ansi and _fold take arguments by $#, not -t 0, so messages aren't dropped when stdin is redirected; _fold falls back to 80 columns - valgrind: source lib and use _alert/_ansi instead of undefined _help - help50.sh: fix _rhetocial typo; cap _helpless payload at 8 KiB - help50/python: handle python dir/file.py, use realpath --canonicalize-missing - Dockerfile: install bsdextrautils explicitly for col
tests/smoke.sh checks a built image under timeouts: non-interactive login shells exit, help50 deps are installed, wrappers print with stdin redirected. Run via make smoke, and in CI before pushing to Docker Hub.
Fix help50 bugs and add smoke test
This was referenced Sep 21, 2026
script records everything on the pty, including tab-completion listings, history recall, and redrawn prompts. _help50 dropped only the first line as the command, so after tab-completing a command with no output, the leftover echo was treated as its output and passed to _helpless. Now drop everything through the first line that ends with the command as recorded in history, joining backslash continuations and their PS2 prompts; fall back to dropping the first logical line if the command isn't found.
Ignore terminal echo before the command line in typescripts
The typescript was capped with head -n 1024, keeping the start of the output. Errors are usually at the end (tracebacks, make: *** Error, segfaults), so a program that printed a lot and then failed lost its error before any helper or _helpless saw it. Now keep the first 64 lines (where the command line is echoed and found) plus the last 1024, with a marker for what was omitted. _helpless now also receives the command line as a second argument, so whatever explains the output (in cs50/codespace, the CS50 Duck via cs50.ai) can see what was run. The default _helpless ignores it; output stays the first argument, so the codespace's empty-output check is unaffected.
Nothing exercised _help50 itself: the head/tail cap and the new command-line argument were only checked by hand under a pty. Drive _help50 directly in the image instead, with a fabricated typescript and a fake _helpless, and assert that a command printing 3000 lines then an error yields the command line as the second argument and, as the first, output that starts at the program's first line, carries the omission marker, drops the middle, and ends with the error. Also check that a failed command with no output yields an empty first argument, which is what the codespace's empty-output check depends on. The check fails against the previous help50.sh.
Keep the end of long output, and pass the command line to _helpless
Students used to run `help50 make foo`. Help now arrives automatically after any failed command, so run COMMAND with the same exit status and let the prompt hook handle the rest: help50 make foo behaves exactly like make foo. Builtins go through bash -c with the shell's error wording preserved; an unknown command fails with the shell's own "command not found" message so helpers match it. Bare help50 prints usage (exit 0) explaining the new behaviour. The subcommands (start/stop/...) are unchanged; only they require non-root.
Within a help50 session, define help50 as a shell function that evals COMMAND in the calling shell, so aliases (rm -i), functions, and cd behave exactly as they would directly, and no stderr is rewritten through an unwaited sed. In the script (outside a session), route path-shaped names (./foo.c, ./dir) through bash -c so the shell's own Permission denied / Is a directory errors reach the helpers instead of a synthesized "command not found"; wait for sed before exiting; pass -- to type so option-like commands don't leak usage noise. In the prompt hook, treat `help50 COMMAND` as COMMAND when deriving argv, so helpers that look at positional words (e.g., `check 50`) and the ./ re-make hint still fire. Smoke-test the passthrough: exit statuses, exact error text for unknown, option-like, builtin, and path-shaped commands, usage, sudo, the in-shell function, and the hook's argv handling.
Run `help50 COMMAND` as though COMMAND were typed directly
- Read the typescript bounded (first 64K + last 1M) instead of the whole file into a variable, so the prompt after a failed command no longer scales with how much it printed: 35 MB took 2.4 s, now 0.1 s, and 350 MB would have taken 24 s. - Run each helper under timeout (5 s, then SIGKILL), so a slow or stuck helper cannot stall the prompt; today's helpers can't block, but the framework accepts helpers in any language. - HELP50_DISABLED in the environment disables help50 at login and is reported by help50 is-enabled/status. Set as an organization-wide Codespaces secret, it turns help50 off for everyone at their next login without rebuilding an image; set by one user, it's a persistent personal opt-out. Smoke tests cover all three.
- HELP50_DISABLED=0 (or false, no, off, case-insensitively) now counts as unset, so that an admin who sets the org secret to 0 to turn help50 back on gets what they asked for, rather than every student staying disabled with no error. The is-enabled message now shows the value and says to unset it. Smoke tests cover the false-y values and that the lock file is still honored when the environment doesn't disable. - Note in the prompt hook that timeout runs each helper in its own process group, so ctl-c no longer reaches a stuck helper; the timeout itself is the bound. --foreground would restore ctl-c but stop timeout from killing the helper's children, which would give back the hang this is meant to remove.
Bound the prompt hook's cost, and add a kill switch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To be squashed when merging.
This new version is implemented in Bash (instead of Python) as follows, wherein usage is inspired by
systemctl, even though it doesn't run as a daemon but, rather, per login shell. It runs locally and automatically now, without any server.help50 startsets$HELP50to$$, the PID of the shell in which the command was run and launchesscript, which logs standard I/O to/tmp/help50.$$.typescript.help50 stopsendsSIGHUPto the PID ofscript.help50 statuschecks for$HELP50, which is set only whenhelp50is started for a shell.help50 disablewrites/tmp/help50.lock.help50 enabledeletes/tmp/help50.lock.help50 is-enabledchecks for/tmp/help50.lock./etc/profile.d/help50.shis a config that that's only sourced when$HELP50is set.$PROMPT_COMMANDto_help50, which is a Bash function implemented therein that, if the most recent command exited with non-0 status (per$?), checks for/tmp/help50.$HELP50.typescript, passes it as standard input to each executable in/opt/cs50/lib/help50/(implemented in any language)._helpfulfunction that, by default, displays it in yellow to help the user._helplessinstead, which doesn't do anything incs50/clibut can be overridden incs50/codespaceto relay it to ddb50._helpedis called, which doesn't do anything incs50/clibut can be overridden incs50/codespaceto indicate to the user that help is (no longer) available./etc/profile.d/cli.shstartshelp50automatically./opt/cs50/lib/clicontains several helper functions (written in Bash) that our own wrappers andhelp50use.