Repository navigation
fix(cloud): accept canonical answer receipts #706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { readFile } from 'node:fs/promises'; | |||||
| import { CloudFlowError, cloudFetch, cloudRunId, isCloudRecord, type CloudConnectionOptions } from './cloud-http.js'; | ||||||
| import { cloudRunState } from './cloud-run-record.js'; | ||||||
| import { readCloudHumanWaitValue } from './cloud-human-wait.js'; | ||||||
| import { readCloudRepositoryGrant } from './cloud-repository-grant.js'; | ||||||
| import { readSurfaceAuthority, type SurfaceModuleAuthority } from './authored-flow-loader.js'; | ||||||
| import type { CloudAuthoredAuthority } from './cloud-run.js'; | ||||||
|
|
||||||
|
|
@@ -29,24 +30,32 @@ export class CloudAnswerError extends Error { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| interface CloudRecordedHumanAnswer { | ||||||
| waitId: string; | ||||||
| answer: boolean; | ||||||
| note?: string; | ||||||
| } | ||||||
|
|
||||||
| const route = (runId: string): string => `/api/v1/workflows/runs/${runId}`; | ||||||
| const get = (path: string, options: CloudConnectionOptions): Promise<unknown> => | ||||||
| cloudFetch(path, options, { method: 'GET', detail: true }); | ||||||
| const post = (path: string, body: unknown, options: CloudConnectionOptions): Promise<unknown> => | ||||||
| cloudFetch(path, options, { method: 'POST', body: JSON.stringify(body), detail: true }); | ||||||
|
|
||||||
| /** | ||||||
| * Accepted answer route shapes are explicit: humanWait (or wait), or openWaits, | ||||||
| * and an optional recorded answer { waitId, answer, note? }. Unknown/ambiguous | ||||||
| * shapes refuse before writes. A closed wait is retryable only with that proof. | ||||||
| */ | ||||||
| export async function readCloudHumanWait(runId: string, options: CloudConnectionOptions = {}) { | ||||||
| cloudRunId(runId); | ||||||
| const body = await get(`${route(runId)}/answer`, options); | ||||||
| function readCloudHumanAnswerRecord(body: unknown, runId: string): { | ||||||
| wait?: ReturnType<typeof readCloudHumanWaitValue>; | ||||||
| recorded?: CloudRecordedHumanAnswer; | ||||||
| } { | ||||||
| if (!isCloudRecord(body)) throw new CloudFlowError('invalid_response', 'Cloud returned no human answer record.'); | ||||||
| if (body.runId !== undefined && body.runId !== runId) { | ||||||
| if (body.runId !== runId) { | ||||||
| throw new CloudFlowError('invalid_response', 'Cloud returned a human answer record for a different run.'); | ||||||
| } | ||||||
| if (body.error !== undefined && body.error !== null) { | ||||||
| throw new CloudFlowError('invalid_response', 'Cloud returned an error beside a human answer record.'); | ||||||
| } | ||||||
| if (body.ok !== undefined && body.ok !== true) { | ||||||
| throw new CloudFlowError('invalid_response', 'Cloud explicitly refused the human answer record.'); | ||||||
| } | ||||||
| // An absent and a null field mean the same thing everywhere below. | ||||||
| const present = (key: string): boolean => body[key] !== undefined && body[key] !== null; | ||||||
| if (['humanWait', 'wait', 'openWaits'].filter(present).length > 1) { | ||||||
|
|
@@ -59,19 +68,49 @@ export async function readCloudHumanWait(runId: string, options: CloudConnection | |||||
| } | ||||||
| const wait = readCloudHumanWaitValue(candidates[0]); | ||||||
| if (candidates.length === 1 && !wait) throw new CloudFlowError('invalid_response', 'Cloud returned an invalid open human wait.'); | ||||||
| const recorded = body.answer; | ||||||
| if (['humanAnswer', 'answer'].filter(present).length > 1) { | ||||||
| throw new CloudFlowError('invalid_response', 'Cloud returned competing recorded human answer fields.'); | ||||||
| } | ||||||
| const recorded = body.humanAnswer ?? body.answer; | ||||||
| if (recorded !== undefined && recorded !== null | ||||||
| && (!isCloudRecord(recorded) || !readCloudHumanWaitValue(recorded) || typeof recorded.answer !== 'boolean' | ||||||
| || (recorded.note !== undefined && recorded.note !== null && typeof recorded.note !== 'string'))) { | ||||||
| throw new CloudFlowError('invalid_response', 'Cloud returned an invalid recorded human answer.'); | ||||||
| } | ||||||
| if (wait && isCloudRecord(recorded) && recorded.waitId !== wait.waitId) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Treat a recorded answer for an older wait as stale during GET discovery instead of rejecting the whole route; keep the same-wait check for POST acknowledgements. Otherwise, a run parked at a later human gate cannot be answered. Prompt for AI agents |
||||||
| throw new CloudFlowError('invalid_response', 'Cloud returned a human answer for a different wait.'); | ||||||
| } | ||||||
|
Comment on lines
+80
to
+82
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Later human gates cannot be answered When Cloud retains an earlier answer beside a new Learn moreThe answer route can retain an answer for an earlier wait while reporting the current open wait. The preceding implementation selected the open wait and ignored the earlier decision in answerCloudFlow. Rejecting the whole response now prevents the command from answering the current gate. Keep the strict same-wait requirement for POST acknowledgements, but allow a stale recorded answer on GET and select only an answer whose wait ID matches the current gate. Example: A run answered Recommended fix: Separate GET discovery from POST acknowledgement validation. On GET, discard a recorded answer for a different wait; on POST, require the receipt's wait ID and decision to match the submitted wait. Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||
| return { wait, recorded: isCloudRecord(recorded) | ||||||
| // Like the wait fields, a null note means absent. | ||||||
| ? { waitId: recorded.waitId as string, answer: recorded.answer as boolean, | ||||||
| note: typeof recorded.note === 'string' ? recorded.note : undefined } | ||||||
| : undefined }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Accepted answer route shapes are explicit: humanWait (or wait), or openWaits, | ||||||
| * and an optional recorded humanAnswer (legacy: answer) { waitId, answer, note? }. | ||||||
| * Unknown fields are additive, but competing or cross-run/wait shapes refuse. | ||||||
| */ | ||||||
| export async function readCloudHumanWait(runId: string, options: CloudConnectionOptions = {}) { | ||||||
| cloudRunId(runId); | ||||||
| return readCloudHumanAnswerRecord(await get(`${route(runId)}/answer`, options), runId); | ||||||
| } | ||||||
|
|
||||||
| function normalizedAnswerNote(note: string | undefined): string | undefined { | ||||||
| const normalized = note?.trim(); | ||||||
| return normalized === undefined || normalized.length === 0 ? undefined : normalized; | ||||||
| } | ||||||
|
|
||||||
| function sameRecordedAnswer( | ||||||
| recorded: CloudRecordedHumanAnswer, | ||||||
| waitId: string, | ||||||
| answer: boolean, | ||||||
| note: string | undefined, | ||||||
| ): boolean { | ||||||
| return recorded.waitId === waitId && recorded.answer === answer && recorded.note === note; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Normalize the recorded note in the comparison as well. Otherwise a stored note with surrounding whitespace or an empty string can never match the normalized Prompt for AI agents
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| const HEX64 = /^[a-f0-9]{64}$/u; | ||||||
|
|
||||||
| function isSurfaceAuthority(value: unknown): value is SurfaceModuleAuthority { | ||||||
|
|
@@ -125,6 +164,10 @@ async function resumeBody(record: Record<string, unknown>, runId: string, source | |||||
| throw new CloudFlowError('unsupported_source', 'Cannot resume a run with extensions: Cloud must expose their original submissions.'); | ||||||
| } | ||||||
| } | ||||||
| const repositoryGrant = readCloudRepositoryGrant(authority.repository); | ||||||
| if (authority.repository !== undefined && repositoryGrant === undefined) { | ||||||
| throw new CloudFlowError('invalid_response', 'Cloud run carries an invalid repository grant; refusing to widen or alter it.'); | ||||||
| } | ||||||
| let workflow = record.workflow; | ||||||
| if (source !== undefined) { | ||||||
| try { workflow = await readFile(source, 'utf8'); } | ||||||
|
|
@@ -144,6 +187,7 @@ async function resumeBody(record: Record<string, unknown>, runId: string, source | |||||
| surface: await resumeSurface(authority.source.surface, source), | ||||||
| }); | ||||||
| return { workflow, authoredAuthority, fileType: 'ts', relayflowVersion: 'v2', resume: runId, | ||||||
| ...(repositoryGrant === undefined ? {} : { repositoryGrant }), | ||||||
| ...(typeof record.workspaceId === 'string' ? { workspaceId: record.workspaceId } : {}) }; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -160,18 +204,19 @@ export async function answerCloudFlow(runId: string, answer: boolean, options: C | |||||
| if (!waitId || (state.humanWait && state.humanWait.waitId !== waitId)) { | ||||||
| throw new CloudFlowError('invalid_response', 'Cloud answer record does not identify the run’s parked wait.'); | ||||||
| } | ||||||
| // A recorded answer that omits its optional note does not contradict --note. | ||||||
| if (recorded && (recorded.answer !== answer | ||||||
| || (recorded.note !== undefined && recorded.note !== options.note))) { | ||||||
| const note = normalizedAnswerNote(options.note); | ||||||
| if (recorded && !sameRecordedAnswer(recorded, waitId, answer, note)) { | ||||||
| throw new CloudFlowError('invalid_input', 'The recorded answer differs; a closed human wait cannot be changed.'); | ||||||
|
Comment on lines
+207
to
209
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Older whitespace notes block resume After upgrading, retrying a recorded answer with a padded note makes Learn moreThe earlier client sent Example: The old CLI posts Recommended fix: Normalize the recorded note for comparison as well, or preserve verbatim equality as a compatibility fallback while keeping normalized notes for new answer POSTs. Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||
| } | ||||||
| const body = await resumeBody(record as Record<string, unknown>, runId, options.source); | ||||||
| let answerRecorded = recorded !== undefined; | ||||||
| try { | ||||||
| if (!answerRecorded) { | ||||||
| const accepted = await post(`${route(runId)}/answer`, { waitId, answer, ...(options.note === undefined ? {} : { note: options.note }) }, options); | ||||||
| // Only an explicit acknowledgement counts; {} or { ok: null } is not a recorded decision. | ||||||
| if (!isCloudRecord(accepted) || accepted.ok !== true || accepted.error !== undefined) { | ||||||
| const accepted = await post(`${route(runId)}/answer`, { waitId, answer, ...(note === undefined ? {} : { note }) }, options); | ||||||
| const acknowledged = readCloudHumanAnswerRecord(accepted, runId); | ||||||
| if (!acknowledged.wait || !acknowledged.recorded | ||||||
| || acknowledged.wait.waitId !== waitId | ||||||
| || !sameRecordedAnswer(acknowledged.recorded, waitId, answer, note)) { | ||||||
|
Comment on lines
+216
to
+219
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/sdk/src/cloud-answer.ts
Line: 207-210
Comment:
**Guide describes rejected answers**
`docs/CLOUD.md:936-944` still says only `{ ok: true }` confirms an answer and that an earlier wait’s answer does not block the current wait. This PR explicitly rejects both cases. Users diagnosing rejected answers will get the wrong guidance. Update that section and the response fields at lines 953-955 to match the new contract.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||
| throw new CloudFlowError('invalid_response', 'Cloud did not confirm the human answer.'); | ||||||
| } | ||||||
| answerRecorded = true; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import { isCloudRecord } from './cloud-http.js'; | ||
|
|
||
| export interface CloudRepositoryGrant { | ||
| host?: 'gitlab'; | ||
| owner: string; | ||
| repo: string; | ||
| baseBranch: string; | ||
| baseSha: string; | ||
| branch: string; | ||
| mode?: 'head'; | ||
| workflows?: true; | ||
| access?: 'read'; | ||
| } | ||
|
|
||
| const coordinate = /^[A-Za-z0-9][A-Za-z0-9_.-]{0,99}$/u; | ||
|
|
||
| function validGitlabNamespace(value: unknown): value is string { | ||
| if (typeof value !== 'string' || value.length > 255) return false; | ||
| const parts = value.split('/'); | ||
| return parts.length >= 1 && parts.length <= 20 && parts.every(part => ( | ||
| coordinate.test(part) && !part.endsWith('.') && !part.endsWith('.git') && !part.endsWith('.atom') | ||
| )); | ||
| } | ||
|
|
||
| function validRepositoryBranch(value: unknown): value is string { | ||
| return typeof value === 'string' | ||
| && /^[A-Za-z0-9][A-Za-z0-9._/-]{0,199}$/u.test(value) | ||
| && value !== 'HEAD' | ||
| && !value.includes('..') | ||
| && !value.includes('//') | ||
| && value.split('/').every(part => ( | ||
| part !== '' && !part.startsWith('.') && !part.endsWith('.') && !part.endsWith('.lock') | ||
| )); | ||
| } | ||
|
|
||
| /** | ||
| * Rebuild the request portion of persisted Cloud repository authority. | ||
| * Cloud re-admits this request against the persisted identity and current | ||
| * connection; the SDK must neither infer missing authority nor widen it. | ||
| */ | ||
| export function readCloudRepositoryGrant(value: unknown): CloudRepositoryGrant | undefined { | ||
| if (value === undefined) return undefined; | ||
| if (!isCloudRecord(value)) return undefined; | ||
| const gitlab = value.host === 'gitlab'; | ||
| if ((value.host !== undefined && value.host !== 'github' && value.host !== 'gitlab') | ||
| || typeof value.owner !== 'string' | ||
| || !(gitlab ? validGitlabNamespace(value.owner) : coordinate.test(value.owner)) | ||
| || typeof value.repo !== 'string' | ||
| || !/^[A-Za-z0-9_.-]{1,100}$/u.test(value.repo) | ||
| || value.repo === '.' || value.repo === '..' | ||
| || (gitlab && (value.repo.endsWith('.git') || value.repo.endsWith('.atom') || value.repo.endsWith('.'))) | ||
| || !validRepositoryBranch(value.baseBranch) | ||
| || !validRepositoryBranch(value.branch) | ||
| || (value.mode !== undefined && value.mode !== 'new' && value.mode !== 'head') | ||
| || (value.workflows !== undefined && typeof value.workflows !== 'boolean') | ||
| || (gitlab && value.workflows === true) | ||
| || (value.access !== undefined && value.access !== 'read') | ||
| || (value.access === 'read' && value.workflows === true) | ||
| || (gitlab && value.access === 'read') | ||
| || (value.mode !== 'head' && !value.branch.startsWith('relayflow/')) | ||
| || value.branch === value.baseBranch | ||
| || typeof value.baseSha !== 'string' | ||
| || !/^[a-f0-9]{40}$/u.test(value.baseSha)) { | ||
| return undefined; | ||
| } | ||
| return { | ||
| ...(gitlab ? { host: 'gitlab' as const } : {}), | ||
| owner: value.owner.toLowerCase(), | ||
| repo: value.repo.toLowerCase(), | ||
| baseBranch: value.baseBranch, | ||
| baseSha: value.baseSha, | ||
| branch: value.branch, | ||
| ...(value.mode === 'head' ? { mode: 'head' as const } : {}), | ||
| ...(value.workflows === true ? { workflows: true as const } : {}), | ||
| ...(value.access === 'read' ? { access: 'read' as const } : {}), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: A matching legacy
answerfield is still treated as confirmation, so this can reportanswerRecorded: truewithout canonicalhumanAnswer; read confirmation only fromhumanAnswer.Prompt for AI agents