Repository navigation
[tools] Replace APK utilities with skill file apps - #12108
Conversation
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
Code Review — ⚠️ Needs minor changes
Reviewed the top commit ([tools] Replace APK utilities with skill file apps) on top of the stacked base #12104. The refactor cleanly consolidates the standalone reader/decompressor executables into two skill-owned dotnet run --file apps plus a shared AssemblyStorePayload helper, and wires both apps into Windows CI via --help smoke runs. The path-traversal guard (GetSafeOutputFile), overwrite refusal, and unique-output checks are good defensive touches.
Findings
⚠️ 2 error-handling issues (inline):InvalidDataExceptionfrom the path-traversal guard is not inMain’s catch list in the extractor, andParseArchListthrows an uncaughtInvalidOperationExceptionon bad--archinput in the reader. Both surface as stack traces instead of clean messages.- 💡 1 cleanup: empty unused
ShowHelp ()in the reader.
None are blockers, but the two error-handling items are worth addressing so malformed input/malicious archives fail gracefully.
Notes
- CI status is currently pending (checks not yet reported) — verify the two new file-app build steps go green before merge.
- Reviewed as the single commit atop #12104 per the PR description; retarget to
mainonce #12104 lands.
Automated review; verify suggestions against full context.
Generated by Android PR Reviewer for #12108 · 106.2 AIC · ⌖ 18.8 AIC · ⊞ 6.8K
Comment /review to run again
jonathanpeppers
left a comment
There was a problem hiding this comment.
Otherwise this looks good. 👍
Move assembly-store inspection and managed assembly extraction into skill-owned .NET file-based apps. Remove the redundant executable projects and launch wrappers while keeping the shared compatibility parser and exercising both apps in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 55493571-86e2-4326-850e-ea55883aa5d8
Report invalid architecture filters and path traversal errors without stack traces, remove dead reader code, and correct the extractor usage text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 55493571-86e2-4326-850e-ea55883aa5d8
Relocate the shared parser and compatibility tests under the read-assembly-store skill, update all source and project consumers, and remove the obsolete assembly-store-reader-mk2 directory. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c81166b-f589-4e13-87d3-942acf3c9df2
75188b8 to
197d0cb
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Android PR Review — ⚠️ Minor suggestions (no blocking issues)
Reviewed as the single commit stacked on #12104: replacing the tools/assembly-store-reader* / decompress-assemblies executables with two dotnet run --file skill apps and moving the shared AssemblyStore parser + tests under .github/skills/read-assembly-store/.
Verdict
No bugs or correctness issues found. This is a clean, well-executed refactor. Two 💡 non-blocking suggestions posted inline.
Positive callouts
- Path-traversal defense is solid.
GetSafeOutputFilenormalizes withPath.GetFullPath+Path.GetRelativePathand rejects rooted /..-escaping paths, and writes useFileMode.CreateNewto refuse overwrites — good defense-in-depth for untrusted customer packages.HaveUniqueOutputsalso prevents output-name collisions up front. - Improved arch parsing.
TryParseArchListnow validates against the supported set and returns a graceful error instead of the oldParseArchListthrowing an uncaughtInvalidOperationException. - New
AssemblyStorePayload.TryExtractELFPayloadvalidates stream capabilities, guardsoffset/size > Int64.MaxValue, pools its copy buffer viaArrayPool, and restores stream position on the not-ELF path. - Relative
#:projectpaths verified correct for both file apps, and CI now smoke-builds both viadotnet run --file ... -- --help.
CI
Status is currently pending / no checks reported for 197d0cb. Not mergeable until the dotnet-android pipeline reports green — please confirm the two new "Build ... file app" steps and AssemblyStore.Tests pass.
Notes
- This PR is stacked on #12104 and should be retargeted to
mainafter that merges, per the description.
Prioritized: bugs > safety > performance > tests > duplication > consistency > docs. Suggestions are optional.
Generated by Android PR Reviewer for #12108 · 148.4 AIC · ⌖ 18.8 AIC · ⊞ 6.8K
Comment /review to run again
* ci: repoint watch-upstream to relocated AssemblyStore reader dotnet/android#12108 (commit f1aecf9) deleted tools/assembly-store-reader-mk2/ and moved the AssemblyStore parser we vendor in src/Sentry.Android.AssemblyReader/ to .github/skills/read-assembly-store/src/AssemblyStore/. Our watch matrix still tracked the old path, which will never receive another commit, so we would silently stop being alerted to upstream changes. Point the watcher at the new location. #skip-changelog Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Tweak comments --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
read-assembly-storeto invoke its bundled app directlyextract-android-assembliesskill for customer APK/AAB investigation and decompilation preparationStack
This PR is stacked on #12104, which provides the shared v1/v2/v3 and LZ4/Zstd compatibility layer. Review this PR as the single commit on top of that branch. It can be retargeted to
mainafter #12104 merges.File apps
The extractor handles:
payload, and_assembly_storewrappersCustomer inputs remain local. Extraction rejects path traversal, output-name collisions, and overwrites.
Validation
skill-creatorvalidation and package successfully