Address review feedback

internal/api/runme/runner/v1/runner.proto

@@ -178,10 +178,16 @@ message ExecuteRequest {
   // file extension associated with script
   string file_extension = 26;
 
-  // optional well known id for cell/block
+  // optional well known id for cell/block. "know" meaning that
+  // CLI/notebook UX have id/name for cells/blocks that contain commands.
+  // While the runner doesn't require the name to work, it is useful for
+  // auxiliary concerns (e.g. tracing, logging, etc).
   string known_id = 27;
 
-  // optional well known name for cell/block
+  // optional well known name for cell/block. "know" meaning that
+  // CLI/notebook UX have id/name for cells/blocks that contain commands.
+  // While the runner doesn't require the name to work, it is useful for
+  // auxiliary concerns (e.g. tracing, logging, etc).
   string known_name = 28;
 }
 

internal/api/runme/runner/v2alpha1/runner.proto

@@ -138,9 +138,9 @@ message ExecuteRequest {
   // project used to load environment variables from .env files.
   optional Project project = 22;
 
-  // store_env_vars, if true, will store the stdout under well known name
+  // store_stdout_in_env, if true, will store the stdout under well known name
   // and the last ran block in the environment variable `__`.
-  bool store_env_vars = 23;
+  bool store_stdout_in_env = 23;
 }
 
 message ExecuteResponse {

internal/command/config_path_normalizer.go

@@ -52,7 +52,7 @@ func pathNormalizer(cfg *Config) (*Config, func() error, error) {
 		}
 	}
 
-	// default to "cat"
+	// Default to "cat"
 	cat, err := exec.LookPath("cat")
 	if err == nil {
 		result := proto.Clone(cfg).(*Config)

internal/gen/proto/ts/runme/runner/v2alpha1/runner_pb.d.ts

@@ -246,12 +246,12 @@ export interface ExecuteRequest {
      */
     project?: Project;
     /**
-     * store_env_vars, if true, will store the stdout under well known name
+     * store_stdout_in_env, if true, will store the stdout under well known name
      * and the last ran block in the environment variable `__`.
      *
-     * @generated from protobuf field: bool store_env_vars = 23;
+     * @generated from protobuf field: bool store_stdout_in_env = 23;
      */
-    storeEnvVars: boolean;
+    storeStdoutInEnv: boolean;
 }
 /**
  * @generated from protobuf message runme.runner.v2alpha1.ExecuteResponse

internal/gen/proto/ts/runme/runner/v2alpha1/runner_pb.js

@@ -286,7 +286,7 @@ class ExecuteRequest$Type extends MessageType {
             { no: 20, name: "session_id", kind: "scalar", T: 9 /*ScalarType.STRING*/ },
             { no: 21, name: "session_strategy", kind: "enum", T: () => ["runme.runner.v2alpha1.SessionStrategy", SessionStrategy, "SESSION_STRATEGY_"] },
             { no: 22, name: "project", kind: "message", T: () => Project },
-            { no: 23, name: "store_env_vars", kind: "scalar", T: 8 /*ScalarType.BOOL*/ }
+            { no: 23, name: "store_stdout_in_env", kind: "scalar", T: 8 /*ScalarType.BOOL*/ }
         ]);
     }
 }

internal/runner/command.go

@@ -690,7 +690,7 @@ func inferFileProgram(sys *system.System, programPath string, languageID string)
 		}
 	}
 
