diff --git a/apps/mobile/src/features/threads/git/GitOverviewSheet.tsx b/apps/mobile/src/features/threads/git/GitOverviewSheet.tsx index 828c8838e31a..31ad85873d1c 100644 --- a/apps/mobile/src/features/threads/git/GitOverviewSheet.tsx +++ b/apps/mobile/src/features/threads/git/GitOverviewSheet.tsx @@ -55,6 +55,18 @@ type GitOverviewSheetProps = StaticScreenProps<{ }; export function GitOverviewSheet(props: GitOverviewSheetProps) { + const navigation = useNavigation(); + const { environmentId, threadId } = props.route.params; + // A hand-typed deep link can carry a blank ID, which the branded IDs reject. + const isBlankLink = environmentId.trim().length === 0 || threadId.trim().length === 0; + useEffect(() => { + if (isBlankLink) navigation.goBack(); + }, [isBlankLink, navigation]); + if (isBlankLink) return null; + return ; +} + +function GitOverviewSheetContent(props: GitOverviewSheetProps) { const { layout } = useAdaptiveWorkspaceLayout(); const navigation = useNavigation(); const insets = useSafeAreaInsets(); diff --git a/apps/web/src/lib/composerContextRecords.test.ts b/apps/web/src/lib/composerContextRecords.test.ts index e5a83030a5fa..3634cea6d448 100644 --- a/apps/web/src/lib/composerContextRecords.test.ts +++ b/apps/web/src/lib/composerContextRecords.test.ts @@ -514,7 +514,7 @@ describe("composerContextRecords", () => { expect( isSameComposerContextPayload(base, { ...base, - elements: base.elements?.map((element) => ({ + elements: base.elements!.map((element) => ({ ...element, htmlPreview: '', })), @@ -523,7 +523,7 @@ describe("composerContextRecords", () => { expect( isSameComposerContextPayload(base, { ...base, - elements: base.elements?.map((element) => ({ + elements: base.elements!.map((element) => ({ ...element, source: { functionName: "Checkout", diff --git a/packages/contracts/src/baseSchemas.test.ts b/packages/contracts/src/baseSchemas.test.ts index 3e2c05c35058..d1bcd4c153a0 100644 --- a/packages/contracts/src/baseSchemas.test.ts +++ b/packages/contracts/src/baseSchemas.test.ts @@ -48,8 +48,9 @@ describe("ForwardCompatibleArray", () => { ]); }); - it("sends an element it cannot encode as a hole instead of failing the array", () => { - const Named = ForwardCompatibleArray(Schema.Struct({ name: TrimmedNonEmptyString })); + const Named = ForwardCompatibleArray(Schema.Struct({ name: TrimmedNonEmptyString })); + + it("drops an element it cannot encode instead of failing the array", () => { const wire = JSON.parse( JSON.stringify( Schema.encodeUnknownSync(Schema.toCodecJson(Named))([ @@ -59,9 +60,34 @@ describe("ForwardCompatibleArray", () => { ]), ), ); - expect(wire).toEqual([{ name: "a" }, null, { name: "b" }]); + expect(wire).toEqual([{ name: "a" }, { name: "b" }]); expect(fromWire(Named)(wire)).toEqual([{ name: "a" }, { name: "b" }]); }); + + it("drops it too when a wrapper reads the encoded array as JSON values", () => { + // How context records are bounded before forward-compatible decoding. + const Wrapped = Schema.Array(Schema.Unknown).pipe(Schema.decodeTo(Named)); + expect( + Schema.encodeUnknownSync(Schema.toCodecJson(Wrapped))([{ name: "a" }, { name: " " }]), + ).toEqual([{ name: "a" }]); + }); + + it("still drops the null holes a server on an earlier build sends", () => { + expect(fromWire(Named)([{ name: "a" }, null, { name: "b" }])).toEqual([ + { name: "a" }, + { name: "b" }, + ]); + }); + + it("does not accept holes as a decoded value", () => { + expect(Schema.is(Named)([undefined])).toBe(false); + // A sparse array's missing index is a hole too. + const sparse: Array<{ name: string }> = [{ name: "a" }]; + sparse.length = 2; + expect(Schema.is(Named)(sparse)).toBe(false); + expect(() => Named.make(sparse)).toThrow(); + expect(Schema.is(Named)([{ name: "a" }])).toBe(true); + }); }); describe("ForwardCompatibleUnion", () => { diff --git a/packages/contracts/src/baseSchemas.ts b/packages/contracts/src/baseSchemas.ts index adb64a1a4a6f..3ca88df66b19 100644 --- a/packages/contracts/src/baseSchemas.ts +++ b/packages/contracts/src/baseSchemas.ts @@ -140,13 +140,31 @@ export const ForwardCompatibleArray = (element: Elem Schema.UndefinedOr(Schema.toType(element)).pipe( Schema.catchEncoding(() => Effect.succeedSome(undefined)), ), + ).check( + // The holes above are an encoding detail: a decoded value has none, so + // `Schema.is` and `make` still reject an array that does. Aborts, so a + // later check on the array never sees a hole. + Schema.makeFilter( + (values) => { + // Every index, not `every`, which skips the holes of a sparse array. + for (let index = 0; index < values.length; index++) { + if (values[index] === undefined) return false; + } + return true; + }, + { expected: "an array without holes" }, + true, + ), ), SchemaTransformation.transform< ReadonlyArray, ReadonlyArray >({ decode: (values) => values.filter((value) => value !== undefined), - encode: (values) => values, + // An element that fails its own checks is dropped before the wire, so + // a wrapper that reads the encoded array as JSON values never meets + // the hole it left. + encode: (values) => values.filter((value) => value !== undefined), }), ), ) as unknown as ForwardCompatibleArray; diff --git a/packages/contracts/src/composerContext.test.ts b/packages/contracts/src/composerContext.test.ts index 87aa403cd6f7..b43e59c2840b 100644 --- a/packages/contracts/src/composerContext.test.ts +++ b/packages/contracts/src/composerContext.test.ts @@ -1,4 +1,6 @@ import { describe, expect, it } from "vite-plus/test"; +import * as Cause from "effect/Cause"; +import * as Exit from "effect/Exit"; import * as Option from "effect/Option"; import * as Schema from "effect/Schema"; @@ -197,6 +199,45 @@ describe("OrchestrationMessageContext", () => { ).toThrow(); }); + it("sends a message without a record it cannot encode", () => { + const wire = Schema.encodeUnknownSync(Schema.toCodecJson(OrchestrationMessageContext))({ + version: 1, + records: [ + decodeContext({ version: 1, records: [knownRecords.terminal] }).records[0], + { ...knownRecords.terminal, contextId: "ctx_2", terminalLabel: " " }, + ], + }); + expect(decodeContext(wire).records.map((record) => record.contextId)).toEqual(["ctx_1"]); + }); + + it("sends a message without the records the wire cannot carry", () => { + const wire = Schema.encodeUnknownSync(Schema.toCodecJson(OrchestrationMessageContext))({ + version: 1, + records: [ + decodeContext({ version: 1, records: [knownRecords.terminal] }).records[0], + { + ...base, + contextId: "ctx_2", + kind: "future-kind", + label: "x", + payload: { count: Number.NaN }, + }, + { ...knownRecords["review-comment"], contextId: "ctx_3", fenceLanguage: undefined }, + // JSON.stringify throws on a bigint on every engine. + { ...base, contextId: "ctx_4", kind: "future-kind", label: "y", payload: { n: 1n } }, + ], + }); + expect(decodeContext(wire).records.map((record) => record.contextId)).toEqual(["ctx_1"]); + }); + + it("reports a hole as a schema issue, even when collecting every issue", () => { + const result = Schema.decodeUnknownExit(Schema.toType(OrchestrationMessageContext))( + { version: 1, records: [undefined] }, + { errors: "all" }, + ); + expect(Exit.isFailure(result) && Cause.hasFails(result.cause)).toBe(true); + }); + it("normalizes decoded record identifiers", () => { const context = decodeContext({ version: 1, diff --git a/packages/contracts/src/composerContext.ts b/packages/contracts/src/composerContext.ts index 6d8975a0100d..116c141c9dfb 100644 --- a/packages/contracts/src/composerContext.ts +++ b/packages/contracts/src/composerContext.ts @@ -155,6 +155,9 @@ export const ElementContextRecord = Schema.Struct({ }); export type ElementContextRecord = typeof ElementContextRecord.Type; +// Optional record fields are `optionalKey`: a record holding an explicit +// `undefined` cannot be sent as JSON, so it fails its own check and is dropped +// alone instead of failing the whole message. export const PreviewAnnotationContextRecord = Schema.Struct({ ...recordBase, kind: Schema.Literal("preview-annotation"), @@ -165,14 +168,14 @@ export const PreviewAnnotationContextRecord = Schema.Struct({ targetSummary: ShortString, styleChanges: Schema.Array(ShortString).check(Schema.isMaxLength(200)), /** Picked elements inside the annotation, with the detail the agent needs to find them. */ - elements: Schema.optional(Schema.Array(ElementContextDetails).check(Schema.isMaxLength(50))), + elements: Schema.optionalKey(Schema.Array(ElementContextDetails).check(Schema.isMaxLength(50))), /** Original target ids and edits allow pasted annotations to retain exact style changes. */ - elementIds: Schema.optional(Schema.Array(ShortString).check(Schema.isMaxLength(50))), + elementIds: Schema.optionalKey(Schema.Array(ShortString).check(Schema.isMaxLength(50))), /** Region and stroke geometry is lossy on purpose, but their counts feed the target summary, so a pasted annotation still says what it marked. */ - regionCount: Schema.optional(NonNegativeInt), - strokeCount: Schema.optional(NonNegativeInt), - styleChangeDetails: Schema.optional( + regionCount: Schema.optionalKey(NonNegativeInt), + strokeCount: Schema.optionalKey(NonNegativeInt), + styleChangeDetails: Schema.optionalKey( Schema.Array( Schema.Struct({ targetId: ShortString, @@ -184,7 +187,7 @@ export const PreviewAnnotationContextRecord = Schema.Struct({ ).check(Schema.isMaxLength(200)), ), /** The screenshot travels as its own image record; this links the two. */ - screenshotContextId: Schema.optional(ComposerContextId), + screenshotContextId: Schema.optionalKey(ComposerContextId), }); export type PreviewAnnotationContextRecord = typeof PreviewAnnotationContextRecord.Type; @@ -199,8 +202,8 @@ export const ReviewCommentContextRecord = Schema.Struct({ rangeLabel: ShortString, text: BoundedString(COMPOSER_CONTEXT_REVIEW_TEXT_MAX_CHARS), diff: BoundedString(COMPOSER_CONTEXT_REVIEW_DIFF_MAX_CHARS), - fenceLanguage: Schema.optional(BoundedString(64)), - pullRequest: Schema.optional(PullRequestContextMetadata), + fenceLanguage: Schema.optionalKey(BoundedString(64)), + pullRequest: Schema.optionalKey(PullRequestContextMetadata), }).check(Schema.makeFilter((record) => record.endIndex >= record.startIndex)); export type ReviewCommentContextRecord = typeof ReviewCommentContextRecord.Type; @@ -231,6 +234,8 @@ export const ThreadContextRecord = Schema.Struct({ }); export type ThreadContextRecord = typeof ThreadContextRecord.Type; +const isJson = Schema.is(Schema.Json); + /** * Catch-all for kinds this build does not know. Known discriminators are excluded so a * malformed known record fails its own schema instead of sliding through unchecked. @@ -241,12 +246,16 @@ export const UnknownContextRecord = Schema.Struct({ kind: ComposerContextKind.check(Schema.isPattern(KNOWN_KIND_PATTERN)), payload: Schema.Unknown.check( Schema.makeFilter((payload) => { + let encoded: string | undefined; try { - const encoded = JSON.stringify(payload); - return encoded !== undefined && encoded.length <= 64_000; + encoded = JSON.stringify(payload); } catch { + // Cycles, bigints, and payloads nested deeper than this engine's stack. return false; } + // Only JSON values, so a payload the wire cannot carry (a Date, NaN, an + // undefined field) fails this record alone instead of the whole message. + return encoded !== undefined && encoded.length <= 64_000 && isJson(payload); }), ), });