feat(serve): add session-scoped permission route#4232
Merged
Conversation
Contributor
📋 Review SummaryThis PR implements session-scoped permission voting for the daemon serve infrastructure, adding 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Collaborator
Author
|
发现一个需要在 draft 转 ready 前补齐的验证缺口：PR8 新增了 我本地在当前 PR8 stacked 工作区跑了： npm run build && npm run bundle && cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/qwen-serve-routes.test.ts -t "capabilities envelope"结果确认失败：实际返回 14 个 features，包含 建议：PR8 在 PR7 更新到 13 个 features 的基础上，把 integration expected features 同步到 14 个，并把测试名从 “10 Stage 1 features” 改成不容易漂移的描述。 Generated by GPT-5 model |
wenshao
approved these changes
May 17, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No critical issues found. The implementation is well-structured with good test coverage (293 targeted tests passing).
Suggestion: The new types DaemonPermissionAlreadyResolvedData and DaemonPermissionAlreadyResolvedEvent (defined in packages/sdk-typescript/src/daemon/events.ts) are not re-exported from the barrel files (packages/sdk-typescript/src/daemon/index.ts and packages/sdk-typescript/src/index.ts). Every other DaemonPermission* type is individually exported. SDK consumers can still access these types through the DaemonControlEvent union, but cannot import them by name for function parameters, test assertions, or documentation annotations. Consider adding them to the export blocks for API surface consistency.
LGTM! ✅ — mimo-v2.5-pro via Qwen Code /review
wenshao
requested changes
May 17, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Suggestion: DaemonPermissionAlreadyResolvedEvent and DaemonPermissionAlreadyResolvedData are defined in events.ts but not re-exported from the barrel file packages/sdk-typescript/src/daemon/index.ts. External SDK consumers cannot import these types directly.
Add both types to the export type { ... } from './events.js' block in index.ts.
Additional suggestions (not mapped to specific diff lines):
- S7/S8: The two POST permission handlers in
server.tsand the twoDaemonClientmethods are near-identical copy-paste. Consider extracting shared helpers to reduce duplication. - S9:
DaemonPermissionAlreadyResolvedDataincludessessionIdbutDaemonPermissionResolvedDatadoes not — consider addingsessionIdto the resolved data type for API consistency. - S10: Bridge-level unit tests for
SessionNotFoundError/InvalidPermissionOptionErrorthrows fromrespondToSessionPermissionand the cross-session already-resolved guard are missing.
— glm-5.1 via Qwen Code /review
|
|
||
| const publishPermissionAlreadyResolved = ( | ||
| record: PermissionResolutionRecord, | ||
| originatorClientId?: string, |
Collaborator
There was a problem hiding this comment.
[Critical] originatorClientId has inverted semantics between permission_resolved and permission_already_resolved
In resolvePending (line ~1449), originatorClientId identifies the winning voter. Here in publishPermissionAlreadyResolved, the same field receives the late/losing voter's ID. Same field name in the event envelope, opposite semantic meaning.
SDK consumers handling both permission_resolved and permission_already_resolved through the DaemonControlEvent union will attribute the resolution to the wrong client — especially problematic in multi-client setups where winner and loser are different users.
Suggested fix: Either (a) carry the original winner's clientId in a separate field like resolvedByClientId and rename the late voter to lateVoterClientId, or (b) omit originatorClientId from permission_already_resolved since the winner already received attribution via the earlier permission_resolved event.
— glm-5.1 via Qwen Code /review
| case 'permission_already_resolved': { | ||
| const pendingPermissions = { ...base.pendingPermissions }; | ||
| delete pendingPermissions[event.data.requestId]; | ||
| return { |
Collaborator
There was a problem hiding this comment.
[Critical] permission_already_resolved reducer unconditionally overwrites sessionId
The permission_resolved case (line ~304) does not touch sessionId. This case unconditionally sets sessionId: event.data.sessionId, which silently corrupts the tracked session identity if events are ever replayed, delivered out of order, or multiplexed from multiple sessions.
The test at daemonEvents.test.ts:354 passes only because the preceding permission_request event already set sessionId to the same value, making the overwrite invisible.
Suggested change
| return { | |
| case 'permission_already_resolved': { | |
| const pendingPermissions = { ...base.pendingPermissions }; | |
| delete pendingPermissions[event.data.requestId]; | |
| return { | |
| ...base, | |
| pendingPermissions, | |
| }; | |
| } |
— glm-5.1 via Qwen Code /review
| delete pendingPermissions[event.data.requestId]; | ||
| return { ...base, pendingPermissions }; | ||
| } | ||
| case 'permission_already_resolved': { |
Collaborator
There was a problem hiding this comment.
[Suggestion] unmatchedPermissionResolutionCount not tracked for permission_already_resolved
The permission_resolved case (line ~305) increments unmatchedPermissionResolutionCount and sets lastUnmatchedPermissionResolutionId when the requestId is absent from pendingPermissions. This case silently no-ops for the same condition, breaking observability symmetry. SDK consumers cannot detect stale or unexpected permission_already_resolved events through standard telemetry.
Suggested change
| case 'permission_already_resolved': { | |
| case 'permission_already_resolved': { | |
| if (!(event.data.requestId in base.pendingPermissions)) { | |
| return { | |
| ...base, | |
| sessionId: base.sessionId, | |
| unmatchedPermissionResolutionCount: | |
| base.unmatchedPermissionResolutionCount + 1, | |
| lastUnmatchedPermissionResolutionId: event.data.requestId, | |
| }; | |
| } | |
| const pendingPermissions = { ...base.pendingPermissions }; | |
| delete pendingPermissions[event.data.requestId]; | |
| return { | |
| ...base, | |
| pendingPermissions, | |
| }; | |
| } |
— glm-5.1 via Qwen Code /review
| }); | ||
| return; | ||
| } | ||
| sendBridgeError(res, err, { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Legacy route error handling inconsistency
This new session-scoped route catches unexpected errors with sendBridgeError(res, err, ...) producing a structured JSON 500. The legacy POST /permission/:requestId route (line ~702) does throw err instead, relying on Express's default error handler. Same error type produces different HTTP response shapes across the two routes.
Suggested change
| sendBridgeError(res, err, { | |
| // In the legacy POST /permission/:requestId handler catch block, | |
| // replace `throw err;` with: | |
| sendBridgeError(res, err, { | |
| route: 'POST /permission/:requestId', | |
| }); | |
| return; |
— glm-5.1 via Qwen Code /review
| sessionId, | ||
| }); | ||
| return; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] 404 response ambiguity: already-resolved vs never-existed
Both "request already resolved by another voter" and "request never existed" return 404 with the same body. Clients without SSE subscriptions cannot distinguish these fundamentally different conditions.
Consider returning 409 Conflict for the already-resolved case (with the winning outcome in the body), and reserve 404 for truly unknown requestIds. Update the SDK's respondToSessionPermission to handle the 409 status accordingly.
— glm-5.1 via Qwen Code /review
wenshao
requested changes
May 17, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional suggestions (not mapped to specific lines):
- Code duplication: The
safeBody+isValidOutcomevalidation andInvalidPermissionOptionErrorcatch block are duplicated verbatim between the new scoped route and the legacy route inserver.ts. Extract a shared helper (e.g.,parsePermissionOutcome(req, res)) to keep them in sync. - Missing input validation tests: The new
POST /session/:id/permission/:requestIdroute has zero tests for malformed outcomes (missingoptionId, emptyoptionId, empty body). The legacy route has 3 such tests — add matching coverage for the scoped route. - Missing SDK error-path test:
DaemonClient.respondToSessionPermissionlacks a test for non-200/non-404 responses (e.g., 400, 500). The legacyrespondToPermissiontests this path. - Missing logging: Critical state transitions (
permission_already_resolvedpublished, duplicate vote hit, cross-session rejection) have zero log statements. Adddebug-level logging for oncall visibility.
| if (!pending) { | ||
| const record = resolvedPermissions.get(requestId); | ||
| if (record) { | ||
| if (context?.clientId !== undefined) { |
Collaborator
There was a problem hiding this comment.
[Critical] Uncaught InvalidClientIdError in legacy respondToPermission resolved-permission path
The new !pending branch (lines 2608–2620) calls resolveTrustedClientId(entry, context.clientId) without a try-catch. When the caller's clientId isn't registered with record.sessionId's session, this throws before publishPermissionAlreadyResolved is called — the event is silently lost and the caller gets a misleading 400 invalid_client_id instead of the expected 404.
This is a behavioral regression: pre-PR, duplicate votes on the legacy route silently returned false/404. Now they throw HTTP 400 when a clientId header is present but belongs to a different session.
Suggested change
| if (context?.clientId !== undefined) { | |
| if (entry) { | |
| try { | |
| originatorClientId = resolveTrustedClientId( | |
| entry, | |
| context.clientId, | |
| ); | |
| } catch { | |
| // clientId not registered with this session; | |
| // still publish the event without originatorClientId | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| respondToSessionPermission(sessionId, requestId, response, context) { | ||
| const entry = byId.get(sessionId); | ||
| if (!entry) throw new SessionNotFoundError(sessionId); |
Collaborator
There was a problem hiding this comment.
[Suggestion] respondToSessionPermission eagerly resolves clientId before checking session match
resolveTrustedClientId is called at line 2641, but the pending.sessionId !== sessionId check is at line 2652. If the clientId is invalid for the URL's session but the request actually belongs to a different session, this throws InvalidClientIdError (400) instead of correctly returning 404 with a "request belongs to other session" message.
Move resolveTrustedClientId after the session-mismatch guard.
Suggested change
| if (!entry) throw new SessionNotFoundError(sessionId); | |
| const pending = pendingPermissions.get(requestId); | |
| if (!pending) { | |
| const record = resolvedPermissions.get(requestId); | |
| if (record?.sessionId === sessionId) { | |
| const originatorClientId = resolveTrustedClientId(entry, context?.clientId); | |
| publishPermissionAlreadyResolved(record, originatorClientId); | |
| } | |
| return false; | |
| } | |
| if (pending.sessionId !== sessionId) return false; | |
| const originatorClientId = resolveTrustedClientId(entry, context?.clientId); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| sendBridgeError(res, err, { | ||
| route: 'POST /session/:id/permission/:requestId', | ||
| sessionId, | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Ambiguous 404 error: request never existed vs belongs to other session vs already resolved
The 404 body "No pending permission request for session" conflates three distinct cases: (a) requestId never existed, (b) requestId exists but belongs to a different session, (c) requestId was already resolved. HTTP-only callers have no way to distinguish these — they don't see the permission_already_resolved SSE event. Consider returning 409 when the requestId exists but belongs to another session, or include a reason field differentiating "not_found" from "already_resolved".
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| delete pendingPermissions[event.data.requestId]; | ||
| return { | ||
| ...base, | ||
| sessionId: event.data.sessionId, |
Collaborator
There was a problem hiding this comment.
[Suggestion] Inconsistent sessionId update between permission_already_resolved and permission_resolved reducers
The permission_already_resolved handler updates sessionId: event.data.sessionId (line 325), but the permission_resolved handler preserves base.sessionId via { ...base, pendingPermissions } (line 317). Both are resolution-family events and should behave consistently. Since events are always published on the owning session's SSE bus, the sessionId overwrite is redundant — consider removing it from permission_already_resolved to match permission_resolved, or add it to both.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
chiga0
force-pushed
the
feat/session-scoped-permission-route
branch
from
b80d5dd to
4eae97a
Compare
Collaborator
Author
|
已根据最新 review 和当前 main 状态更新 PR8：
刻意保持兼容： 本地验证已跑： cd packages/cli && npx vitest run src/serve/httpAcpBridge.test.ts src/serve/server.test.ts
cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts test/unit/daemonEvents.test.ts
npm run build
npm run typecheck
npm run bundle
cd integration-tests && QWEN_SANDBOX=false npx vitest run cli/qwen-serve-routes.test.ts -t "capabilities envelope"Generated by GPT-5 model |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
approved these changes
May 17, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-approving after the rebased single-commit 4eae97a33b. My two prior CHANGES_REQUESTED rounds are all addressed in the current head — the GitHub threads are stale (no auto-mark-resolved), but the code changes are in place.
Verified addressed
DaemonPermissionAlreadyResolvedEvent/DaemonPermissionAlreadyResolvedDatanow exported from bothpackages/sdk-typescript/src/daemon/index.tsand the top-levelpackages/sdk-typescript/src/index.tsbarrel, so external SDK consumers can import them directly.- Code duplication between the scoped and legacy permission handlers in
server.tscollapsed into two shared helpers —parsePermissionVoteBody(req, res)(handlessafeBody+isValidOutcome+ the 400 path) andsendPermissionVoteError(res, err, route)(handlesInvalidPermissionOptionError+InvalidClientIdError+ 500 fallthrough). Both routes now sit at ~25 lines each. - Input-validation coverage on the new route — four new 400 tests in
server.test.ts:400 on a malformed scoped selected outcome,400 when scoped outcome is missing entirely,400 when scoped selected outcome has an empty-string optionId,400 with invalid_option_id when bridge rejects a scoped option. Plus 4 more 200/404 happy/sad paths. respondToSessionPermissionnow checksrecord?.sessionId === sessionIdbefore callingresolveTrustedClientId(entry, context?.clientId), so we don't eagerly burn a clientId resolution against a request that turns out to belong to another session.
Partial / documented as Stage 1 limitation
- The 404 response body for "already-resolved vs never-existed vs wrong-session" is still generic in the HTTP layer; the bridge does distinguish internally (publishes
permission_already_resolvedto subscribers when a same-session vote arrives for an already-settled request, and writes a debug line on cross-session requests). The legacy route has an explicit comment acknowledging the conflation as Stage 1. I'm OK leaving this as-is — observers get the discrimination they need via the SSE event, and HTTP 404 stability has its own value.
Local verification on 4eae97a33b
httpAcpBridge.test.ts+eventBus.test.ts: 124 passed (includes the newpublishes permission_already_resolved when a scoped vote loses the raceregression).- SDK targeted vitest (
DaemonClient+DaemonSessionClient+daemonEvents): 88 passed (PR body lists 79 — author kept adding tests through the review iterations). sdk-typescripttypecheck: clean.tsc --noEmit -p packages/cli/tsconfig.jsonfiltered to PR-touched files: 0 errors.server.test.tsdoesn't load locally (pre-existing missingsupertestenv issue on this install; CI's matrix runs it green).
CI status at time of writing
- Classify PR / Lint / CodeQL: SUCCESS
- Test (mac · ubuntu · windows, Node 22.x): IN_PROGRESS
Approving on substance; please wait for the matrix to come back before merging.
LGTM.
wenshao
reviewed
May 17, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Inline comments below. Overall architecture is solid; issues are mostly polish + one subtle behavior change to call out in the description.
Should fix before merge (~10-min changes): reducer duplication, drop resolvedPermissionOrder parallel array, harmonize the two event-data shapes, fix the misleading catch message, clarify the subtle legacy-route behavior change.
Nice-to-have follow-ups: SDK helper extraction, originatorClientId on duplicate-vote, explanatory comments on clientId-validation asymmetry, document the 512 cap derivation.
Comment on lines
+320
to
+332
| case 'permission_already_resolved': { | ||
| if (!(event.data.requestId in base.pendingPermissions)) { | ||
| return { | ||
| ...base, | ||
| unmatchedPermissionResolutionCount: | ||
| base.unmatchedPermissionResolutionCount + 1, | ||
| lastUnmatchedPermissionResolutionId: event.data.requestId, | ||
| }; | ||
| } | ||
| const pendingPermissions = { ...base.pendingPermissions }; | ||
| delete pendingPermissions[event.data.requestId]; | ||
| return { ...base, pendingPermissions }; | ||
| } |
Collaborator
There was a problem hiding this comment.
Identical to the permission_resolved case at lines 307-318 — collapse with fall-through to avoid drift:
case 'permission_resolved':
case 'permission_already_resolved': {
if (!(event.data.requestId in base.pendingPermissions)) {
return {
...base,
unmatchedPermissionResolutionCount:
base.unmatchedPermissionResolutionCount + 1,
lastUnmatchedPermissionResolutionId: event.data.requestId,
};
}
const pendingPermissions = { ...base.pendingPermissions };
delete pendingPermissions[event.data.requestId];
return { ...base, pendingPermissions };
}
Comment on lines
+57
to
+62
| export interface DaemonPermissionAlreadyResolvedData { | ||
| requestId: string; | ||
| sessionId: string; | ||
| outcome: PermissionOutcome; | ||
| [key: string]: unknown; | ||
| } |
Collaborator
There was a problem hiding this comment.
Shape diverges from DaemonPermissionResolvedData (lines 51-55), which omits sessionId. The reducer for permission_already_resolved (lines 320-332) doesn't actually read sessionId from data either — only requestId. Either drop it here (it's implicit from the subscription), or also add it to DaemonPermissionResolvedData for consistency. Asymmetry today will turn into reducer-needs-to-handle-both-shapes complexity tomorrow.
Comment on lines
+752
to
+756
| interface PermissionResolutionRecord { | ||
| requestId: string; | ||
| sessionId: string; | ||
| outcome: RequestPermissionResponse['outcome']; | ||
| } |
Collaborator
There was a problem hiding this comment.
Consider capturing originatorClientId?: string here so publishPermissionAlreadyResolved can echo who originally won the race. Right now the duplicate-vote event has no originator info — the test in httpAcpBridge.test.ts explicitly asserts expect(alreadyEvt.originatorClientId).toBeUndefined() — so multi-client UIs can't surface "originally resolved by peer X". Not a blocker; could be a follow-up.
Comment on lines
+758
to
+760
| // Bounded duplicate-vote cache. Stores only requestId/sessionId/outcome, so | ||
| // 512 records stays small while covering normal UI reconnect/race windows. | ||
| const MAX_RESOLVED_PERMISSION_RECORDS = 512; |
Collaborator
There was a problem hiding this comment.
Worth quantifying the cache window. "covers normal UI reconnect/race windows" is vague — at ~10 votes/sec across all sessions, 512 records ≈ 50 seconds; at 1 vote/sec, ≈ 8 minutes. A one-line derivation ("≈ N minutes at typical vote rates of X/sec") would help future tuners judge whether the cap fits the workload.
Comment on lines
+1436
to
+1445
| const rememberResolvedPermission = (record: PermissionResolutionRecord) => { | ||
| if (!resolvedPermissions.has(record.requestId)) { | ||
| resolvedPermissionOrder.push(record.requestId); | ||
| } | ||
| resolvedPermissions.set(record.requestId, record); | ||
| while (resolvedPermissionOrder.length > MAX_RESOLVED_PERMISSION_RECORDS) { | ||
| const oldest = resolvedPermissionOrder.shift(); | ||
| if (oldest !== undefined) resolvedPermissions.delete(oldest); | ||
| } | ||
| }; |
Collaborator
There was a problem hiding this comment.
The parallel resolvedPermissionOrder array is redundant: JS Map preserves insertion order natively, and Array.shift() is O(n) on each eviction (≈ 512 element moves). Drop the array and iterate the Map directly:
const rememberResolvedPermission = (record: PermissionResolutionRecord) => {
resolvedPermissions.set(record.requestId, record);
while (resolvedPermissions.size > MAX_RESOLVED_PERMISSION_RECORDS) {
const oldestKey = resolvedPermissions.keys().next().value;
if (oldestKey !== undefined) resolvedPermissions.delete(oldestKey);
}
};(If you want LRU recency on re-set, delete before set. Current code is FIFO-by-first-resolution, which is what the diff produces too.)
Comment on lines
+1466
to
+1471
| } catch { | ||
| writeServeDebugLine( | ||
| `skipped duplicate-vote notification for permission ` + | ||
| `${JSON.stringify(record.requestId)} during shutdown.`, | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
The catch swallows ANY events.publish error but the debug line attributes it to "shutdown". If publish fails for some other reason in the future (and the catch being there at all suggests it's expected to throw under conditions other than the happy path), the bug will be silently mis-labeled. Either match the established style of resolvePending at lines 1495-1497 (/* bus closed during shutdown */), or write a message that's honest about what's actually being caught — e.g. failed to publish duplicate-vote notification (event bus closed).
Comment on lines
+2651
to
+2657
| if (!pending) { | ||
| const record = resolvedPermissions.get(requestId); | ||
| if (record) { | ||
| publishPermissionAlreadyResolved(record); | ||
| } | ||
| return false; | ||
| } |
Collaborator
There was a problem hiding this comment.
Subtle behavior change for the existing /permission/:requestId route: the no-pending path now publishes permission_already_resolved on the session's bus, not just on the new scoped route. The PR description frames the new event as a session-scoped addition, but this line means clients voting on the legacy route will also start seeing it on the SSE stream of the relevant session.
This is additive (older clients ignore unknown event types), but worth calling out in the PR description so existing-route consumers aren't surprised. Or if scoping is intended, move the publish out of respondToPermission and into respondToSessionPermission only.
Comment on lines
+2680
to
+2691
| if (!pending) { | ||
| const record = resolvedPermissions.get(requestId); | ||
| if (record?.sessionId === sessionId) { | ||
| resolveTrustedClientId(entry, context?.clientId); | ||
| publishPermissionAlreadyResolved(record); | ||
| } else if (record) { | ||
| writeServeDebugLine( | ||
| `rejected permission vote ${JSON.stringify(requestId)} ` + | ||
| `for session ${JSON.stringify(sessionId)}; request belongs to ` + | ||
| `session ${JSON.stringify(record.sessionId)}.`, | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
Behavioral difference vs. legacy respondToPermission (lines 2641-2650): the legacy path validates context.clientId against all sessions even when pending is missing — an unknown clientId throws InvalidClientIdError (→ 400). This scoped path silently returns false for the wrong-session-record branch without validating clientId at all.
The test session-scoped duplicate votes do not validate clients against another session enshrines this lenient behavior. Possibly intentional (don't reveal cross-session existence via auth errors), but the asymmetry deserves a code comment explaining the design choice — future maintainers will otherwise see this as a missed clientId check and "fix" it.
Comment on lines
+611
to
+647
| async respondToSessionPermission( | ||
| sessionId: string, | ||
| requestId: string, | ||
| response: PermissionResponse, | ||
| clientId?: string, | ||
| ): Promise<boolean> { | ||
| return await this.fetchWithTimeout( | ||
| `${this.baseUrl}/session/${encodeURIComponent(sessionId)}/permission/${encodeURIComponent(requestId)}`, | ||
| { | ||
| method: 'POST', | ||
| headers: this.headers({ 'Content-Type': 'application/json' }, clientId), | ||
| body: JSON.stringify(response), | ||
| }, | ||
| async (res) => { | ||
| if (res.status === 200) { | ||
| try { | ||
| await res.body?.cancel(); | ||
| } catch { | ||
| /* body already consumed or no body */ | ||
| } | ||
| return true; | ||
| } | ||
| if (res.status === 404) { | ||
| try { | ||
| await res.body?.cancel(); | ||
| } catch { | ||
| /* body already consumed or no body */ | ||
| } | ||
| return false; | ||
| } | ||
| throw await this.failOnError( | ||
| res, | ||
| 'POST /session/:id/permission/:requestId', | ||
| ); | ||
| }, | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
~95% identical to respondToPermission (lines 565-603). Extract a shared private helper:
private async postPermissionVote(
url: string,
response: PermissionResponse,
clientId: string | undefined,
errorContext: string,
): Promise<boolean> { /* the shared fetch+200/404/throw logic */ }Then respondToPermission and respondToSessionPermission each become a 4-line URL-builder + helper call. Reduces drift risk between the two routes.
7 tasks
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
2 participants
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.
Summary
POST /session/:id/permission/:requestId, keeps legacyPOST /permission/:requestId, exposes a matching SDK method, and emits/recognizespermission_already_resolvedwhen a duplicate scoped vote arrives after the first responder has already settled the request.Validation
permission_already_resolvedfor observers.npm run buildpassed with the existing vscode companion curly warning ineditorGroupUtils.ts.permission_already_resolvedreducer tests.Scope / Risk
permission_already_resolvedkeeps a bounded in-memory record of recent resolutions so duplicate votes can notify observers without unbounded memory growth.session_permission_voteafter this lands.DaemonSessionClient.respondToPermission()still uses it for old-daemon compatibility; new clients can opt intorespondToSessionPermission().Testing Matrix
Testing matrix notes:
Linked Issues / Bugs