Skip to content

[AUTOMATION] feat: clean up runtime host startup#256

Open
michiosw wants to merge 1 commit into
mainfrom
feat/cleanup-runtimehost-cwd-failure
Open

[AUTOMATION] feat: clean up runtime host startup#256
michiosw wants to merge 1 commit into
mainfrom
feat/cleanup-runtimehost-cwd-failure

Conversation

@michiosw

@michiosw michiosw commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary
This cleans up runtime host startup by failing fast when the current working directory cannot be resolved.

Before this, internal/runtimehost/host.go ignored the os.Getwd error and opened the initial session with an empty cwd, which hid the real startup failure and made session state harder to trust.

Now runtimehost.Start resolves cwd through one testable helper and stops immediately if that lookup fails:

cwd, err = currentWorkingDir()
if err != nil {
    _ = host.Close(context.Background())
    return nil, fmt.Errorf("resolve current working directory: %w", err)
}

Why
This gives kontext-cli a cleaner maintenance path for local runtime startup:

runtimehost.Start
-> cwd resolution
-> explicit failure + cleanup
-> no empty session cwd

This PR does not broaden behavior beyond the cleanup scope.

What changed
Added a test seam for current working directory lookup
Removed the hidden fallback that continued with an empty cwd
Strengthened startup failure handling in runtimehost.Start
Updated tests for cwd resolution failure

Verification
go test ./internal/runtimehost -count=1
go test ./internal/managedobserve -count=1
git diff --check

michiosw commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@greptile-apps

greptile-apps Bot commented Jun 7, 2026

Copy link
Copy Markdown

Greptile Summary

This PR changes runtime host startup to fail when the default cwd cannot be resolved. The main changes are:

  • Adds a package-level cwd resolver hook around os.Getwd.
  • Returns a wrapped startup error when cwd lookup fails.
  • Closes the partially started host on that failure path.
  • Adds a test for cwd lookup failure during Start.

Confidence Score: 4/5

This is close, but the daemon startup regression should be fixed before merging.

  • The new fail-fast path is correct for wrapper-owned initial sessions.

  • The same lookup now also runs for managed observe daemon startup, where no initial cwd is used.

  • A daemon launched from an invalid working directory can now fail before serving hooks.

  • internal/runtimehost/host.go should skip cwd resolution when SkipInitialSession is true.

Important Files Changed

Filename Overview
internal/runtimehost/host.go Adds fail-fast cwd lookup, but currently applies it to the no-initial-session daemon path.
internal/runtimehost/host_test.go Adds regression coverage for cwd lookup errors during normal runtime host startup.

Reviews (1): Last reviewed commit: "fix: fail fast on runtimehost cwd lookup..." | Re-trigger Greptile

Comment on lines 177 to 184
cwd := opts.CWD
if cwd == "" {
cwd, _ = os.Getwd()
cwd, err = currentWorkingDir()
if err != nil {
_ = host.Close(context.Background())
return nil, fmt.Errorf("resolve current working directory: %w", err)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Skip unused cwd lookup

This resolves the current directory before checking SkipInitialSession, but the managed observe daemon calls runtimehost.Start with SkipInitialSession: true and no CWD. In that mode this cwd value is not used to open an initial session; daemon-observed sessions get their cwd later from hook events. If launchd starts the daemon from a deleted or unreadable working directory, currentWorkingDir() now fails and the daemon does not start at all, even though it does not need a startup cwd.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8afb84fd06

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +179 to +182
cwd, err = currentWorkingDir()
if err != nil {
_ = host.Close(context.Background())
return nil, fmt.Errorf("resolve current working directory: %w", err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid resolving cwd when initial session is skipped

When opts.SkipInitialSession is true (as in internal/managedobserve/daemon.go, which starts the runtime host without passing CWD), the resolved cwd is never used because the OpenSession block is skipped, but this new lookup still runs and now fails if the process's working directory has been removed or is otherwise unreadable. That makes the managed observe daemon fail to start for an irrelevant cwd error; move the cwd resolution inside the !opts.SkipInitialSession branch or skip the error in that mode.

Useful? React with 👍 / 👎.

@michiosw michiosw changed the title fix: fail fast on runtimehost cwd lookup errors [AUTOMATION] feat: clean up runtime host startup Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant