diff --git a/cli/src/internal/execution/shellSession.ts b/cli/src/internal/execution/shellSession.ts index f8e75e85..78f67e1b 100644 --- a/cli/src/internal/execution/shellSession.ts +++ b/cli/src/internal/execution/shellSession.ts @@ -70,10 +70,26 @@ if [ -n "$__ql_timeout" ]; then fi if [ "$__ql_command" = true ]; then "\${__ql_args[@]}" "\${__ql_script_args[@]}" + _task_exit_code=$? else - . "\${__ql_args[0]}" "\${__ql_script_args[@]}" + # Match task.sh: keep script exit/exec/options inside a child shell while + # retaining before-hook variables and functions for the script. + # Keep the supervisor alive until the child's signal/EXIT traps finish, but + # do not turn user cancellation into an ordinary completion with after hooks. + __ql_saved_signals=$(trap -p INT TERM HUP QUIT ALRM TSTP) + __ql_cancel_code='' + trap '__ql_cancel_code=$((128 + $(kill -l INT)))' INT + trap '__ql_cancel_code=$((128 + $(kill -l TERM)))' TERM + trap '__ql_cancel_code=$((128 + $(kill -l HUP)))' HUP + trap '__ql_cancel_code=$((128 + $(kill -l QUIT)))' QUIT + trap '__ql_cancel_code=$((128 + $(kill -l ALRM)))' ALRM + trap '__ql_cancel_code=$((128 + $(kill -l TSTP)))' TSTP + ( . "\${__ql_args[0]}" "\${__ql_script_args[@]}" ) + _task_exit_code=$? + trap - INT TERM HUP QUIT ALRM TSTP + eval "$__ql_saved_signals" + if [ -n "$__ql_cancel_code" ] && ! [ -f "$__ql_timeout" ]; then exit "$__ql_cancel_code"; fi fi -_task_exit_code=$? if [ -n "$__ql_timeout" ] && [ -f "$__ql_timeout" ]; then _task_exit_code=124; fi __ql_run_after exit "$_task_exit_code" diff --git a/cli/test/integration/differential.test.cjs b/cli/test/integration/differential.test.cjs index 842f710c..e6bb8b50 100644 --- a/cli/test/integration/differential.test.cjs +++ b/cli/test/integration/differential.test.cjs @@ -7,7 +7,7 @@ const { execFileSync, spawnSync } = require('node:child_process'); const { createContext, sourceEnvironment } = require('../../dist/internal/runtime/context'); const { executeTask } = require('../../dist/internal/execution/taskRunner'); -test('unmodified legacy Shell and TS agree on shell hook state and account modes', async (t) => { +test('Shell and CLI agree on isolated script state and account modes', async (t) => { const root = await fs.realpath( await fs.mkdtemp(path.join(os.tmpdir(), 'ql-differential-')), ); @@ -110,13 +110,13 @@ test('unmodified legacy Shell and TS agree on shell hook state and account modes async () => { const cases = [ { name: 'return', script: 'return 7', code: 7, legacy: 0, after: true }, - { name: 'exit', script: 'exit 7', code: 7, legacy: 7, after: false }, + { name: 'exit', script: 'exit 7', code: 7, legacy: 0, after: true }, { name: 'errexit', script: 'set -e; false', code: 1, - legacy: 1, - after: false, + legacy: 0, + after: true, }, { name: 'trap-return', @@ -130,8 +130,8 @@ test('unmodified legacy Shell and TS agree on shell hook state and account modes name: 'trap-exit', script: 'trap \'printf "exit-trap\\n" >> "$TRACE"\' EXIT; exit 7', code: 7, - legacy: 7, - after: false, + legacy: 0, + after: true, trap: true, }, ]; @@ -174,8 +174,8 @@ test('unmodified legacy Shell and TS agree on shell hook state and account modes assert.equal(result.exitCode, item.code, item.name); const expected = [ 'before', - ...(item.after ? [`after:${item.code}`] : []), ...(item.trap ? ['exit-trap'] : []), + ...(item.after ? [`after:${item.code}`] : []), ]; for (const file of [legacyTrace, modernTrace]) assert.deepEqual( diff --git a/cli/test/internal/local.test.cjs b/cli/test/internal/local.test.cjs index 8dc6977b..4d4438dc 100644 --- a/cli/test/internal/local.test.cjs +++ b/cli/test/internal/local.test.cjs @@ -445,7 +445,7 @@ test('hook exports reach scripts and after hooks share shell state with inline c assert.match(log, /hook:before:inline:after:7/); }); -test('ordinary shell tasks share unexported variables and functions across before script and after hooks', async (t) => { +test('shell tasks inherit before-hook state without leaking script state into after hooks', async (t) => { const { executeTask } = require('../../dist/internal/execution/taskRunner'); const root = sandbox(t); const context = createContext( @@ -464,7 +464,7 @@ test('ordinary shell tasks share unexported variables and functions across befor ); fs.writeFileSync( context.paths.file_task_after, - 'script_function\nprintf "after:%s:%s:%s\\n" "$before_value" "$script_value" "$_task_exit_code"\n', + 'if declare -F script_function >/dev/null; then exit 99; fi\nprintf "after:%s:%s:%s\\n" "$before_value" "${script_value-unset}" "$_task_exit_code"\n', ); const result = await executeTask(context, { argv: ['shared.sh'], @@ -478,7 +478,7 @@ test('ordinary shell tasks share unexported variables and functions across befor ); assert.match( log, - /before:private\narg:space value\nscript:changed\nafter:private:changed:7/, + /before:private\narg:space value\nafter:private:unset:7/, ); }); @@ -681,7 +681,7 @@ test('designated shell accounts retain hook functions and script state in a shar path.join(context.paths.dir_log, result.logPath), 'utf8', ); - assert.match(log, /selected:third&first\nafter:third&first:from_script:4/); + assert.match(log, /selected:third&first\nafter:third&first::4/); }); test('lifecycle reports share execution identity and statistics survive a rejected final status', async (t) => { diff --git a/cli/test/internal/taskLifecycle.test.cjs b/cli/test/internal/taskLifecycle.test.cjs index 28051afb..3c8e8d1d 100644 --- a/cli/test/internal/taskLifecycle.test.cjs +++ b/cli/test/internal/taskLifecycle.test.cjs @@ -27,13 +27,14 @@ test('shared Shell lifecycle scopes timeouts to the task and runs cleanup exactl { name: 'failure', task: 'return 7', code: 7 }, { name: 'function timeout preserves state', task: 'STATE=changed; sleep 10', code: 124, timeout: true, state: 'changed' }, { name: 'external timeout', args: ['sleep', '10'], code: 124, timeout: true }, - { name: 'Shell script timeout preserves state', task: 'STATE=changed; sleep 10', shell: true, code: 124, timeout: true, state: 'changed' }, + { name: 'Shell script timeout preserves parent hook state', task: 'STATE=changed; sleep 10', shell: true, code: 124, timeout: true }, { name: 'slow before is outside task timeout', before: 'sleep 0.2', task: 'return 0', code: 0 }, { name: 'slow after is outside task timeout', after: 'sleep 0.2', task: 'return 0', code: 0 }, { name: 'background child cannot hold pipes after timeout cleanup', task: '(trap "" INT; sleep 30) & sleep 30', after: 'sleep 0.3', code: 124, timeout: true }, { name: 'script may redirect fd 3', shell: true, task: 'exec 3> "$TRACE.fd"; return 0', after: 'sleep 0.3', code: 0 }, { name: 'before may close fd 3 and redirect fd 9', before: 'exec 3>&- 9> "$TRACE.lock"', task: 'return 0', after: 'sleep 0.3', code: 0 }, - { name: 'timeout cleanup survives script fd 3 redirection', shell: true, task: 'exec 3> "$TRACE.fd"; STATE=changed; sleep 30', after: 'sleep 0.3', code: 124, timeout: true, state: 'changed' }, + { name: 'timeout cleanup survives script fd 3 redirection', shell: true, task: 'exec 3> "$TRACE.fd"; STATE=changed; sleep 30', after: 'sleep 0.3', code: 124, timeout: true }, + { name: 'Shell exit preserves user trap and runs after once', shell: true, task: 'trap \'printf "exit\\n" >> "$TRACE"\' EXIT; exit 9', code: 9, userExit: true }, { name: 'explicit exit preserves user trap and skips after', task: 'trap \'printf "exit\\n" >> "$TRACE"\' EXIT; exit 9', code: 9, noAfter: true, userExit: true }, { name: 'user EXIT trap survives timeout with parent cleanup', task: 'trap \'printf "exit\\n" >> "$TRACE"\' EXIT; sleep 10', code: 124, timeout: true, userExit: true }, { name: 'forced termination falls back to cleanup', task: 'trap "" INT; sleep 30', code: 124, timeout: true },