Add Apple mobile integration and setup flow - #252
Conversation
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the release authorization gaps below are fixed. The PR adds the Apple build/release MCP integration and setup UI, but execution is not server-gated on a later human approval, clean releases can be disabled by repository config, and the selected-user restriction is bypassed in shared sessions.
🔁 Not merge-ready and no live session owns this branch — add the |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![open-session-os-tella-dev[bot]](https://avatars.githubusercontent.com/u/65127121?s=60&v=4){kind=small}
| ); | ||
| return { ...result, plan: safePlanView(result.plan) }; | ||
| } | ||
| case "apple_release_execute": |
There was a problem hiding this comment.
🔴 P1 — Release execution does not enforce a separate human approval turn
Planning returns both planId and the full commit SHA, and this same MCP process immediately exposes apple_release_execute, whose only confirmation is that returned SHA. An agent can therefore call apple_release_plan_testflight, copy the values from its result, and call execute in the same turn, uploading to TestFlight without any human response. The skill instruction is only prompt guidance, contrary to this codebase's security rule that sensitive actions must be enforced at the tool/credential layer. Persist an approval grant created by a later authenticated human event and require executePlan to consume it, or move execution behind an equivalent server-side confirmation boundary; a value disclosed by the planning tool cannot prove approval.
There was a problem hiding this comment.
Fixed in 72dd175: planning now writes a signed pending request, execution requires and atomically consumes a one-time signed grant, and only a later authenticated allowlisted Settings action can create that grant. Echoing planId/commit is no longer sufficient.
| export async function enforceReleasePolicy(projectDir: string) { | ||
| const loaded = await loadConfig(projectDir); | ||
| const git = await gitSnapshot(projectDir); | ||
| if ((loaded.config.release?.requireClean ?? true) && !git.clean) { |
There was a problem hiding this comment.
🔴 P1 — Repository config can disable the clean-commit release guarantee
A committed .opensession/apple-mobile.json can set release.requireClean to false. If HEAD is commit abc123 and Sources/App.swift then has uncommitted changes, planning succeeds and presents abc123 for approval, but Xcode archives the modified working tree. The signed plan is therefore not actually bound to the source being released, contradicting both the stated clean-commit guarantee and the shipped skill's unconditional rule. Reject requireClean: false during config loading or check !git.clean unconditionally for every release plan.
There was a problem hiding this comment.
Fixed in 72dd175: config loading rejects release.requireClean=false and release policy now checks git cleanliness unconditionally. A regression test covers the repository-config bypass.
| APPLE_ASC_ISSUER_ID: String(issuerId), | ||
| APPLE_ASC_PRIVATE_KEY_PATH: String(privateKeyPath), | ||
| }, | ||
| allowedUsers, |
There was a problem hiding this comment.
🔴 P1 — Selected release users do not exclusively gate release access
This uses the ordinary allowedUsers metadata, whose runner implementation admits a server when either the current prompter or the session creator matches. Consequently, if allowed user Alice creates a session and unlisted Bob later steers that shared session, Bob receives apple-release and can sign or upload builds. That violates the setup UI's “People allowed to release” contract and the new setup guide's claim that other users' sessions cannot access it. Add a strict current-prompter-only gate for this credentialed server, while preserving the existing creator fallback for connections that intentionally share creator-scoped access.
There was a problem hiding this comment.
Fixed in 72dd175: apple-release now has a current-prompter-only gate; the session-creator fallback remains unchanged for ordinary restricted MCP servers. Regression tests cover both branches.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the P1 approval ambiguity below is fixed. The previous blockers around separate approval, clean worktrees, and current-prompter authorization are addressed, but the approval UI does not identify the exact IPA being authorized. Pending requests also accumulate in a way that eventually hides valid plans.
🔁 Not merge-ready and no live session owns this branch — add the |
| </div> | ||
| <Button | ||
| size="sm" | ||
| className="phone:min-h-11" |
There was a problem hiding this comment.
🔴 P1 — Approval rows do not identify the IPA being authorized
For upload plans, this row displays only IPA upload, the project basename, and the commit. Two plans for different IPAs in the same project and commit therefore render identically even though sourceArtifactSha256 differs. For example, a person intending to approve reviewed.ipa can click the indistinguishable row for other.ipa, after which execution consumes a valid grant and uploads that artifact. This defeats the stated requirement that the human review the exact plan. Display a stable correlation value such as planId plus the upload artifact SHA-256, and preferably the full project identity, so the person can match the row to the plan presented in chat.
There was a problem hiding this comment.
Fixed in 598b58a: the signed request and approval UI now include full project identity, plan ID, full commit, and for IPA uploads the artifact filename and SHA-256, so same-project/same-commit uploads are distinguishable.
| const dir = approvalsDir(root); | ||
| if (!existsSync(dir)) return []; | ||
| const requests: ReleaseApprovalRequest[] = []; | ||
| for (const name of readdirSync(dir).sort().slice(-100)) { |
There was a problem hiding this comment.
🟠 P2 — The request limit can hide newly created approval plans
Expired request files are never removed unless their plan is executed, while this slices the lexicographically sorted UUID filenames before checking expiry or creation time. Once 101 requests have accumulated, an arbitrary request is omitted; as more expired requests remain, a newly created plan increasingly may not appear in Settings and therefore cannot receive the required click approval. Filter request files first, remove or ignore expired entries while scanning, then sort valid requests by createdAt and apply the 100-item limit.
There was a problem hiding this comment.
Fixed in 598b58a: request files are filtered and signature/schema/date-validated before sorting by createdAt and applying the 100-item cap. Expired request/grant/plan data is removed, and grant files never enter the candidate set.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the P1 findings below are fixed. The new commits correctly add separate human approval, unconditional clean-worktree enforcement, and current-prompter-only release access. However, approval still cannot distinguish uploaded IPAs, and execution is not isolated from changes made after validation.
🔁 Not merge-ready and no live session owns this branch — add the |
| throw new Error("Invalid release plan"); | ||
| if (Date.parse(plan.expiresAt) < Date.now()) | ||
| throw new Error("Release plan has expired"); | ||
| const current = await enforceReleasePolicy(projectDir); |
There was a problem hiding this comment.
🔴 P1 — Release execution is not pinned to the validated files
loadPlan validates the mutable checkout and existing IPA, then returns their original paths for execution. Another session or editor can modify Sources/App.swift after this check while the subsequent Xcode archive is running, producing a signed build that differs from the approved commit. Likewise, an upload plan hashes its IPA here but xcrun later reopens the same mutable path. Execute build plans from an isolated checkout fixed at plan.commit, and copy upload artifacts into controlled storage when planning so execution uses the exact approved bytes.
There was a problem hiding this comment.
Fixed in 598b58a: execution now creates a fresh detached git worktree at the signed plan commit, verifies its HEAD, cleanliness, and committed config hash, then materializes every release command against that private checkout. Upload planning atomically copies the IPA into private controlled storage and signs the copied bytes; execution makes a private per-run copy, rechecks its SHA-256, and uploads that path instead of the mutable selected-worktree file.
| : ""} | ||
| </div> | ||
| <code className="mt-1 block break-all text-meta text-faint"> | ||
| {request.commit} |
There was a problem hiding this comment.
🔴 P1 — Approval rows still do not identify the IPA being authorized
Upload approvals display only IPA upload, the project basename, and commit, despite the request carrying sourceArtifactSha256. If reviewed.ipa and other.ipa are planned from the same clean project commit, their rows are identical even though execution uploads different bytes. An approver can therefore grant the wrong plan. Include the artifact name in the signed approval request and display it together with its SHA-256 before enabling approval.
There was a problem hiding this comment.
Fixed in 598b58a: upload approval requests now sign the artifact filename and SHA-256, and every approval row displays the full project path, plan ID, full commit, artifact filename, and SHA-256. The one-time grant is also bound to the action and artifact identity.
| const dir = approvalsDir(root); | ||
| if (!existsSync(dir)) return []; | ||
| const requests: ReleaseApprovalRequest[] = []; | ||
| for (const name of readdirSync(dir).sort().slice(-100)) { |
There was a problem hiding this comment.
🟠 P2 — The file limit can hide newly created approval requests
The code slices the lexicographically sorted directory before filtering request files, expiry, or creation time. Expired requests are retained, and unconsumed grant files share this directory, so after 100 such files a newly created UUID request can fall outside the slice and never appear in Settings. Filter and validate request files first, sort valid pending requests by createdAt, and only then apply the 100-item limit; expired files should also be removed.
There was a problem hiding this comment.
Fixed in 598b58a: pending discovery now scans request files only, verifies signatures/schema/identity/timestamps before collecting, deletes expired requests plus their grants and controlled plan data, skips granted requests, sorts valid pending requests by createdAt, and only then caps the result at 100. Regression coverage uses 101 requests plus grant files.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the P1 below is fixed. The new commits address the previously reported approval-turn, clean-worktree, immutable-input, approval-row, and request-limit issues. However, a non-admin teammate can still grant themselves access to the credentialed release connection.
🔁 Not merge-ready and no live session owns this branch — add the |
| return Response.json(appleMobileSetupStatus()); | ||
| } | ||
|
|
||
| if (path === "/api/connections/apple-mobile" && req.method === "PUT") { |
There was a problem hiding this comment.
🔴 P1 — Non-admin users can add themselves to the release allowlist
This configuration endpoint has no requireWorkspaceAdmin(ctx) check. On a role-aware workspace, any authenticated non-admin can PUT the currently exposed teamId, omit the secret fields so existingEnv(RELEASE_SERVER) preserves the configured API credentials, and replace allowedUsers with their own login. The existing generic PUT /api/connections/mcp/apple-release path at lines 513-523 provides the same bypass by calling setMcpAllowedUsers directly. On their next prompt they receive apple-release, and the approval endpoint also treats them as an authorized approver, defeating the selected-release-users boundary. Require workspace-admin authorization for this setup PUT and for generic MCP mutations that can alter or remove apple-release.
There was a problem hiding this comment.
Fixed in 37ff854: Apple mobile setup PUT now requires workspace-admin authorization. Generic MCP POST/PUT/DELETE mutations also require workspace-admin authorization when the target is apple-release, while ordinary MCP server mutations retain their prior behavior. Route regressions cover non-admin setup self-add, generic PUT/DELETE/create rejection, credential preservation, ordinary-server behavior, and admin update/delete/setup success. The 60 focused tests and typecheck, lint, format, and module-side-effect checks pass.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the P1 below is fixed. The previous approval, clean-worktree, allowlist, artifact identity, request ordering, and immutable-execution findings are addressed, but the new admin guard can still be bypassed when creating the protected MCP connection.
🔁 Not merge-ready and no live session owns this branch — add the |
| if (path === "/api/connections/mcp" && req.method === "POST") { | ||
| const body = await req.json().catch(() => null); | ||
| if (!body) return Response.json({ error: "Invalid JSON" }, { status: 400 }); | ||
| if (typeof body.name === "string" && requiresAllowedUsers(body.name)) { |
There was a problem hiding this comment.
🔴 P1 — Whitespace bypasses the Apple release admin check
This authorization check examines the raw name, while addMcpServer trims it before validation and storage. A non-admin can POST an MCP server named " apple-release " with allowedUsers: ["member"]: requiresAllowedUsers returns false here, then addMcpServer normalizes the name to apple-release and installs the credentialed release entry for that member. Normalize the name before checking authorization, matching the normalization performed by the mutation.
| if (typeof body.name === "string" && requiresAllowedUsers(body.name)) { | |
| if ( | |
| typeof body.name === "string" && | |
| requiresAllowedUsers(body.name.trim()) | |
| ) { |
There was a problem hiding this comment.
Fixed in 77fb159: the shared protected-server predicate now trims and case-folds names before identifying apple-release, so generic POST applies the admin gate before addMcpServer performs its own trim. PUT/DELETE path parameters use the same predicate after URL decoding; their downstream exact-name mutation behavior is unchanged. Regressions cover whitespace and uppercase POST names plus exact, uppercase, and whitespace-encoded PUT/DELETE paths. The 62 focused tests and typecheck, lint, format, and module-side-effect checks pass.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the two P1 authorization issues below are fixed. The previous clean-worktree, immutable-input, approval-detail, request-limit, and generic-mutation findings are addressed, but approval consumption can still permit duplicate release execution and the normalized protected-name policy is not applied by the runner.
🔁 Not merge-ready and no live session owns this branch — add the |
| const grantPath = approvalPath(plan.id, "grant", root); | ||
| const claimPath = `${grantPath}.claimed-${crypto.randomUUID()}`; | ||
| try { | ||
| renameSync(grantPath, claimPath); |
There was a problem hiding this comment.
🔴 P1 — Consumed approvals can be reissued during execution
consumeReleaseApproval atomically renames only the grant while leaving the request file available until its finally block. After execution A renames the grant, listReleaseApprovalRequests sees the still-present request and no grant, so it exposes the plan again; another browser can approve it, recreating the grant, and execution B can consume that grant while execution A is still archiving or uploading. This violates the promised one-time grant and can upload the same TestFlight build twice. Atomically claim the request before claiming the grant, make approval fail while that claim exists, and restore the request only if grant claiming fails.
There was a problem hiding this comment.
Fixed in bbc7e25: consumption now atomically renames the signed request before touching the grant, so listing skips it and approval reports that it is no longer pending. Only after that request claim does execution claim the grant. If the grant claim fails, the request is restored; once both claims are held, success or validation failure burns both. Signed request/grant writes are also atomic. Release execution intentionally discards a consumed plan after either success or command failure because an external signing/upload side effect may already have happened, while preflight failures remain retryable. Regressions interleave listing, reapproval, and duplicate consumption in the request-claimed window, verify request restoration without a grant, and verify invalid consumed claims are burned.
| ): boolean { | ||
| if (!Array.isArray(allowedUsers) || allowedUsers.length === 0) return true; | ||
| const gateUsers = | ||
| name === "apple-release" ? [user] : [user, ...(grantUsers || [])]; |
There was a problem hiding this comment.
🔴 P1 — Case variants bypass current-prompter-only release gating
The mutation layer now deliberately treats names such as APPLE-RELEASE as protected via trimmed, case-insensitive matching, but this actual runner gate still uses exact equality. addMcpServer accepts and preserves uppercase names, so an admin-created APPLE-RELEASE entry allowed for Alice is treated as an ordinary server: when Bob prompts in Alice's session, grantUsers = ["Alice"] satisfies the allowlist and Bob receives the release tools. Apply the same requiresAllowedUsers(name) normalization here, and in the parallel effective-config explanation, so every protected spelling enforces current-prompter-only access.
There was a problem hiding this comment.
Fixed in bbc7e25: runner-shared and effective-config now both use the centralized requiresAllowedUsers(name) predicate, including its trim and case-fold normalization, instead of exact string equality. Shared-session regressions verify that lowercase, uppercase, and whitespace variants all reject creator fallback for Bob while allowing the current prompter Alice; explanation regressions verify those same variants report only Bob as the evaluated identity.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the P1 below is fixed. The new commits address the previously reported approval, immutable-input, allowlist-normalization, artifact-identity, and request-limiting issues. However, the release runner still fails open when its required allowlist is absent or malformed.
🔁 Not merge-ready and no live session owns this branch — add the |
| user?: string, | ||
| grantUsers?: Array<string | undefined>, | ||
| ): boolean { | ||
| if (!Array.isArray(allowedUsers) || allowedUsers.length === 0) return true; |
There was a problem hiding this comment.
🔴 P1 — Apple release becomes fleet-wide when its allowlist is missing
This early return admits every server whose allowedUsers is absent, empty, or malformed before checking requiresAllowedUsers(name). Although the mutation helpers reject that state, readMcpConfig() accepts operator-edited or pre-existing configuration directly. For example, an apple-release entry retaining its App Store credentials but accidentally losing allowedUsers is then exposed to Bob and to automation runs with no user at all. That violates the stated unconditional release authorization boundary. Fail closed for protected names before applying the ordinary-server default.
| if (!Array.isArray(allowedUsers) || allowedUsers.length === 0) return true; | |
| if ( | |
| requiresAllowedUsers(name) && | |
| (!Array.isArray(allowedUsers) || allowedUsers.length === 0) | |
| ) | |
| return false; | |
| if (!Array.isArray(allowedUsers) || allowedUsers.length === 0) return true; |
There was a problem hiding this comment.
Fixed in 393d320: the runner now identifies protected names first and fails closed unless allowedUsers is a non-empty array made entirely of non-blank strings. Missing, empty, scalar, non-string, blank, and mixed allowlists deny apple-release even for a matching user or creator and for automation runs with no current user. Ordinary servers retain their absent/empty unrestricted default and creator fallback. Effective-config uses the same validation and explicitly reports an invalid required gate as denied instead of claiming the server is fleet-visible. Regressions cover all malformed states, no-current-user automation, valid protected access, and unchanged ordinary behavior. The 66 focused tests and typecheck, lint, format, and module-side-effect checks pass.
🤖 OS review · approve · confidence 5/5Safe to merge. The latest change correctly makes
|
1 participant
Summary
@tellahq/opensession-apple-mobileworkspace package, shipped skill, examples, and stableopensession apple-mobile-mcpentry pointallowedUsersauthorization forapple-releaseapple-release; make the normalized runner gate fail closed when its required allowlist is absent, empty, or malformed, with matching effective-config explanationsVerification
bun run typecheckbun run lintbun run format:checkbun scripts/check-module-side-effects.tsCI baseline investigation
Current
mainatf426222bbfails the same two jobs in run33315758630. On this PR, the only unit failure is the existingSessionViewer.socket.test.tssource-text assertion, and Ubuntu again times out waiting for the base server to start; macOS installation passes. The Apple-focused tests and all static checks pass locally.Started by Jaap Frolich in this Assistant session