Skip to content

Pr 6332 - #6399

Closed
AadiyKhan wants to merge 15 commits into
nexu-io:mainfrom
AadiyKhan:pr-6332
Closed

Pr 6332#6399
AadiyKhan wants to merge 15 commits into
nexu-io:mainfrom
AadiyKhan:pr-6332

Conversation

@AadiyKhan

@AadiyKhan AadiyKhan commented Aug 4, 2026

Copy link
Copy Markdown

Why

This PR consolidates two critical daemon/runtime fixes to prevent split reviews:
1. **Antigravity Prompt Delivery (#5495):** The Antigravity CLI wrapper currently fails to pass prompts properly when invoked with `-p -` because the adapter attempts to pipe to `stdin` which the mock CLI mishandles.
2. **Plugin ZIP Extraction Memory & Resource Leaks:** The daemon previously used `JSZip` to buffer entire 50MB plugin uploads in memory, causing 150MB-250MB memory spikes per request. Furthermore, if extraction failed (e.g., zip-bomb expanded size exceeding the limit), the staging directory was left orphaned in the system temp folder, allowing attackers to exhaust disk space with repeated failed uploads.

What users will see

- Antigravity agent interactions now work flawlessly without hanging on the terminal greeting.
- Installing large plugins is significantly faster and uses less than 10MB of memory.
- The daemon is much more stable and resilient against malicious OOM or disk-exhaustion attacks during plugin uploads.

Surface area

- `apps/daemon/src/runtimes/defs/antigravity.ts`: Replaced `stdin` piping with an atomic temp file write for prompts, preserving `0600` permissions.
- `apps/daemon/src/services/plugin-installation.ts`: Swapped `JSZip` for `yauzl`. Used `stream.pipeline` to pipe decompressed file chunks directly to

fs.createWriteStream. Added try/catch cleanup in stageUploadedPluginZip to wipe the stagedFolder on extraction failure.

Validation

- [x] Typechecks pass (`tsc`).
- [x] Verified Antigravity connection-test suite passes with the new prompt delivery and correct flag parsing.
- [x] Added `test(daemon): add zip bomb extraction limit test` which asserts that an aggressively compressed ZIP bomb exceeding 50MiB rejects extraction AND cleans up the temp staging directory successfully.
- [x] Existing plugin installation SSRF and traversal tests continue to pass.

@lefarcen

lefarcen commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Thanks @AadiyKhan — this touches the daemon runtime/install path, so I’ve queued it with the right reviewer and will follow up on the PR template bits separately.

💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …):
Take over nexu-io/open-design#6399 until it merges — read https://raw.githubusercontent.com/nexu-io/looper/main/skills/pr-takeover/SKILL.md and follow it.

@lefarcen
lefarcen requested a review from PerishCode August 4, 2026 05:27
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Aug 4, 2026
@lefarcen

lefarcen commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Looks like you also have #6332 open touching the same daemon/runtime files and the same Antigravity prompt-delivery bug family. If this branch is the one you want to keep moving, it’d help to consolidate on a single PR so review doesn’t split across two copies.

@lefarcen

lefarcen commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🧪 Queued for QA validation — this PR changes live daemon/runtime behavior, so it should get a manual QA pass before merge. Nothing needed from you right now; we’ll update here once it’s validated. Thanks for the contribution! 🙏

@lefarcen lefarcen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AadiyKhan — thanks for putting up the Antigravity adapter update. Before pool review picks this up, could you fill in the PR body sections for Why / What users will see / Surface area / Validation? Right now it’s still mostly the template shell, so it’s hard to tell whether this is meant to stay scoped to the Antigravity prompt fix or also intentionally includes the plugin-installation hardening.

@PerishCode PerishCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed all changed ranges across the Antigravity adapter and plugin installation path. The ZIP streaming rewrite has a resource-lifecycle regression that can accumulate attacker-controlled temporary data, so it needs to be fixed before merge.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

