test(install-vm): cover authenticated daemon upgrades - #934
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR expands the optional Firecracker install suite to build and exercise authenticated daemon upgrades, and strengthens fixture and release-evidence validation.
Confidence Score: 3/5This PR should not merge until fixture copying rejects or safely handles checkout symlinks that can redirect writes outside the isolated build tree. The new fixture builder recreates checkout-controlled symlinks and then writes modified package and protocol contents through fixed destination paths, allowing an opt-in maintainer workflow to overwrite an external file. Files Needing Attention: test/cli/install-vm/prepare-daemon-upgrade-fixtures.ts
|
| Filename | Overview |
|---|---|
| test/cli/install-vm/prepare-daemon-upgrade-fixtures.ts | Adds daemon fixture attestation and isolated builds, but preserved checkout symlinks can redirect later fixed-path writes and the helper directly accesses process.env. |
| test/cli/install-vm/prepare-fixtures.ts | Extends fixture schema, packages, binary digest derivation, and symlink-resistant fixture verification. |
| test/cli/install-vm/results.ts | Adds required-evidence enforcement and filesystem-backed validation of authenticated daemon migration artifacts. |
| test/cli/install-vm/scenarios/authenticated-daemon-upgrade.sh | Exercises incumbent retirement and reconnection of two original authenticated clients with process-identity evidence. |
| test/cli/install-vm/validate-release-result.ts | Supports targeted validation and binds daemon evidence to locally verified fixture binaries and build inputs. |
| test/cli/install-vm/contract.ts | Defines and validates the closed grammar for scenario-specific required evidence and command expectations. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Checkout[Source checkout] --> Copy[Copy checkout and dependency snapshot]
Copy --> A[Rewrite and build daemon revision A]
Copy --> B[Rewrite and build daemon revision B]
A --> Packages[Checksum-bound fixture packages]
B --> Packages
Packages --> VM[Authenticated upgrade VM scenario]
VM --> Evidence[Commands, observations, and artifacts]
Evidence --> Validation[Release evidence validation]
Prompt To Fix All With AI
### Issue 1
test/cli/install-vm/prepare-daemon-upgrade-fixtures.ts:189-191
**Symlinks escape build isolation**
If a dirty or untrusted checkout replaces `package.json` or `src/session/protocol.ts` with an escaping symlink, `copyCheckout` recreates that symlink and `buildVariant` writes through it, overwriting the external target under the maintainer's permissions. Reject escaping checkout symlinks or verify these destination paths are contained regular files before rewriting them.
**How this was verified:** The copied symlink reaches `writeFileSync` without an intervening containment or regular-file check.
### Issue 2
test/cli/install-vm/prepare-daemon-upgrade-fixtures.ts:133-135
**Compiler bypasses environment validation**
The new compiler helper reads and spreads `process.env` directly, bypassing the repository's required validated environment-access mechanism and making fixture compiler configuration depend on unvalidated ambient values. Pass the required environment explicitly through the prescribed environment API instead.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "test(install-vm): cover authenticated da..." | Re-trigger Greptile
| if (stat.isSymbolicLink()) { | ||
| symlinkSync(readlinkSync(source), target); | ||
| continue; |
There was a problem hiding this comment.
Symlinks escape build isolation
If a dirty or untrusted checkout replaces package.json or src/session/protocol.ts with an escaping symlink, copyCheckout recreates that symlink and buildVariant writes through it, overwriting the external target under the maintainer's permissions. Reject escaping checkout symlinks or verify these destination paths are contained regular files before rewriting them.
How this was verified: The copied symlink reaches writeFileSync without an intervening containment or regular-file check.
Context Used: security.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/cli/install-vm/prepare-daemon-upgrade-fixtures.ts
Line: 189-191
Comment:
**Symlinks escape build isolation**
If a dirty or untrusted checkout replaces `package.json` or `src/session/protocol.ts` with an escaping symlink, `copyCheckout` recreates that symlink and `buildVariant` writes through it, overwriting the external target under the maintainer's permissions. Reject escaping checkout symlinks or verify these destination paths are contained regular files before rewriting them.
**How this was verified:** The copied symlink reaches `writeFileSync` without an intervening containment or regular-file check.
**Context Used:** security.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/security.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid. Checkout copying now rejects source escapes, absolute links, links that escape after relocation, and symlinked destination parents. The two mutable source paths must also be contained regular files with no symlink component, and both are validated before either write. Added leaf, parent, relocation/re-entry, contained-link, and unchanged-external-target regression coverage.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
| const bunLink = path.join(compilerBin, "bun"); | ||
| symlinkSync(bunExecutable, bunLink); | ||
| const env = { |
There was a problem hiding this comment.
Compiler bypasses environment validation
The new compiler helper reads and spreads process.env directly, bypassing the repository's required validated environment-access mechanism and making fixture compiler configuration depend on unvalidated ambient values. Pass the required environment explicitly through the prescribed environment API instead.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/cli/install-vm/prepare-daemon-upgrade-fixtures.ts
Line: 133-135
Comment:
**Compiler bypasses environment validation**
The new compiler helper reads and spreads `process.env` directly, bypassing the repository's required validated environment-access mechanism and making fixture compiler configuration depend on unvalidated ambient values. Pass the required environment explicitly through the prescribed environment API instead.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
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!
There was a problem hiding this comment.
Not changing this one. The fixture helper deliberately accepts an injected environment and otherwise inherits the invoking build environment, matching this repository’s script conventions. It prepends a private compiler directory, pins the attested Bun executable, and verifies both command resolution and the resolved executable before building; Hunk has no prescribed validated-environment abstraction to use here.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
That context is fair. Since this helper intentionally accepts an injected environment or inherits the invoking build environment, then prepends a private bun directory and verifies both command resolution and the attested executable, the validated-environment concern does not apply here. No change is needed; I’m withdrawing this comment.
b1b72cc to
78b2e1c
Compare
78b2e1c to
219eca3
Compare
219eca3 to
a8931ed
Compare
a8931ed to
95334a1
Compare
Stack
#932 → #933 → 3 of 3. Merge bottom-to-top.
Problem
Synthetic install fixtures cannot prove that a real installed Hunk binary preserves an authenticated incumbent, waits for quiescence, launches exactly one upgraded successor, and reconnects original windows without restart.
Approach
Existing synthetic
900.0.xtopology fixtures remain unchanged.Verification
bun run typecheckauthenticated-daemon-upgradescenario — passedtmp/install-vm/runs/2026-08-30T23-38-52-916Z-2424467/result.jsonf81980fdcea401342c774f8df45ea70448572d1e2d9b1867813706b1b2df83e822d64717cdd012b11fe285f4cffbf7a6cfa645ce571776ab36fd571875b409446c4e8951ec2383703cf39545bf1f8e4a56dd9448dfdd488cc1d6657dc8d46d95/f453712df2a6684cfe9b72d11145d7758e450952ea0b05f0ff97a70077540d9abun run changeset:statusgit diff --checkKnown limitations
The complete Firecracker install matrix was not rerun; this PR targets the authenticated daemon-upgrade scenario. Firecracker coverage is Linux x64 only.
This PR description was generated by Pi using gpt-5.6-sol