Skip to content

fix(agent/agentscripts): create missing log_path parent directory - #28166

Merged
35C4n0r merged 2 commits into
mainfrom
35C4n0r/fix-coder-script-log-path
Aug 14, 2026
Merged

fix(agent/agentscripts): create missing log_path parent directory#28166
35C4n0r merged 2 commits into
mainfrom
35C4n0r/fix-coder-script-log-path

Conversation

@35C4n0r

@35C4n0r 35C4n0r commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Previously, a coder_script whose log_path pointed under a directory that did not yet exist failed before the script ran, with no per-script log output. OpenFile(logPath, O_CREATE|O_RDWR, 0o600) creates the log file but not its parent directories, so the open returned ENOENT. The failure only surfaced in the agent log (startup script(s) failed / shutdown script(s) failed) and never reached the script's own UI logs, which made it look like a silent failure.

This creates the resolved parent directory with MkdirAll(filepath.Dir(logPath), 0o700) before opening the log file, so the script runs and its log is written. 0o700 matches the existing script data-dir and secret-file directory conventions in this package. Resolution of ~, environment variables, and paths relative to LogDir is unchanged; only the parent directory is now created.

Fixes #21986

Implementation notes and validation

Change

  • agent/agentscripts/agentscripts.go: in (*Runner).run, after the full logPath resolution and before OpenFile, create the parent directory:

    logDir := filepath.Dir(logPath)
    if err = r.Filesystem.MkdirAll(logDir, 0o700); err != nil {
        return xerrors.Errorf("create script log file directory %q: %w", logDir, err)
    }

Regression test

  • agent/agentscripts/agentscripts_test.go: TestExecuteCreatesMissingLogDir runs a script with a nested, nonexistent LogPath and asserts the streamed output and that the log file is created.
  • The test uses afero.NewOsFs() on purpose: afero.NewMemMapFs() auto-creates parent directories on OpenFile, so it cannot reproduce the reported failure.
  • Verified red without the fix (open .../does/not/exist/install.log: no such file or directory) and green with it.

Local validation

  • gofmt clean, go vet, go build, golangci-lint run on the package, and go test -race ./agent/agentscripts/ all pass.

End-to-end

  • Validated on a dev instance with a template whose coder_script.log_path targets a nested directory that does not exist. The agent created the parents with mode 0700 and wrote the log file; the workspace agent reported healthy.

Prior attempts

  • #22796 and #25545 proposed the same directory-creation approach. Both were closed for non-technical reasons (a low-effort AI PR and a stale community PR), not rejected on the merits. This supersedes them, authored by the issue owner, using 0o700 and adding a regression test.

Raised on behalf of @35C4n0r by Coder Agents.

A coder_script whose log_path points under a directory that does not exist failed before execution because OpenFile creates the log file but not its parent directories. Create the parent directory with MkdirAll before opening the log file.
@github-actions github-actions Bot added the community Pull Requests and issues created by the community. label Aug 14, 2026
@35C4n0r
35C4n0r marked this pull request as ready for review August 14, 2026 17:16
logger.Info(ctx, "running agent script", slog.F("script", script.Script))

logDir := filepath.Dir(logPath)
if err = r.Filesystem.MkdirAll(logDir, 0o700); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Just wondering if we could define a constant for 0o700. Also, should we make this configurable via an environment variable or config option, with 0o700 as the default?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if we could define a constant for 0o700

0o700 states the exact permission bits, it feels more intuitive/easier to read.

should we make this configurable via an environment variable or config option, with 0o700 as the default

0o700 seems like a safe private default, the workspace owner can chmod it afterward if they need looser perms. We can revisit this if a use case like this comes up.

@BobbyHo BobbyHo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just left a NIT comment (non-blocking)

@35C4n0r
35C4n0r enabled auto-merge (squash) August 14, 2026 17:59
@35C4n0r
35C4n0r merged commit af90d8e into main Aug 14, 2026
28 checks passed
@35C4n0r
35C4n0r deleted the 35C4n0r/fix-coder-script-log-path branch August 14, 2026 18:09
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community Pull Requests and issues created by the community.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: log_path in coder script causes the script to fail silently

2 participants