readStream.on('data', (chunk: Buffer) => {
totalBytes += chunk.length;
if (totalBytes > maxBytes) {
readStream.destroy(new Error('zip extracted size exceeds 50 MiB'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean up the staging directory when streaming extraction fails. This new path writes each entry into stagedFolder before the aggregate-size check can reject the archive; when readStream.destroy(...) makes pipeline reject, extractPluginZipToFolder rejects before finishUploadedPluginInstall is called, and stageUploadedPluginZip has no catch/finally cleanup. A highly compressed upload that expands past the limit therefore leaves up to the configured limit (and possibly the crossing chunk) under the system temp directory on every request, allowing repeated rejected uploads to consume disk. Wrap extraction and installation in stageUploadedPluginZip with the same try/catch cleanup used by stageUploadedPluginFolder (while preserving finishUploadedPluginInstall's cleanup), and add a ZIP-bomb/oversized-expanded fixture test that asserts rejection and removal of the staging directory.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen

lefarcen commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Thanks — @PerishCode has now left the blocking review on the current head, so the next step is to address that cleanup/test path in plugin-installation.ts and push an update.

Please also tighten up the PR body when you do: it still needs the Why / What users will see / Surface area / Bug fix verification / Validation sections filled in clearly, especially since this branch now bundles the Antigravity fix together with the plugin-installation hardening.

@AadiyKhan

Copy link
Copy Markdown
Author

@lefarcen Thanks for the heads up! I've updated the PR description to use the standard template. This PR now intentionally consolidates both the Antigravity prompt fix and the plugin installation hardening to avoid splitting the reviews across two copies, so we can close #6332 in favor of this one.

@PerishCode Great catch on the staging directory leak. I've pushed a commit that:

  1. Wraps the extraction in stageUploadedPluginZip with the same try/catch cleanup used by stageUploadedPluginFolder.
  2. Adds a new ZIP-bomb extraction limit test (plugin-installation.test.ts) that asserts an oversized expanded archive properly rejects extraction and fully cleans up the temp staging directory.

Let me know if there's anything else needed before the QA validation pass!

@PerishCode PerishCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prior staging-directory cleanup blocker is addressed on this head, and I reviewed all changed ranges. One repository validation failure remains in the new dependency declarations and must be corrected before merge.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/package.json
"prom-client": "15.1.3",
"tar": "7.5.15",
"undici": "7.25.0"
"undici": "7.25.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pin both new dependency specs to exact versions. This line adds yauzl as ^3.4.0, and the same changed dependency block adds @types/yauzl as ^3.4.0; the repository guard reports both as violations because project dependencies must use exact versions or workspace:*. As written, pnpm guard exits 1, so the branch cannot satisfy the required merge validation and future installs could resolve unreviewed releases. Change both specs to 3.4.0 and refresh the lockfile with the pinned workspace pnpm version.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen

lefarcen commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Thanks for the update — this looks much closer now.

@PerishCode has already re-checked the new head and confirmed the staging-directory cleanup issue is fixed. The remaining blocker is the dependency pinning in apps/daemon/package.json: yauzl and @types/yauzl still need to move from ^3.4.0 to exact 3.4.0, with the lockfile refreshed using the repo’s pinned pnpm version. Once that’s pushed, QA can stay queued behind the normal review flow.

@AadiyKhan

Copy link
Copy Markdown
Author

done

@PerishCode PerishCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AadiyKhan I reviewed every changed range on the current head, including the Antigravity managed prompt-file handoff, atomic settings update, streamed plugin ZIP extraction, expanded-size enforcement, failure cleanup, dependency pins, and regression coverage. The two earlier blockers are resolved: rejected extraction now removes its staging directory, and the new dependency specs are exact. Repository guard and the focused ZIP/prompt-file tests pass, and the live PR checks are green. Nice work following through on the resource-lifecycle and validation feedback—this is ready from the implementation-review side.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen
lefarcen requested a review from ivy-ting August 4, 2026 06:03
@lefarcen
lefarcen requested a review from PerishCode August 4, 2026 08:48

@PerishCode PerishCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AadiyKhan I reviewed every changed range on the current head, including the Antigravity managed prompt-file handoff and atomic settings update, the streamed plugin ZIP extraction and expanded-size enforcement, staging cleanup on failure, exact dependency pins, refreshed Nix dependency hashes, and the regression coverage. The two earlier blockers remain resolved. The focused daemon tests (48 tests), daemon typecheck, repository guard, and live PR checks all pass. Thanks for following through carefully on the resource-lifecycle feedback and keeping the packaging hashes synchronized—this is ready from the implementation-review side.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@AadiyKhan

Copy link
Copy Markdown
Author

@lefarcen can you approve it again for runs , i have fixed the nix issue

@lefarcen

lefarcen commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Thanks for the heads-up — I can’t re-approve this from the bot side here.

I’ve re-requested @PerishCode on the new head so the implementation approval can refresh, and QA stays queued behind that as soon as this head is cleared.

@lefarcen
lefarcen requested a review from PerishCode August 4, 2026 09:23

Siri-Ray commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Closing this as a duplicate of #6332.

Both PRs currently point to the same head commit (72446415) and have an identical 15-commit, 7-file diff. Review history, current-head approval, manual QA tracking, and follow-up CI will continue on #6332 so the discussion and required checks stay in one place.

Thank you for following through on the Antigravity and plugin-installation fixes.

@Siri-Ray Siri-Ray closed this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/M PR changes 100-300 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants