diff --git a/src/application/ProviderOperationExecution.ts b/src/application/ProviderOperationExecution.ts index bf4ad4b0e..966a43f29 100644 --- a/src/application/ProviderOperationExecution.ts +++ b/src/application/ProviderOperationExecution.ts @@ -43,7 +43,7 @@ export const executeProviderOperation = async < provider: ProviderOperationPort, confidence: "observed" | "derived", ): Promise> => { - if (options?.signal?.aborted === true) + if (options?.signal?.aborted) return err(new AnalysisCancelledError(operation)); const request = schema.safeParse({ operation, input }); if (!request.success) @@ -51,8 +51,10 @@ export const executeProviderOperation = async < const executed = await provider.execute(request.data, options); if (!executed.ok) return executed; const execution = executed.value; - return ok( - createEvidence(execution.subject ?? undefined, execution.provider, { + const evidence = createEvidence( + execution.subject ?? undefined, + execution.provider, + { operation, parameters: jsonObjectSchema.parse(request.data.input), result: execution.result, @@ -60,6 +62,16 @@ export const executeProviderOperation = async < limitations: execution.limitations, locations: execution.locations, confidence, - }), + }, ); + // A provider that cannot observe the signal may finish its work after the + // caller cancelled, so re-check rather than reporting a completed operation + // the caller no longer wants. The observation it completed is still this + // provider's Evidence, so the cancellation carries it as the partial + // observation instead of discarding a finished result. + if (options?.signal?.aborted) + return err( + new AnalysisCancelledError(operation, { partialObservation: evidence }), + ); + return ok(evidence); }; diff --git a/src/application/SupportedClients.ts b/src/application/SupportedClients.ts index 55a651a52..7c3811a23 100644 --- a/src/application/SupportedClients.ts +++ b/src/application/SupportedClients.ts @@ -539,7 +539,7 @@ export const SUPPORTED_CLIENT_DEFINITIONS = [ configPath: ({ home, platform, env }: ClientPathContext) => join(devinDirectory({ home, platform, env }), "mcp_config.json"), markerPath: devinDirectory, - format: "json", + format: "devin", }, { name: "opencode", diff --git a/src/application/apktool/ApktoolResourceAnalysisService.test.ts b/src/application/apktool/ApktoolResourceAnalysisService.test.ts index 6242bb54d..2220ce9e6 100644 --- a/src/application/apktool/ApktoolResourceAnalysisService.test.ts +++ b/src/application/apktool/ApktoolResourceAnalysisService.test.ts @@ -83,3 +83,37 @@ it("preserves cancellation before execution", async () => { if (result.ok) return; expect(result.error).toBeInstanceOf(AnalysisCancelledError); }); + +it("preserves a cancellation that races a completed observation", async () => { + const controller = new AbortController(); + const racingPort: ApktoolResourceAnalysisPort = { + async inspectAvailability() { + return { status: "available", code: null, reason: null, diagnostics: {} }; + }, + async close() {}, + async execute() { + // The provider may finish its work before the abort is delivered, so a + // successful result is not by itself proof the caller still wants it. + controller.abort(); + return ok(execution); + }, + }; + const result = await new ApktoolResourceAnalysisService(racingPort).execute( + "decode_android_resources", + { path: "/targets/app.apk", include_strings: true }, + { signal: controller.signal }, + ); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toBeInstanceOf(AnalysisCancelledError); + // The observation the provider completed is retained on the cancellation, so + // a session still records the Evidence instead of losing finished work. + const retained = parseEvidence(result.error.partialObservation); + expect(retained.operation).toBe("decode_android_resources"); + expect(retained.provider).toEqual(identity); + expect(retained.parameters).toEqual({ + path: "/targets/app.apk", + include_strings: true, + }); + expect(retained.confidence).toBe("observed"); +}); diff --git a/src/application/javascript/JavaScriptArtifactAnalysis.ts b/src/application/javascript/JavaScriptArtifactAnalysis.ts index e61d3ae94..6b48b448c 100644 --- a/src/application/javascript/JavaScriptArtifactAnalysis.ts +++ b/src/application/javascript/JavaScriptArtifactAnalysis.ts @@ -553,10 +553,10 @@ const collectSourceMapOriginals = ( if (!Array.isArray(names)) return invalidSourceMap(file, "Source map has no sources array."); const contents = map.sourcesContent; - if (!hasValidSourceMapContents(names.length, contents)) + if (!hasValidSourceMapContents(contents)) return invalidSourceMap( file, - "Source map sourcesContent must contain one string or null entry per source.", + "Source map sourcesContent must be an array of strings or null entries.", ); if (map.sourceRoot != null && typeof map.sourceRoot !== "string") return invalidSourceMap( diff --git a/src/cli/apktoolCommands.ts b/src/cli/apktoolCommands.ts index c7795f9d4..217889f5a 100644 --- a/src/cli/apktoolCommands.ts +++ b/src/cli/apktoolCommands.ts @@ -6,7 +6,10 @@ import { createApktoolResourceAnalysisProvider } from "../composition/apktool.js import { CLI_COMMANDS } from "../cliCommandNames.js"; import { logCliCommand } from "../cliLogging.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; -import type { ApktoolOperation } from "../domain/apktool/apktoolResourceAnalysis.js"; +import { + apktoolInputSchemas, + type ApktoolOperation, +} from "../domain/apktool/apktoolResourceAnalysis.js"; import type { Logger } from "pino"; import type { CliInstance } from "./types.js"; import { withCommandCancellation } from "./commandCancellation.js"; @@ -49,22 +52,19 @@ export const registerApktoolCommands = ( args: z.object({ path: z.string().min(1).describe("Local APK file to decode"), }), + // The locale grammar and the string-projection default belong to the + // apktool input owner. Read them from that schema rather than restating a + // narrower locale pattern, which refused the owner's own advertised + // `b+zh+Hans+CN` form at the CLI boundary. options: z.object({ - locale: z - .string() - .min(2) - .max(16) - .regex( - /^[a-z]{2,3}(-[A-Za-z0-9]{2,8})*$/u, - "Locale must look like de or pt-BR", - ) - .optional() - .describe( - "Project strings from res/values- instead of the default", - ), + locale: apktoolInputSchemas.decode_android_resources.shape.locale, strings: z .boolean() - .default(true) + .default( + apktoolInputSchemas.decode_android_resources.shape.include_strings.parse( + undefined, + ), + ) .describe( "Include string resources; use --no-strings to skip projecting them", ), diff --git a/src/domain/sourceMapContents.test.ts b/src/domain/sourceMapContents.test.ts index 06f07197b..dd80f39fb 100644 --- a/src/domain/sourceMapContents.test.ts +++ b/src/domain/sourceMapContents.test.ts @@ -2,10 +2,18 @@ import { expect, it } from "vitest"; import { hasValidSourceMapContents } from "./sourceMapContents.js"; -it("accepts absent map-level source contents and aligned nullable slots", () => { - expect(hasValidSourceMapContents(2, undefined)).toBe(true); - expect(hasValidSourceMapContents(2, null)).toBe(true); - expect(hasValidSourceMapContents(2, [null, "retained source"])).toBe(true); - expect(hasValidSourceMapContents(2, ["retained source"])).toBe(false); - expect(hasValidSourceMapContents(2, [null, 42])).toBe(false); +it("accepts absent map-level source contents and nullable string slots", () => { + expect(hasValidSourceMapContents(undefined)).toBe(true); + expect(hasValidSourceMapContents(null)).toBe(true); + expect(hasValidSourceMapContents([null, "retained source"])).toBe(true); + expect(hasValidSourceMapContents([null, 42])).toBe(false); + expect(hasValidSourceMapContents("retained source")).toBe(false); +}); + +it("leaves sourcesContent length to source decoding (ECMA-426 DecodeSourceMapSources)", () => { + expect(hasValidSourceMapContents([])).toBe(true); + expect(hasValidSourceMapContents(["retained source"])).toBe(true); + expect( + hasValidSourceMapContents(["a", null, "unused trailing content"]), + ).toBe(true); }); diff --git a/src/domain/sourceMapContents.ts b/src/domain/sourceMapContents.ts index 5d55810a9..c6a621f1a 100644 --- a/src/domain/sourceMapContents.ts +++ b/src/domain/sourceMapContents.ts @@ -1,12 +1,15 @@ -/** Validate optional source-map content against its source inventory. */ -export const hasValidSourceMapContents = ( - sourceCount: number, - contents: unknown, -): boolean => +/** + * Validate optional source-map content against its source inventory. + * + * ECMA-426 DecodeSourceMapSources reads a content slot only while its index is + * within `sourcesContent`, so a shorter array leaves trailing sources without + * content and a longer array contributes no extra sources. Only the entry types + * are checked here. + */ +export const hasValidSourceMapContents = (contents: unknown): boolean => contents === undefined || contents === null || (Array.isArray(contents) && - contents.length === sourceCount && contents.every( (content: unknown) => content === null || typeof content === "string", )); diff --git a/src/javascript/sourceMaps/SourceMapFormat.ts b/src/javascript/sourceMaps/SourceMapFormat.ts index f8975c9b6..a2edcab52 100644 --- a/src/javascript/sourceMaps/SourceMapFormat.ts +++ b/src/javascript/sourceMaps/SourceMapFormat.ts @@ -1,6 +1,5 @@ import { z } from "zod"; import { WEB_SOURCE_MAP_LIMITS } from "../../domain/webSourceLocation.js"; -import { hasValidSourceMapContents } from "../../domain/sourceMapContents.js"; import { sourceMapJsonText } from "../../domain/sourceMapEnvelope.js"; import type { EncodedSourceMapXInput } from "@jridgewell/trace-mapping"; @@ -267,18 +266,6 @@ const inspectLeaf = ( consumeRecords(budget, Array.isArray(rawNames) ? rawNames.length : 0); consumeRecords(budget, Array.isArray(rawContents) ? rawContents.length : 0); options.check?.(); - if ( - options.profile === "browser-collection" && - !hasValidSourceMapContents( - Array.isArray(rawSources) ? rawSources.length : 0, - rawContents, - ) - ) - throw new SourceMapFormatFailure( - "format", - "Source-map sourcesContent must align with its source declarations.", - ); - options.check?.(); const leaf = leafSchema.parse(node.value); const layout = inspectMappingLayout(leaf.mappings, options.check); consumeRecords(budget, layout.segments + layout.lines); diff --git a/tests/boundary/browser/webSourceMapFetcher.test.ts b/tests/boundary/browser/webSourceMapFetcher.test.ts index 6dcabb75b..5fab66f58 100644 --- a/tests/boundary/browser/webSourceMapFetcher.test.ts +++ b/tests/boundary/browser/webSourceMapFetcher.test.ts @@ -810,7 +810,6 @@ describe("web source-map sourcesContent validation", () => { it.each([ ["a scalar sourcesContent value", { sourcesContent: "source" }], ["a non-string, non-null source entry", { sourcesContent: [42] }], - ["a sourcesContent array with the wrong length", { sourcesContent: [] }], ])("reports %s as invalid", async (_description, content) => { const result = await fetchWebSourceMaps([request], input(), undefined, { fetch: () => @@ -841,6 +840,50 @@ describe("web source-map sourcesContent validation", () => { }); }); + it.each([ + ["shorter", ["import './a.ts';"]], + ["longer", ["import './a.ts';", null, "unused trailing content"]], + ])( + "keeps every source declaration when sourcesContent is %s than sources", + async (_length, sourcesContent) => { + const result = await fetchWebSourceMaps([request], input(), undefined, { + fetch: () => + Promise.resolve( + new Response( + JSON.stringify({ + version: 3, + sources: ["a.ts", "b.ts"], + sourcesContent, + names: [], + mappings: "", + }), + { status: 200 }, + ), + ), + }); + + expect(result.items[0]).toMatchObject({ + status: "included", + limitation: null, + original_sources: [ + { + source_index: 0, + declaration_index: 0, + source: `${origin}/assets/a.ts`, + artifact: { text: "import './a.ts';" }, + }, + { + source_index: 1, + declaration_index: 1, + source: `${origin}/assets/b.ts`, + artifact: null, + }, + ], + }); + expect(result.items[0]?.original_sources).toHaveLength(2); + }, + ); + it("accepts null source-map source contents", async () => { const result = await fetchWebSourceMaps([request], input(), undefined, { fetch: () => diff --git a/tests/boundary/cli/apktoolLocaleOption.test.ts b/tests/boundary/cli/apktoolLocaleOption.test.ts new file mode 100644 index 000000000..4bc596695 --- /dev/null +++ b/tests/boundary/cli/apktoolLocaleOption.test.ts @@ -0,0 +1,92 @@ +import { expect, it } from "vitest"; +import { Cli } from "incur"; +import pino from "pino"; + +import type { ApktoolResourceAnalysisPort } from "../../../src/application/apktool/ApktoolResourceAnalysisPort.js"; +import { createAnalysisExecution } from "../../../src/application/AnalysisProvider.js"; +import { registerApktoolCommands } from "../../../src/cli/apktoolCommands.js"; +import { presentOmittableDefaults } from "../../../src/cli/omittableDefaults.js"; +import type { ApktoolRequest } from "../../../src/domain/apktool/apktoolResourceAnalysis.js"; +import { ok } from "../../../src/domain/result.js"; + +const invoke = async (argv: readonly string[]) => { + const requests: ApktoolRequest[] = []; + const provider: ApktoolResourceAnalysisPort = { + async inspectAvailability() { + throw new Error("unexpected provider probe"); + }, + async close() {}, + async execute(request) { + requests.push(request); + return ok( + createAnalysisExecution( + { strings: [] }, + { id: "apktool", name: "Apktool", version: null }, + ), + ); + }, + }; + const cli = Cli.create("rea", { sync: false }); + registerApktoolCommands(cli, pino({ level: "silent" }), {}, provider); + presentOmittableDefaults(cli); + + let stdout = ""; + let exitCode = 0; + await cli.serve([...argv], { + env: {}, + exit: (code) => { + exitCode = code; + }, + stdout: (text) => { + stdout += text; + }, + }); + return { exitCode, stdout, requests }; +}; + +/** + * Locales the apktool input owner admits. The owner's own rejection message + * advertises `b+zh+Hans+CN`, so the CLI cannot be narrower than this list + * without refusing a caller-selected locale the decoding lane supports. + */ +const OWNER_ADMITTED_LOCALES = [ + "de", + "pt-BR", + "pt-rBR", + "zh-rCN", + "en-419", + "b+zh+Hans+CN", +] as const; + +it("forwards every locale the apktool input owner admits", async () => { + for (const locale of OWNER_ADMITTED_LOCALES) { + const result = await invoke([ + "decode-android-resources", + "/targets/app.apk", + "--locale", + locale, + "--json", + ]); + + expect(result.exitCode, `exit code for locale ${locale}`).toBe(0); + expect(result.requests, `request for locale ${locale}`).toEqual([ + { + operation: "decode_android_resources", + input: { path: "/targets/app.apk", include_strings: true, locale }, + }, + ]); + } +}); + +it("still refuses a locale the apktool input owner refuses", async () => { + const result = await invoke([ + "decode-android-resources", + "/targets/app.apk", + "--locale", + "not a locale", + "--json", + ]); + + expect(result.exitCode).not.toBe(0); + expect(result.requests).toEqual([]); +}); diff --git a/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts b/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts index aabfdd19b..1254d11dd 100644 --- a/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts +++ b/tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts @@ -135,7 +135,6 @@ it("retains every source declared by every included local source map", async () it.each([ ["a non-array sourcesContent value", { sourcesContent: "source" }], ["a non-string, non-null source entry", { sourcesContent: [42] }], - ["a source-content array with the wrong length", { sourcesContent: [] }], ])( "reports %s as an invalid local source map", async (_description, content) => { @@ -210,6 +209,51 @@ it("accepts omitted and explicitly null source-map source contents", async () => expect(result.graph.coverage.status).toBe("complete"); }); +it.each([ + ["shorter", ["export const answer = 42;"]], + ["longer", ["export const answer = 42;", null, "unused trailing content"]], +])( + "keeps every source declaration when sourcesContent is %s than sources", + async (length, sourcesContent) => { + const root = await createTestTempDirectory( + `rea-javascript-${length}-source-content-`, + ); + await writeFile( + join(root, "bundle.js.map"), + JSON.stringify({ + version: 3, + sources: ["a.ts", "b.ts"], + sourcesContent, + names: [], + mappings: "AAAA;ACAA", + }), + ); + + const result = await reconstructJavaScriptArtifact({ input_path: root }); + const originals = result.graph.nodes.filter( + ({ identity }) => identity.strategy === "source-map-original", + ); + + expect( + originals + .map(({ identity, observations }) => ({ + original_source: + "original_source" in identity ? identity.original_source : null, + source_index: observations[0]?.properties.source_index, + content_available: observations[0]?.properties.content_available, + })) + .sort( + (left, right) => + Number(left.source_index) - Number(right.source_index), + ), + ).toEqual([ + { original_source: "a.ts", source_index: 0, content_available: true }, + { original_source: "b.ts", source_index: 1, content_available: false }, + ]); + expect(result.graph.coverage.status).toBe("complete"); + }, +); + it("retains unnamed source declarations without shifting later source evidence", async () => { const root = await createTestTempDirectory("rea-javascript-unnamed-source-"); const text = JSON.stringify({