fix: resolve sessions by name in attach, stop, and theia by planger · Pull Request #58 · eclipse-enclave/enclave

planger · 2026-08-25T19:46:25Z

Why

Containers are named enclave-<tool>-<project-hash>-<session>, but attach, stop <name>, and theia passed their positional argument straight to Docker as an exact container name. The flow documented on the website — enclave --background --name my-task then enclave attach my-task — could therefore never work.

Global uniqueness of session names is deliberately not introduced: container names are already unique via the project hash, and reusing --name review across several repos is a normal parallel-agent workflow.

What

attach, stop <name>, and theia/theia-next share one resolver (internal/app/session_target.go). A full container name still matches verbatim; anything else is matched as a session name, preferring sessions from the same worktree, then the same project, then the whole host. When a name matches several containers, the candidates are listed instead of one being guessed. --tool narrows candidates when given on the CLI.
enclave attach takes an optional argument: with a single running session in the project, bare attach picks it (matching theia).
stop <name> also considers stopped containers, so a leftover can be cleared by name.
Session-name normalization moved to model.SanitizeSessionName. The enclave.session label now stores the sanitized form, so it equals the trailing container-name segment; ps --name / status --name sanitize their filter input, which fixes --name "My Task" not matching --name my-task.
enclave ps gained a SESSION column (- for a project's default container) so the addressable name is discoverable.
The duplicate-session error now suggests enclave stop my-task rather than the full container name.
Docs updated: docs/cli-reference.md, docs/persistence.md, website/docs/docs/cli.md, and the attach/stop/theia help text.

Note for consumers

ps --json sessionName now reports the sanitized value (my-task, not My Task) for names that needed sanitizing. Field and shape are unchanged. Sessions started by an older build keep their raw label and still resolve, since matching sanitizes both sides.

How to test

Unit coverage: internal/app/session_target_test.go, internal/model/session_name_test.go, internal/runtime/session_name_test.go, internal/app/command_ps_test.go. make build, make test, make lint, and make check-license-headers pass; the resolver has not been exercised against live containers.

Manual:

enclave --background --name my-task
enclave ps                 # NAME shows the container, SESSION shows my-task
enclave attach my-task     # attaches; Ctrl-\ to detach
enclave attach             # single running session in the project is picked
enclave stop my-task

Ambiguity and scoping:

# same name in two different projects
(cd ~/repo-a && enclave --background --name review)
(cd ~/repo-b && enclave --background --name review)
cd ~/repo-a && enclave attach review   # resolves to repo-a's session
cd /tmp && enclave attach review       # lists both candidate container names

Sanitization: enclave --background --name "My Task" is addressable as enclave attach my-task, and enclave ps --name "My Task" and --name my-task list the same row.

Containers are named enclave-<tool>-<hash>-<session>, but attach/stop/theia passed their positional argument straight to Docker as an exact container name, so the documented `--name my-task` + `attach my-task` flow could never work. All three now share one resolver that also accepts a session name, preferring the current project and reporting ambiguity instead of guessing; session names stay non-unique across projects, which parallel agents rely on. The session label now stores the sanitized name so it matches the container name suffix, which also fixes `ps --name "My Task"` finding nothing, and `ps` gained a SESSION column so the addressable name is discoverable.

github-actions · 2026-08-25T19:47:37Z

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-02 15:59 UTC

Filtering by --name is done in the app layer with both sides sanitized instead of as an exact label filter, so `stop --name "???"` (which sanitizes to nothing) stops nothing rather than every background container, and the session label keeps the user-typed name: no ps/status JSON value change and pre-upgrade containers stay selectable. Also restores resolution by container ID, applies --tool to the candidates rather than to the listing so an exact container name always resolves, registers --tool on attach/theia as the documented disambiguator, and keeps the no-argument form inside the current project and on detached sessions so a bare `attach` cannot grab a foreground session's TTY.

planger · 2026-08-25T20:06:40Z

Follow-up commit a37ee43 addresses a subagent review of the first commit. The substantive findings:

Dangerous regression (introduced by the first commit): stop --name "???" sanitized the query to "", and the docker label filter is only applied when non-empty, so the command stopped and force-removed every background container of the tool. --name filtering for ps, status, and stop now happens in the app layer with both sides sanitized; a name that sanitizes to nothing matches nothing.

Dropped the session-label change. The label keeps the user-typed name, since sanitized-both-sides matching makes the stored form irrelevant. This removes the ps --json sessionName / status --json session_name value change flagged in the description above (no contract change anymore), and pre-upgrade containers whose label holds e.g. feature/ABC-123 stay selectable by --name.

Container IDs resolve again. attach <id> / stop <id> used to reach Docker directly; the resolver only compared names. IDs (12-char, full, or a prefix of at least 8 hex chars) are matched after session names, so names stay authoritative.

--tool is real now. The docs claimed it narrowed candidates, but neither attach nor theia registered the flag — so one --name used by two tools in the same project was permanently ambiguous with no escape hatch other than the full container name. Both commands now accept --tool, and the tool filter is applied to the candidates instead of to the listing, so an exact container name or ID still resolves regardless of tool.

No-argument form stays home. Bare attach/theia fell back host-wide when the current project had no session, so cd ~ && enclave attach could land in another project's agent. Auto-selection is now hard-scoped to the current project, and attach restricts it to detached sessions — attaching a second TTY to a foreground session means two terminals fighting over its stdin. Explicit names still resolve host-wide when unambiguous.

Also dropped an unused parameter with a misleading comment, and fixed the doc claims about --tool, the SESSION column (it also shows auto-assigned 1, 2, …), and the stale attach <container> row in the website overview table.

Tests added for all of the above (including two that failed first and caught real bugs in the fix: the unsanitizable-name guard and the project-scope fallback). make build, make test, make lint, make check-license-headers pass. Still no live-container verification — Docker is unavailable in my environment.

Two review points I did not act on, for the record: theia still loads preferences for the working-directory project even when an explicit name selects another project's container (pre-existing, and theia was always host-wide), and runAttach/runStop/runTheia have no injection seam, so their wiring is covered only at the helper level.

EclipseSourceAI

Note

Autonomous AI review.

This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.

Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.

To get an updated review after pushing changes, a maintainer may re-request a review from this account.

Running in Eclipse Enclave, submitted via review-guard-mcp

Makes attach, stop <name>, and theia/theia-next resolve their positional argument as a session name (and container ID) instead of passing it verbatim to Docker, so the documented --background --name my-task / attach my-task flow works. All three share a new resolver in internal/app/session_target.go: exact container name first, then sanitized session-name matching scoped current-worktree → project → host, then container ID, with ambiguity reported rather than guessed. sanitizeSessionName moved from runtime to model.SanitizeSessionName, --name filtering for ps/status/stop moved into the app layer with both sides sanitized, and ps gained a SESSION column.

The approach is sound and the follow-up commit already fixed the worst of it (the stop --name "???" wipe, container IDs, --tool actually being registered). Build and tests pass locally.

Points worth a maintainer's attention:

SessionFilter.SessionName is now dead in internal/backend, and it is exactly the field whose empty-means-no-filter semantics caused the stop --name "???" regression. Removing it closes that off.
stop <name> and stop --name <name> now diverge on tool scope, background-only, and stopped inclusion. That is easy to trip over.
The host-wide fallback for explicit names is more consequential for stop (which force-removes) than for attach, especially with auto-assigned 1, 2, … names that collide across projects by construction.
resolveExecTarget in internal/runtime/exec.go already does worktree-preferred selection. Two resolvers with different error wording now coexist.

The resolver has not been exercised against live containers, per the PR description. The cross-project and stopped-container paths in particular deserve a manual pass.

EclipseSourceAI · 2026-08-28T17:51:38Z

+// in sanitized form, which the backend cannot express as a label filter.
 func psSessionFilterFor(opts model.Options) backend.SessionFilter {
-	filter := backend.SessionFilter{Tool: psToolFilter(opts), SessionName: psSessionFilter(opts)}
+	filter := backend.SessionFilter{Tool: psToolFilter(opts)}


With ps, status, and stop all moved to app-layer matching, SessionFilter.SessionName has no producers left. It is still declared (types.go) and still turned into a label filter (docker.go). Dropping it removes the empty-value-means-no-filter footgun that caused the stop --name "???" regression instead of leaving it around for the next caller.

Removed in f24bf91: the field and the docker label filter it produced are gone. ps, status, and stop all match names in the app layer now, so nothing was left to feed it.

EclipseSourceAI · 2026-08-28T17:51:39Z

+	sessions, err := be.List(context.Background(), backend.SessionFilter{All: true, Background: &background, Tool: run.Tool})
 	if err != nil {
 		logx.Errorf("Failed to list background sessions: %v", err)
 		return 1
 	}
+	// Sanitized matching happens here rather than as a label filter, so that a
+	// name which sanitizes to nothing stops nothing instead of everything.
+	if name := strings.TrimSpace(run.SessionName); name != "" {
+		sessions = sessionsMatchingName(sessions, name)
+	}


stop <name> and stop --name <name> now behave quite differently: the positional path resolves across every tool, every project, and includes stopped and foreground containers, while this path stays pinned to run.Tool (defaults to claude) and Background: true. Users will read them as the same thing, so either align them or spell the difference out in docs/cli-reference.md.

Kept as two selectors, but the project-scope axis is gone: 37bcd0b scopes stop --name to the current project as well (it previously listed background sessions of every project, which is the same collision hazard as the positional form). The remaining differences — one session vs. all matching, tool from an explicit --tool vs. the resolved tool, background-only — are now spelled out in docs/cli-reference.md and in a new stop --help long text.

EclipseSourceAI · 2026-08-28T17:51:40Z

+	scoped, _ := projectSessions(matches, q.Project)
+	return selectSingleSession(scoped, requested)


When the current project has no match, this falls through to the host-wide candidates, and a single match elsewhere is used silently. That is fine for attach, but stop force-removes the container, and auto-assigned session names are 1, 2, … per project, so enclave stop 1 from a project without a session 1 can reach into an unrelated project. Restricting the fallback when IncludeStopped is set would be safer.

Fixed in f24bf91, and extended in 37bcd0b. Name resolution for stop no longer falls back host-wide (sessionTargetQuery.ProjectScopedNames); a name that matches nothing in the current project is rejected with a pointer to the container name or ID, which still resolve from anywhere. The same scoping now applies to stop --name, which listed background sessions of every project.

EclipseSourceAI · 2026-08-28T17:51:40Z

+// from the very same worktree first, then any session of the project. ok
+// reports whether the project matched anything, so that callers can decide
+// between falling back to all candidates and reporting nothing.
+func projectSessions(sessions []backend.Session, project model.Project) (scoped []backend.Session, ok bool) {


resolveExecTarget already implements same-worktree-preferred selection with ambiguity reporting for exec/shell (exec.go). Two implementations with different error wording now coexist for the same job. Not asking for the refactor in this PR, but a maintainer should decide whether exec should move onto this resolver.

Noted, not addressed here. Merging them is not mechanical: exec/shell synthesize the container name from --name instead of searching, list by name prefix, and do not accept container IDs — only the worktree-preferred selection overlaps. Worth its own issue.

EclipseSourceAI · 2026-08-28T17:51:41Z

+With no argument, `attach` picks the single detached session of the current
+project (a foreground session is entered with `exec` instead), and `theia` picks
+its single running container.


theia is project-scoped now too (autoSelectSession scopes by project whether or not BackgroundOnly is set), so "its single running container" reads as host-wide and is misleading. The theia --help long text already says "of the current project".

Fixed in f24bf91: the sentence now reads "the single running container of the current project".

xai

I think we should also address three safety issues related to session selection before merging. See inline comments for details.

`stop` no longer reaches into another project by session name: the auto-assigned names `1`, `2`, … collide across projects and removal is destructive, so another project's session needs its container name or ID. A positional argument that is present but blank is rejected instead of falling back to auto-selection, and an explicitly blank `--name` matches nothing rather than every background session. Ambiguous container-ID prefixes are reported instead of resolved by listing order. Also drops the now-unused `SessionFilter.SessionName` label filter, whose empty-means-everything semantics caused the original `--name` regression.

`stop --name` filtered background sessions of every project, so the name collision the positional form now guards against was still reachable through the flag. Both forms are project-scoped; an unresolvable project matches nothing instead of the whole host. Only a complete container ID may extend a recorded (truncated) one, since a value of some length in between cannot be verified against it.

planger · 2026-08-31T13:39:57Z

Follow-up commits f24bf91 and 37bcd0b address the review.

stop reaching into another project by name (both reviewers). Fixed, and wider than reported: the positional form is now project-scoped via sessionTargetQuery.ProjectScopedNames, and stop --name — which listed background sessions of every project — is scoped the same way. Scoping only the positional form would have left the exact enclave stop 1 hazard reachable through the flag. An unresolvable project matches nothing rather than everything. Exact container names and container IDs still resolve host-wide, so cross-project removal remains possible, just not by a colliding name. attach/theia keep the host-wide fallback: they don't destroy anything.

Note for consumers: enclave stop --name <name> no longer removes matching sessions of other projects. Bare enclave stop is unchanged (all background containers of the tool, all projects).

Blank --name / blank positional. sessionNameFilter(opts) returns (name, given), so an explicitly blank --name matches nothing instead of skipping the filter; ps, status, and stop share it. requestedSession rejects a positional argument that is present but blank — that path is what made enclave stop "$SESSION" with an unset variable fall through to auto-selection. To keep "not given" distinguishable in attach, the CLI now puts the detach keys first in CmdArgs (a parse test pins the layout; longer term a DetachKeys field on RunOptions would remove that positional coupling). runStop was split so that stopSessions(ctx, be, opts, projectDir) takes a backend: the command-level table covers the positive --name case, a same-named session in another project, blank --name, unsanitizable --name, blank positional, and the unfiltered batch form.

Ambiguous container-ID prefix. sessionsByContainerID collects every match and reports ambiguity like an ambiguous name. Recorded IDs are truncated to 12 characters, so only a value of exactly the full ID length (64) may extend one — a 20-character "prefix" can no longer piggyback on a real 12-character ID. Tests cover the collision and the partial extension.

SessionFilter.SessionName is dead. Removed, along with its docker label filter — that empty-means-everything field is what caused the earlier regression.

stop <name> vs stop --name <name>. Not aligned; they really are two selectors, and --name is the batch filter shared with ps/status. Project scope now agrees between them, and the remaining differences are documented in docs/cli-reference.md and in the new stop --help text: the positional form removes exactly one session of any tool unless --tool is passed, while --name removes every background session of the resolved tool (a profile or config tool counts) whose name matches. Both include containers that have already exited.

resolveExecTarget duplication. Agreed, but not a mechanical merge: exec/shell synthesize a container name from --name rather than searching, list by name prefix, and don't accept container IDs. Only the worktree-preferred selection overlaps, so it is worth its own issue.

theia doc wording. Fixed; it now reads "the single running container of the current project".

make build, make test, make lint, and make check-license-headers pass. Still no live-container verification.

EclipseSourceAI

Note

Autonomous AI review.

This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.

Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.

To get an updated review after pushing changes, a maintainer may re-request a review from this account.

Running in Eclipse Enclave, submitted via review-guard-mcp

Follow-up on f24bf91 and 37bcd0b. Checked these against the current code and tests, all hold up:

stop's positional and --name forms are now both project-scoped (ProjectScopedNames, projectHashSessions), closing the cross-project removal hazard for auto-assigned names like 1.
Blank --name and blank positional arguments are rejected/matched-to-nothing instead of falling back to "no filter" or "auto-select" (sessionNameFilter, requestedSession), which also fixes the underlying cause of the earlier stop --name "???" force-remove regression.
Ambiguous container-ID prefixes are now collected and reported instead of picked by list order (sessionsByContainerID + selectSingleSession), and a value can only extend a stored truncated ID when it's exactly full length.
SessionFilter.SessionName and its docker label filter are removed.
docs/cli-reference.md, docs/persistence.md, and the new stop --help text now spell out how stop <name> and stop --name differ, and the theia project-scoping wording is fixed.

Build and full test suite pass locally. No new issues in the commits since my last pass; the exec/resolveExecTarget duplication is acknowledged as out of scope with a reasonable explanation (different selection semantics, worth its own issue).

These previous comments can be resolved as they are now handled:

I can't resolve them myself as I would need write permission on this repository.

xai

Thanks for the fixes!

The changes look good, and all parts of the test plan (+ some additional testing) execute as expected.

planger requested a review from xai August 25, 2026 20:07

planger marked this pull request as ready for review August 25, 2026 20:07

EclipseSourceAI reviewed Aug 28, 2026

View reviewed changes

xai requested changes Aug 29, 2026

View reviewed changes

Comment thread internal/app/command_stop.go Outdated

Comment thread internal/app/session_target.go Outdated

Comment thread internal/app/session_target.go Outdated

planger added 2 commits August 31, 2026 13:27

planger requested review from EclipseSourceAI and xai August 31, 2026 13:43

EclipseSourceAI reviewed Aug 31, 2026

View reviewed changes

xai approved these changes Sep 2, 2026

View reviewed changes

planger merged commit 8ee49fa into main Sep 2, 2026
10 checks passed

planger deleted the fix/attach-by-session-name branch September 2, 2026 15:59

		scoped, _ := projectSessions(matches, q.Project)
		return selectSingleSession(scoped, requested)

Conversation

planger commented Aug 25, 2026

Why

What

Note for consumers

How to test

Uh oh!

github-actions Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

planger commented Aug 25, 2026

Uh oh!

EclipseSourceAI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

planger commented Aug 31, 2026

Uh oh!

EclipseSourceAI left a comment

Choose a reason for hiding this comment

Uh oh!

xai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 25, 2026 •

edited

Loading