Workshopctl instance context - #1019
Merged
dmitry-lyfar merged 5 commits intoSep 7, 2026
Merged
Conversation
tlm
force-pushed
the
workshopctl-instance-context
branch
from
September 4, 2026 03:49
6c629a2 to
d7624be
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The cookie lookup path exposes internal state errors through an untrusted endpoint.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Propagates request contexts through workshopctl and securely resolves hook contexts for hook-based and ordinary workshop requests.
Changes:
- Adds user-scoped workshop instance ownership validation.
- Adds taskless ephemeral hook contexts.
- Passes request contexts into commands and expands endpoint coverage.
File summaries
| File | Description |
|---|---|
internal/overlord/workshopstate/manager.go |
Adds instance ownership lookup. |
internal/overlord/workshopstate/manager_test.go |
Tests ownership validation. |
internal/overlord/hookstate/manager.go |
Creates ephemeral contexts. |
internal/overlord/hookstate/manager_test.go |
Tests ephemeral context creation. |
internal/overlord/hookstate/ctlcmd/health.go |
Accepts execution context. |
internal/overlord/hookstate/ctlcmd/health_test.go |
Updates command invocations. |
internal/overlord/hookstate/ctlcmd/getsecret.go |
Accepts execution context. |
internal/overlord/hookstate/ctlcmd/getsecret_test.go |
Updates command tests. |
internal/overlord/hookstate/ctlcmd/export_test.go |
Adds context-aware mock execution. |
internal/overlord/hookstate/ctlcmd/ctlcmd.go |
Propagates context through command handling. |
internal/overlord/hookstate/ctlcmd/ctlcmd_test.go |
Verifies context propagation. |
internal/daemon/api_workshopctl.go |
Resolves cookie or instance-backed contexts. |
internal/daemon/api_workshopctl_test.go |
Tests context resolution paths. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ) (*hookstate.Context, Response) { | ||
| hookContext, err := c.d.overlord.HookManager().Context(contextID) | ||
| if err != nil { | ||
| return nil, statusBadRequest("cannot get workshop context: %w", err) |
dmitry-lyfar
approved these changes
Sep 6, 2026
dmitry-lyfar
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, there are a couple of linter issues
Base automatically changed from
system-sdk-secret-provider
to
secrets-implementation
September 7, 2026 03:48
Pass a standard context.Context to workshopctl commands so request-scoped values can be made available during command execution. This prepares get-secret to access the requesting workshop's instance identity. Use the go-flags command handler because its native Execute interface does not support context propagation.
Accept a context.Context in ctlcmd.Run and pass it to the selected command during execution. This allows request-scoped values, including the workshop instance ID, to reach get-secret. Pass the HTTP request context from the workshopctl API handler and verify the propagation contract through the mock command's Execute callback.
Use an existing hook context when a workshopctl request supplies a cookie. For cookie-less requests, verify that the requesting user owns the workshop instance ID before creating a taskless ephemeral hook context. Log internal lookup failures while returning a generic error response to avoid exposing backend details to callers.
dmitry-lyfar
force-pushed
the
workshopctl-instance-context
branch
from
September 7, 2026 03:48
b40f7eb to
584515a
Compare
Avoid anonymous struct context keys in TestRunPassesContextToCommand to satisfy staticcheck SA1029 and prevent key collisions.
No production code writes workshop-cookies to state, so the fallback cannot resolve contexts during normal operation. Ordinary workshopctl requests now use the validated instance-ID path and NewEphemeralContext. Resolve hook context IDs only against active contexts and return a generic HTTP 400 for unknown IDs. This removes the state lookup that could expose internal errors to callers and misclassify them as client errors. Return a plain error because no caller needs a sentinel. Rename the request helper to workshopctlHookContextFromContextID to match the context-id field. Replace the artificial persisted-cookie fixture with coverage for active lookup and rejection of unknown cookies, including when a valid instance ID is supplied.
dmitry-lyfar
approved these changes
Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is stacked on #1017, which adds the authenticated user and workshop instance ID to the HTTP request context.
It propagates that request context through
workshopctlcommand execution and resolves an appropriate hook context for both hook-driven and ordinary workshop requests. This preparesget-secretto determine securely which workshop made a request without coupling command execution directly to the HTTP layer.Context propagation
The internal command interface now accepts a standard
context.Context, and the workshopctl endpoint passesr.Context()into command execution.Request identity belongs in
context.Contextbecause it is request-scoped metadata. Passing it this way avoids adding workshop identity parameters to every command and allows future commands to consume request metadata without further changing the command interface.go-flagsonly invokes commands implementingExecute([]string) error. Since that signature cannot carry a context, the parser now uses a command handler that calls the project's context-aware command interface explicitly.Hook context resolution
Workshopctl commands still require a
hookstate.Context, but requests can originate through two different paths.When a request supplies a hook cookie, the handler resolves the existing active hook context. This preserves the established behavior for commands executed during a hook, including access to the hook task and its context data.
Ordinary SDK wrapper calls do not execute inside a hook and therefore do not have a hook cookie. For these requests, the handler uses the workshop instance ID propagated by #1017.
The instance ID is not trusted by itself. Before creating a context, the workshop manager verifies that the authenticated user owns a workshop with that instance ID. This prevents a caller from presenting another workshop's identifier and using it to obtain that workshop's context.
After ownership is established, the hook manager creates a taskless ephemeral context. This provides the interface expected by workshopctl commands without fabricating a hook task or persisting context data beyond the request.
The cookie and instance-ID paths are kept in separate helpers so their different trust models and lifecycle requirements remain explicit.
Error handling
Unknown or unowned instance IDs are rejected without creating a context.
Backend and state lookup failures are logged server-side, while callers receive a generic internal error response. Returning the underlying error could expose project names, filesystem details, state contents, or other implementation information through an untrusted endpoint.
Tests
Tests cover:
Self-review quick check
Docs