-	// default to "cat"
+	// Default to "cat"
 	res, err := sys.LookPath("cat")
 	if err == nil {
 		return res, args, nil

internal/runner/service.go

@@ -43,6 +43,9 @@ const (
 	msgBufferSize = 32 * 1000 // 32 KB
 )
 
+// Only allow uppercase letters, digits and underscores, min three chars
+var OpininatedEnvVarNamingRegexp = regexp.MustCompile(`^[A-Z_][A-Z0-9_]{1}[A-Z0-9_]*[A-Z][A-Z0-9_]*$`)
+
 type runnerService struct {
 	runnerv1.UnimplementedRunnerServiceServer
 
@@ -509,7 +512,7 @@ func (r *runnerService) Execute(srv runnerv1.RunnerService_ExecuteServer) error
 		if knownName != "" && runnerConformsOpinionatedEnvVarNaming(knownName) {
 			err = sess.SetEnv(knownName, string(stdoutMem))
 			if err != nil {
-				logger.Sugar().Errorf("%v", err)
+				logger.Warn("failed to set env", zap.Error(err))
 			}
 		}
 	}
@@ -619,9 +622,7 @@ func runnerWinsizeToPty(winsize *runnerv1.Winsize) *pty.Winsize {
 }
 
 func runnerConformsOpinionatedEnvVarNaming(knownName string) bool {
-	// only allow uppercase letters, digits and underscores, min three chars
-	re := regexp.MustCompile(`^[A-Z_][A-Z0-9_]{1}[A-Z0-9_]*[A-Z][A-Z0-9_]*$`)
-	return re.MatchString(knownName)
+	return OpininatedEnvVarNamingRegexp.MatchString(knownName)
 }
 
 func (r *runnerService) ResolveProgram(ctx context.Context, req *runnerv1.ResolveProgramRequest) (*runnerv1.ResolveProgramResponse, error) {

internal/runnerv2service/execution.go

@@ -32,6 +32,9 @@ const (
 	msgBufferSize = 2048 << 10 // 2 MiB
 )
 
+// Only allow uppercase letters, digits and underscores, min three chars
+var OpininatedEnvVarNamingRegexp = regexp.MustCompile(`^[A-Z_][A-Z0-9_]{1}[A-Z0-9_]*[A-Z][A-Z0-9_]*$`)
+
 // commandIface is an interface for virtual and native commands.
 type commandIface interface {
 	Pid() int
@@ -46,8 +49,8 @@ type execution struct {
 	KnownName string
 	Cmd       commandIface
 
-	session      *command.Session
-	storeEnvVars bool
+	session          *command.Session
+	storeStdoutInEnv bool
 
 	stdin       io.Reader
 	stdinWriter io.WriteCloser
@@ -61,7 +64,7 @@ func newExecution(
 	id string,
 	cfg *command.Config,
 	session *command.Session,
-	storeEnvVars bool,
+	storeStdoutInEnv bool,
 	logger *zap.Logger,
 ) (*execution, error) {
 	stdin, stdinWriter := io.Pipe()
@@ -105,8 +108,8 @@ func newExecution(
 		KnownName: cfg.GetKnownName(),
 		Cmd:       cmd,
 
-		session:      session,
-		storeEnvVars: storeEnvVars,
+		session:          session,
+		storeStdoutInEnv: storeStdoutInEnv,
 
 		stdin:       stdin,
 		stdinWriter: stdinWriter,
@@ -123,7 +126,7 @@ func (e *execution) Wait(ctx context.Context, sender sender) (int, error) {
 	lastStdout := rbuffer.NewRingBuffer(command.MaxEnvironSizeInBytes)
 	defer func() {
 		_ = lastStdout.Close()
-		e.storeLastOutput(lastStdout)
+		e.storeOutputInEnv(lastStdout)
 	}()
 
 	firstStdoutSent := false
@@ -282,8 +285,8 @@ func (e *execution) closeIO() {
 	e.logger.Info("closed stderr writer", zap.Error(err))
 }
 
-func (e *execution) storeLastOutput(r io.Reader) {
-	if !e.storeEnvVars {
+func (e *execution) storeOutputInEnv(r io.Reader) {
+	if !e.storeStdoutInEnv {
 		return
 	}
 
@@ -302,9 +305,7 @@ func (e *execution) storeLastOutput(r io.Reader) {
 }
 
 func conformsOpinionatedEnvVarNaming(knownName string) bool {
-	// only allow uppercase letters, digits and underscores, min three chars
-	re := regexp.MustCompile(`^[A-Z_][A-Z0-9_]{1}[A-Z0-9_]*[A-Z][A-Z0-9_]*$`)
-	return re.MatchString(knownName)
+	return OpininatedEnvVarNamingRegexp.MatchString(knownName)
 }
 
 type sender interface {

internal/runnerv2service/service_execute.go

@@ -51,7 +51,7 @@ func (r *runnerService) Execute(srv runnerv2alpha1.RunnerService_ExecuteServer)
 		id,
 		req.Config,
 		session,
-		req.StoreEnvVars,
+		req.StoreStdoutInEnv,
 		logger,
 	)
 	if err != nil {

internal/runnerv2service/service_execute_test.go

@@ -197,8 +197,8 @@ func TestRunnerServiceServerExecute_StoreLastStdout(t *testing.T) {
 				},
 			},
 		},
-		SessionId:    sessionResp.GetSession().GetId(),
-		StoreEnvVars: true,
+		SessionId:        sessionResp.GetSession().GetId(),
+		StoreStdoutInEnv: true,
 	}
 
 	err = stream1.Send(req1)
@@ -270,8 +270,8 @@ func TestRunnerServiceServerExecute_StoreKnownName(t *testing.T) {
 			},
 			KnownName: "TEST_VAR",
 		},
-		SessionId:    sessionResp.GetSession().GetId(),
-		StoreEnvVars: true,
+		SessionId:        sessionResp.GetSession().GetId(),
+		StoreStdoutInEnv: true,
 	}
 
 	err = stream1.Send(req1)