[fish] make init hook recursion fish compatible (#1771)

## Summary

Hopefully this fix all the fish compatibility issues that were
introduced with the init hook recursion protection. The strategy in this
PR is:

* Wrap init hooks with POSIX and fish compatible wrapper. (no unset
used, only export and dot source)
* Wrap scripts in POSIX compatible wrapper (but not fish). 
* The script wrapper will only source init hooks once. The first time it
sources it, the init hook wrapper sets an env var and proceeds to run
the hook. If any command in the init hook is a devbox script, it will
not call init hooks recursively because the env var is set. Once the
init hook is done, it uses `export` to make the var empty. (`export` is
compatible with POSIX and fish, `unset` is not)
* The script wrapper is not fish compatible, but that's OK because
scripts are always run with `sh`. (I think it may be easy to make the
script fish compatible by using `test if we ever decide to support
that).

## How was it tested?

* added `devbox run` calls to init hook
* Shelled in (with zsh and fish)
* Ran scripts
* Tested sourcing `devbox shellenv --init-hook` (with zsh and fish)

Author: mikeland73

internal/devbox/devbox.go

@@ -258,7 +258,11 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string
 		// which we don't want. So, one solution is to write the entire command and its arguments into the
 		// file itself, but that may not be great if the variables contain sensitive information. Instead,
 		// we save the entire command (with args) into the DEVBOX_RUN_CMD var, and then the script evals it.
-		err := shellgen.WriteScriptFile(d, arbitraryCmdFilename, shellgen.ScriptBody(d, "eval $DEVBOX_RUN_CMD\n"))
+		scriptBody, err := shellgen.ScriptBody(d, "eval $DEVBOX_RUN_CMD\n")
+		if err != nil {
+			return err
+		}
+		err = shellgen.WriteScriptFile(d, arbitraryCmdFilename, scriptBody)
 		if err != nil {
 			return err
 		}
@@ -325,13 +329,7 @@ func (d *Devbox) EnvExports(ctx context.Context, opts devopt.EnvExportsOpts) (st
 	envStr := exportify(envs)
 
 	if opts.RunHooks {
-		hooksFilename := shellgen.HooksFilename
-		if isFishShell() {
-			hooksFilename = shellgen.HooksFishFilename
-		}
-
-		hooksStr := ". " + shellgen.ScriptPath(d.ProjectDir(), hooksFilename)
-
+		hooksStr := ". " + shellgen.ScriptPath(d.ProjectDir(), shellgen.HooksFilename)
 		envStr = fmt.Sprintf("%s\n%s;\n", envStr, hooksStr)
 	}
 

internal/devbox/shell.go

@@ -311,10 +311,8 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
 	}()
 
 	tmpl := shellrcTmpl
-	hooksFilePath := shellgen.ScriptPath(s.projectDir, shellgen.HooksFilename)
 	if s.name == shFish {
 		tmpl = fishrcTmpl
-		hooksFilePath = shellgen.ScriptPath(s.projectDir, shellgen.HooksFishFilename)
 	}
 
 	err = tmpl.Execute(shellrcf, struct {
@@ -333,7 +331,7 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
 		ProjectDir:         s.projectDir,
 		OriginalInit:       string(bytes.TrimSpace(userShellrc)),
 		OriginalInitPath:   s.userShellrcPath,
-		HooksFilePath:      hooksFilePath,
+		HooksFilePath:      shellgen.ScriptPath(s.projectDir, shellgen.HooksFilename),
 		ShellStartTime:     telemetry.FormatShellStart(s.shellStartTime),
 		HistoryFile:        strings.TrimSpace(s.historyFile),
 		ExportEnv:          exportify(s.env),

internal/shellgen/scripts.go

@@ -1,6 +1,7 @@
 package shellgen
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -18,20 +19,22 @@ import (
 	"go.jetpack.io/devbox/internal/plugin"
 )
 
-//go:embed tmpl/init-hook.tmpl
-var initHookTmpl string
+//go:embed tmpl/script-wrapper.tmpl
+var scriptWrapperTmplString string
+var scriptWrapperTmpl = template.Must(template.New("script-wrapper").Parse(scriptWrapperTmplString))
 
-//go:embed tmpl/init-hook-fish.tmpl
-var initHookFishTmpl string
+//go:embed tmpl/init-hook-wrapper.tmpl
+var initHookWrapperString string
+var initHookWrapperTmpl = template.Must(template.New("init-hook-wrapper").Parse(initHookWrapperString))
 
 const scriptsDir = ".devbox/gen/scripts"
 
-// HooksFilename is the name of the file that contains the project's init-hooks and plugin hooks
-const HooksFilename = ".hooks"
-
-// This is only used in shellrc_fish.tmpl. A bit of a hack, because scripts use
-// sh instead of fish.
-const HooksFishFilename = ".hooks.fish"
+// HooksFilename is the name of the file that contains a wrapper of the
+// project's init-hooks and plugin hooks
+const (
+	HooksFilename    = ".hooks"
+	rawHooksFilename = ".raw-hooks"
+)
 
 type devboxer interface {
 	Config() *devconfig.Config
@@ -69,20 +72,25 @@ func WriteScriptsToFiles(devbox devboxer) error {
 	}
 	hooks := strings.Join(append(pluginHooks, devbox.Config().InitHook().String()), "\n\n")
 	// always write it, even if there are no hooks, because scripts will source it.
-	err = writeInitHookFile(devbox, hooks, initHookTmpl, HooksFilename)
+	err = writeRawInitHookFile(devbox, hooks)
 	if err != nil {
 		return errors.WithStack(err)
 	}
-	written[HooksFilename] = struct{}{}
-	err = writeInitHookFile(devbox, hooks, initHookFishTmpl, HooksFishFilename)
+	written[rawHooksFilename] = struct{}{}
+
+	err = writeInitHookWrapperFile(devbox)
 	if err != nil {
 		return errors.WithStack(err)
 	}
-	written[HooksFishFilename] = struct{}{}
+	written[HooksFilename] = struct{}{}
 
 	// Write scripts to files.
 	for name, body := range devbox.Config().Scripts() {
-		err = WriteScriptFile(devbox, name, ScriptBody(devbox, body.String()))
+		scriptBody, err := ScriptBody(devbox, body.String())
+		if err != nil {
+			return errors.WithStack(err)
+		}
+		err = WriteScriptFile(devbox, name, scriptBody)
 		if err != nil {
 			return errors.WithStack(err)
 		}
@@ -103,29 +111,27 @@ func WriteScriptsToFiles(devbox devboxer) error {
 	return nil
 }
 
-func writeInitHookFile(devbox devboxer, body, tmpl, filename string) (err error) {
-	script, err := createScriptFile(devbox, filename)
+func writeRawInitHookFile(devbox devboxer, body string) (err error) {
+	script, err := createScriptFile(devbox, rawHooksFilename)
 	if err != nil {
 		return errors.WithStack(err)
 	}
 	defer script.Close() // best effort: close file
 
-	// skip adding init-hook recursion guard for the
-	// default hook or any empty hook
-	// there's nothing to guard, and it introduces complexity
-	if body == devconfig.DefaultInitHook || strings.TrimSpace(body) == "" {
-		_, err = script.WriteString(body)
-		return errors.WithStack(err)
-	}
+	_, err = script.WriteString(body)
+	return errors.WithStack(err)
+}
 
-	t, err := template.New(filename).Parse(tmpl)
+func writeInitHookWrapperFile(devbox devboxer) (err error) {
+	script, err := createScriptFile(devbox, HooksFilename)
 	if err != nil {
 		return errors.WithStack(err)
 	}
+	defer script.Close() // best effort: close file
 
-	return t.Execute(script, map[string]any{
-		"Body":         body,
+	return initHookWrapperTmpl.Execute(script, map[string]string{
 		"InitHookHash": "__DEVBOX_INIT_HOOK_" + devbox.ProjectDirHash(),
+		"RawHooksFile": ScriptPath(devbox.ProjectDir(), rawHooksFilename),
 	})
 }
 
@@ -167,6 +173,15 @@ func ScriptPath(projectDir, scriptName string) string {
 	return filepath.Join(projectDir, scriptsDir, scriptName+".sh")
 }
 
-func ScriptBody(d devboxer, body string) string {
-	return fmt.Sprintf(". %s\n\n%s", ScriptPath(d.ProjectDir(), HooksFilename), body)
+func ScriptBody(d devboxer, body string) (string, error) {
+	var buf bytes.Buffer
+	err := scriptWrapperTmpl.Execute(&buf, map[string]string{
+		"Body":         body,
+		"InitHookHash": "__DEVBOX_INIT_HOOK_" + d.ProjectDirHash(),
+		"InitHookPath": ScriptPath(d.ProjectDir(), HooksFilename),
+	})
+	if err != nil {
+		return "", errors.WithStack(err)
+	}
+	return buf.String(), nil
 }

internal/shellgen/tmpl/init-hook-fish.tmpl

@@ -0,0 +1,9 @@
+{{/*
+Wrapping the hooks ensures that any script called within doesn't trigger more
+init hooks.
+Code here should be fish and POSIX compatible. That's why we use export to 
+remove the value
+*/ -}}
+export {{ .InitHookHash }}=true
+. {{ .RawHooksFile }}
+export {{ .InitHookHash }}=""

internal/shellgen/tmpl/init-hook-wrapper.tmpl

@@ -0,0 +1,15 @@
+{{/* 
+    This wraps user scripts in devbox.json. The idea is to only run the init 
+    hooks once, even if the init hook calls devbox run again. This will also
+    protect against using devbox service in the init hook.
+
+    Scripts always use sh to run, so POSIX is OK. We don't (yet) support fish
+    scripts. (though users can run a fish script within their script)
+*/ -}}
+
+if [ -z "${{ .InitHookHash }}" ]; then
+    {{/* init hooks will export InitHookHash ensuring no recursive sourcing*/ -}}
+    . {{ .InitHookPath }}
+fi
+
+{{ .Body }}