mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 22:37:03 +00:00
docs(server): correct ten overstated prose claims from Codex round 8 (IDEA-2756)
Round 8 reviewed only the prose this change adds. Ten claims were broader than the code. All ten are mine; none changes behaviour. Rounds 3, 4 and 7 each caught one of these, which is why round 8 was pointed at the class rather than at a new dimension. The substantive ones: - "gates every endpoint that MINTS a workspace" — autoCreateWorkspace mints from registration, bootstrap and oauth-login and is deliberately outside this gate. The helper doc and the test header now name the two callers and the exclusion instead of claiming universality. - "the agent was handed a workspace it could not then see" (version.go, README, CLAUDE.md) — only true for a connection with an EXPLICIT allow-list. An all_current_workspaces=true connection is not gated per slug and could see what it made. The consent mismatch is the constant; the invisibility was its most visible symptom, not its definition. - "ErrOAuthConnectionNotFound — a pre-Phase-C grant" asserted a cause the code cannot know: ANY missing row takes that branch. Now stated as the expected cause, with the limit of what the code can tell. - "above the 64 MiB body read" conflated the two import paths. 64 MiB is the JSON decode's bound; the bundle path has its own, much larger. The gate precedes both, which is the property that actually matters. - "the request context is decorated AFTER TokenAuth runs" was false, and inherited verbatim from the sibling helper this was modelled on (handlers_oauth_claim_test.go's doClaim), where it is also false. The wrapper sets the identity BEFORE ServeHTTP; it survives because nothing on the /api/v1 chain writes that key. - "lets CreateWorkspace normalize it" — CreateWorkspace slugifies only when the supplied slug is EMPTY, and import supplies a non-empty one, so an imported workspace keeps the ?name= value verbatim. - "The PAT needs a workspace to bind to" — CreateAPIToken takes WorkspaceID as optional. And one where the first fix was worse than the finding: - "Every refusal leg asserts no workspace exists afterwards" was false — the two ordering legs assert status only. My first correction ADDED those assertions, which is the trap the finding was pointing at: a malformed body and an empty name are rejected before creation under every guard placement, so "no such workspace exists" is true of broken and working code alike. Reverted; the header now states which legs carry the counterfactual, and why the ordering legs discriminate on status instead. Gates re-run on the tree being pushed, not an earlier one: gofmt clean, lint 0 issues, internal/server and internal/mcp green, mutation matrix still 9/9.
This commit is contained in:
@@ -87,7 +87,7 @@ Filter by trigger (`always`, `on-implement`, `on-task-complete`, etc.) when rele
|
||||
|
||||
If the user references a workspace this connection can't see (you'll get a 403 from workspace tools, or the workspace won't appear in `pad_workspace.list`), tell the user you can't see that workspace with your current permissions, then walk them through how to grant access: open Pad in their browser → switch to that workspace → avatar menu → "Connect project..." A 6-digit claim code will appear. Have them paste it back in chat, then call `pad_workspace.claim` with `{workspace: "<slug>", code: "<6 digits>"}`. The workspace joins this connection's allow-list and stays until the user revokes it via `/console/connected-apps`. No re-auth required.
|
||||
|
||||
For brand-new workspaces, `pad_workspace.create` with `{name: "<name>"}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the connection currently carries the "may create workspaces" grant — granted at consent time, or enabled later on the connections page. If it does not, the create is **refused with a 403 and no workspace is made** — retrying won't help and neither will the claim flow, since there is nothing to claim. Tell the user this connection isn't permitted to create workspaces, and offer them the two ways forward: create the workspace themselves in the browser and grant access with a claim code, or re-authorize this connection with "Let this app create new workspaces" enabled (`/console/connected-apps` can also flip it on an existing connection).
|
||||
For brand-new workspaces, `pad_workspace.create` with `{name: "<name>"}` (and optional `template`) creates the workspace AND auto-adds it to this connection's allow-list in one call — no claim code needed. Only works when the connection currently carries the "may create workspaces" grant — granted at consent time, or enabled later on the connections page. If the grant is present and set to false, the create is **refused with a 403 and no workspace is made** — retrying won't help and neither will the claim flow, since there is nothing to claim. Tell the user this connection isn't permitted to create workspaces, and offer them the two ways forward: create the workspace themselves in the browser and grant access with a claim code, or re-authorize this connection with "Let this app create new workspaces" enabled (`/console/connected-apps` can also flip it on an existing connection).
|
||||
|
||||
## New workspace: offer to set it up
|
||||
|
||||
|
||||
@@ -623,8 +623,13 @@ const CmdhelpVersion = "0.1"
|
||||
// - "0.26" — current. IDEA-2756: `pad_workspace.action=create` is now
|
||||
// REFUSED with a 403 when the calling OAuth connection's grant has
|
||||
// `may_create_workspaces=false`. Previously that flag gated only the
|
||||
// post-creation auto-add, so the create succeeded and the agent was
|
||||
// handed a workspace it could not then see.
|
||||
// post-creation auto-add, so the create succeeded — and on a
|
||||
// connection with an EXPLICIT workspace allow-list the agent was
|
||||
// handed a workspace it could not then see. (Not universal: a
|
||||
// connection with `all_current_workspaces=true` is not gated per
|
||||
// slug, so it could see what it made. The consent mismatch is the
|
||||
// constant across both; the invisibility was only its most visible
|
||||
// symptom.)
|
||||
//
|
||||
// BEHAVIOR bump, not a shape one — no tool name, action enum, or
|
||||
// parameter changed. Same grounds as v0.25 (activation destination),
|
||||
|
||||
@@ -16,24 +16,40 @@ import (
|
||||
//
|
||||
// The OAuth consent screen's `may_create_workspaces` checkbox used to gate
|
||||
// only maybeAutoAddCreatorConnection's allow-list insert: a connection whose
|
||||
// user left the box unticked could still create workspaces, it just could not
|
||||
// then see them. The ruling: the checkbox is a permission on whether the
|
||||
// user left the box unticked could still create workspaces — invisible to it
|
||||
// when the connection carried an explicit allow-list, visible when it carried
|
||||
// the all_current_workspaces wildcard, and unconsented either way. The ruling: the checkbox is a permission on whether the
|
||||
// connected token may CREATE, so an unset flag refuses outright with a 403,
|
||||
// mirroring handleAuditLog's consent refusal (BUG-2102).
|
||||
//
|
||||
// Two doors mint a workspace under the caller's account, both guarded here by
|
||||
// the shared requireWorkspaceCreationConsent helper:
|
||||
// Two doors let an OAuth-bound caller mint a workspace, both guarded here by
|
||||
// the shared requireWorkspaceCreationConsent helper. (Not every path to
|
||||
// store.CreateWorkspace: autoCreateWorkspace mints one at signup, outside this
|
||||
// gate and deliberately so — no OAuth connection is in context there.)
|
||||
//
|
||||
// - POST /api/v1/workspaces — handleCreateWorkspace
|
||||
// - POST /api/v1/workspaces/import — handleImportWorkspace, which reaches
|
||||
// CreateWorkspace via store.ImportWorkspace, and whose gzip Content-Type
|
||||
// branch dispatches to handleImportWorkspaceBundle from inside itself.
|
||||
//
|
||||
// Every refusal leg asserts the WRONG behaviour's observable consequence — that
|
||||
// no workspace by that slug exists afterwards — and not merely the status code
|
||||
// (CONVE-12). A guard that 403s AFTER store.CreateWorkspace would pass a
|
||||
// status-only assertion while leaving the workspace behind, which is the exact
|
||||
// failure this endpoint's users would report.
|
||||
// Every refusal leg whose request COULD have created something asserts the
|
||||
// wrong behaviour's observable consequence — that no workspace of that NAME
|
||||
// exists afterwards — and not merely the status code (CONVE-12). A guard that
|
||||
// 403s AFTER store.CreateWorkspace would pass a status-only assertion while
|
||||
// leaving the workspace behind, which is the exact failure this endpoint's
|
||||
// users would report.
|
||||
//
|
||||
// The two ORDERING legs are the deliberate exception, and adding the assertion
|
||||
// there would be worse than omitting it: a malformed body and an empty name are
|
||||
// rejected before creation under every guard placement, so "no such workspace
|
||||
// exists" is true of broken and working code alike. Those legs discriminate on
|
||||
// the STATUS instead — 400 if the gate is late, 403 if it is early — which is
|
||||
// the only signal that separates the two placements. (Round 8 flagged the
|
||||
// original blanket claim; the first fix added the vacuous assertions, which is
|
||||
// the failure that claim was warning about.)
|
||||
//
|
||||
// By NAME rather than by slug: the name is what the request supplies verbatim,
|
||||
// while the slug is derived differently on the two paths (see lookupByName).
|
||||
//
|
||||
// Everything drives the real router via srv.ServeHTTP rather than calling the
|
||||
// handler directly, so the tests have an opinion about the ROUTE and not only
|
||||
@@ -58,9 +74,12 @@ func newConsentEnv(t *testing.T, mayCreate bool) *consentEnv {
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
// The PAT needs a workspace to bind to, and the handler under test is the
|
||||
// one that makes workspaces — so seed this one through the store directly.
|
||||
// It is the token's home, never the workspace any test asserts about.
|
||||
// A PAT does not require a workspace (CreateAPIToken takes WorkspaceID as
|
||||
// optional), but binding one keeps this fixture close to a real CLI token
|
||||
// and gives the auth chain something ordinary to resolve. Seeded through
|
||||
// the store directly because the handler under test is the one that makes
|
||||
// workspaces. It is the token's home, never the workspace any test asserts
|
||||
// about.
|
||||
home, err := srv.store.CreateWorkspace(models.WorkspaceCreate{
|
||||
Name: "Consent Home", OwnerID: user.ID,
|
||||
})
|
||||
@@ -91,9 +110,16 @@ func newConsentEnv(t *testing.T, mayCreate bool) *consentEnv {
|
||||
}
|
||||
|
||||
// do issues a request with Bearer PAT auth. When requestID is non-empty the
|
||||
// request context is decorated with an OAuth grant identity AFTER TokenAuth
|
||||
// runs, which is what makes the caller look like an OAuth-bound MCP session.
|
||||
// Mirrors handlers_oauth_claim_test.go's doClaim.
|
||||
// request context is decorated with an OAuth grant identity, which is what
|
||||
// makes the caller look like an OAuth-bound MCP session.
|
||||
//
|
||||
// The wrapper sets the identity BEFORE srv.ServeHTTP, so it is in place before
|
||||
// TokenAuth runs — and survives, because nothing on the /api/v1 chain writes
|
||||
// that context key. Only MCPBearerAuth does, and it is mounted on /mcp alone.
|
||||
// (The sibling helper this is modelled on, handlers_oauth_claim_test.go's
|
||||
// doClaim, describes the same wrapper as decorating the context "AFTER
|
||||
// TokenAuth runs"; that ordering claim is wrong for both, and the mechanism
|
||||
// works for the reason given above instead.)
|
||||
func (e *consentEnv) do(method, path, contentType string, body []byte, requestID string) *httptest.ResponseRecorder {
|
||||
var req *http.Request
|
||||
if body != nil {
|
||||
@@ -128,9 +154,10 @@ func (e *consentEnv) createWorkspace(name, requestID string) *httptest.ResponseR
|
||||
// lookupByName finds a workspace by its NAME rather than its slug. Name is
|
||||
// what the request supplies verbatim; slug is derived, and the derivation
|
||||
// differs between the create path (normalizeWorkspaceInput) and the import
|
||||
// path (which takes the ?name= override as the slug and lets CreateWorkspace
|
||||
// normalize it). Asserting on the name keeps these tests about the guard
|
||||
// instead of about slugification.
|
||||
// path, which passes the ?name= override through as the SLUG — and
|
||||
// CreateWorkspace slugifies only when the supplied slug is empty, so an
|
||||
// imported workspace keeps that value verbatim. Asserting on the name keeps
|
||||
// these tests about the guard instead of about slug derivation.
|
||||
func (e *consentEnv) lookupByName(t *testing.T, name string) *models.Workspace {
|
||||
t.Helper()
|
||||
all, err := e.srv.store.ListWorkspaces()
|
||||
@@ -419,4 +446,5 @@ func TestCreateWorkspace_RefusalMessageNamesTheRemedy(t *testing.T) {
|
||||
if !strings.Contains(strings.ToLower(body), "re-authorize") {
|
||||
t.Errorf("refusal message does not tell the caller how to fix it: %s", body)
|
||||
}
|
||||
e.mustNotExist(t, "Message WS")
|
||||
}
|
||||
|
||||
@@ -291,10 +291,18 @@ func (s *Server) handleReorderWorkspaces(w http.ResponseWriter, r *http.Request)
|
||||
writeJSON(w, http.StatusOK, map[string]string{"status": "ok"})
|
||||
}
|
||||
|
||||
// requireWorkspaceCreationConsent gates every endpoint that MINTS a
|
||||
// workspace under the caller's account on the calling OAuth connection's
|
||||
// `may_create_workspaces` grant. Returns true when the request may
|
||||
// proceed; on false it has already written the response.
|
||||
// requireWorkspaceCreationConsent gates a request that MINTS a workspace
|
||||
// on the calling OAuth connection's `may_create_workspaces` grant.
|
||||
// Returns true when the request may proceed; on false it has already
|
||||
// written the response.
|
||||
//
|
||||
// Two callers, which are the two endpoints an OAuth-bound caller can
|
||||
// mint through: handleCreateWorkspace and handleImportWorkspace. NOT
|
||||
// every path to store.CreateWorkspace — autoCreateWorkspace
|
||||
// (handlers_cloud.go) mints a workspace during registration, bootstrap
|
||||
// and OAuth-login, and is deliberately outside this gate: it runs at
|
||||
// signup with no OAuth connection in context, provisioning the user's
|
||||
// own first workspace rather than acting for a connected app.
|
||||
//
|
||||
// Ruled by Dave on IDEA-2756 (2026-08-26): the consent screen's "may
|
||||
// create workspaces" checkbox is a permission on whether the connected
|
||||
@@ -316,10 +324,13 @@ func (s *Server) handleReorderWorkspaces(w http.ResponseWriter, r *http.Request)
|
||||
// - Not an OAuth grant (PAT, CLI session token, local stdio — no
|
||||
// request_id in context). Creation rides on ordinary account
|
||||
// authority; this flag has no opinion about it.
|
||||
// - ErrOAuthConnectionNotFound — a pre-Phase-C grant whose row has
|
||||
// not been backfilled yet. The backfill mints these with
|
||||
// may_create_workspaces ON (oauth_connections_backfill.go), so
|
||||
// allowing here is that same default applied early, not a gap.
|
||||
// - ErrOAuthConnectionNotFound — no connection row for this grant.
|
||||
// The expected cause is a pre-Phase-C grant not yet backfilled, and
|
||||
// the backfill mints those with may_create_workspaces ON
|
||||
// (oauth_connections_backfill.go), so allowing here is that same
|
||||
// default applied early rather than a gap. Stated as the expected
|
||||
// cause and not the only one, because the code cannot tell them
|
||||
// apart: any missing row takes this branch.
|
||||
// Note the deliberate asymmetry with maybeAutoAddCreatorConnection,
|
||||
// which treats not-found as "no auto-add": that path is declining a
|
||||
// convenience, this one would be inventing a refusal.
|
||||
@@ -795,8 +806,12 @@ func (s *Server) handleImportWorkspace(w http.ResponseWriter, r *http.Request) {
|
||||
// may_create_workspaces grant decides it. Ruled with the create-side
|
||||
// gate on IDEA-2756. It sits ABOVE the Content-Type dispatch so it
|
||||
// covers the tar.gz bundle path too — handleImportWorkspaceBundle is
|
||||
// reachable only from here, so this is its only door — and above the
|
||||
// 64 MiB body read, so a refused caller never uploads a bundle.
|
||||
// reachable only from here, so this is its only door — and above
|
||||
// either body read, so a refused caller never uploads anything. The
|
||||
// two reads have different bounds (the JSON path's 64 MiB
|
||||
// decodeJSONWithLimit, the bundle path's own configurable and much
|
||||
// larger limit); the gate precedes both, which is the property that
|
||||
// matters here.
|
||||
//
|
||||
// Reachability, stated precisely because the create-side gate's is
|
||||
// different: NO OAuth-bound caller can reach this handler today. The
|
||||
|
||||
Reference in New Issue
Block a user