Repository navigation
fix(desktop): keep denied path probes unknown instead of absent - #9722
2SinceJuly wants to merge 6 commits into
Conversation
State-directory lstat, the shared regular-file stat, and apt's signed-release probe all turned any failure into a negative filesystem fact, so a folder a capture could not reach through a denied parent was recorded as absent at start, and a package file or release that merely could not be checked was reported as not a regular file or as missing. Only ENOENT and ENOTDIR now establish absence. ApplicationStateDirectories records exists_at_start as null for other failures, and hostFileKind lets package-file and apt-release callers say the check failed, each with its own reason, rather than asserting a fact they could not establish.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Merged
Verified on the merge commit: those three test files 88/88, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b73ce9476
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (inRelease === null) return null; | ||
| if (inRelease) return [`${prefix}InRelease`]; | ||
| const signed = await hostFileKind(`${prefix}Release.gpg`); | ||
| if (signed === null) return null; |
There was a problem hiding this comment.
When InRelease is known absent, Release.gpg cannot be probed (for example, it is a symlink into a denied directory), and Release is known absent, this early return reports that the release files could not be checked even though neither supported release representation can be complete. Probe the required Release member before collapsing the detached pair to unknown; if it is absent, signedRelease can return undefined, otherwise retain the unknown result.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8336971fd. Release is now probed before the detached pair collapses, so a Release known to be absent returns undefined (apt holds no signed release file) whether or not Release.gpg could be read; a Release that could not be probed, or is present while Release.gpg could not be, still returns null (could not be checked).
Verified: src/desktop/DebianPackageInventory.test.ts, DebianPackageFile.test.ts and AptRepositoryInventory.test.ts — 32 passed, 2 skipped (the apt-config case needs Linux). tsc --noEmit and npm run lint clean. I did not add a test that reproduces the exact denied-symlink state — the local probe ran as the same user, so stat could not be made to return EACCES reliably; the fixture suite covers the three reachable outcomes through hostFileKind.
There was a problem hiding this comment.
No change. The branch already probes the required member before collapsing the pair: when Release.gpg is unknown it reads Release (src/desktop/AptRepositoryInventory.ts:162-166), and only returns undefined when Release is known absent. Confirmed on the current head.
codex observed that an unprobeable Release.gpg short-circuited to unknown even when Release was readable and absent, so a sources entry neither signed representation covers was reported as uncheckable rather than as holding no signed release file. Probe Release first and let a known absence settle the pair; a probe that could not tell still reports unknown when Release is present.
…rors Upstream main now records the failed probe's reason on each state directory and asserts it in its own test, which covers the same denial case this branch added. Keep the upstream shape and drop the startExistence helper it replaced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51f7194743
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /** Whether a host path names a regular file, following links; false when the check could not tell. */ | ||
| export const isHostFile = async (path: string): Promise<boolean> => | ||
| (await stat(path).catch(() => undefined))?.isFile() === true; | ||
| (await hostFileKind(path)) === true; |
There was a problem hiding this comment.
When an ELF RUNPATH/RPATH candidate sits under a denied parent (or stat hits another non-absence failure), hostFileKind returns null, but this expression converts it back to false. In the desktop package path, LibraryPackageInventory.ts:180 passes this wrapper to libraryClosure, whose libraryPackages.ts:114 branch omits false candidates; the capture can then mark the soname missing or select a later linker-cache path without recording that the higher-priority candidate was uncheckable. Propagate the unknown/error through the library-walk contract instead of retaining the boolean collapse.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d948f8054. Root cause: hostFileKind has three outcomes, but LibraryPackageInventory handed the walk a wrapper (isHostFile) that collapsed them to a boolean, so a RUNPATH candidate under a denied parent read as absent.
Structural fix: the walk's isFile contract takes boolean | null and libraryCandidates keeps a candidate it could not check, so host.dependencies sees it and records it in unreadable rather than letting the soname go missing or a later cache path win. isHostFile had no other caller and is removed.
Pin: "treats a probe that could not tell as a candidate instead of an absence" in libraryPackages.test.ts — it fails on the previous head. Local: that file 17/17, tsc --noEmit clean. src/desktop/ExportedAppPython.test.ts has 3 failures on this branch that also fail with my diff reverted, so they are not from this change.
hostFileKind has three outcomes, but the library walk's probe was wrapped back into a boolean, so a RUNPATH candidate under a denied parent read as absent. The walk then reported the soname missing or selected a later linker-cache path while the higher-priority candidate had never been ruled out. The walk now takes the tri-state probe and keeps a candidate it could not check; the lossy isHostFile wrapper has no other caller and is removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d948f80549
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (await host.isFile(candidate)) yield candidate; | ||
| // A probe that could not tell is a candidate that might load, not an | ||
| // absent one: the linker tries it before any later directory or cache hit. | ||
| if ((await host.isFile(candidate)) !== false) yield candidate; |
There was a problem hiding this comment.
When a RUNPATH/RPATH directory denies search with EACCES and a later directory contains the soname, glibc continues and loads the later file; I verified this in the inspected Linux environment with RUNPATH=denied:readable. This branch instead yields the null candidate, after which production elfDependencies returns an identity-less failure and the loop at lines 192-200 breaks, so the capture omits the library/package the launched app actually uses. This is fresh evidence against the earlier thread's assumption that the denied candidate blocks linker search; preserve the probe errno in the walk contract and continue on loader-skippable failures.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right, and the previous push did not actually change this path. libraryCandidates yielded the denied candidate, host.dependencies returned a null-identity failure for it, and the loop broke on that failure, so every later directory and cache entry was never tried.
Fixed in 2cfb3c4: unprobeable candidates now trail the searchable directories and the cache, and a null-identity failure on them is remembered instead of breaking the loop. A readable directory holding the soname therefore wins, and when nothing else provides it the denied candidate is reported under unreadable rather than missing, since a folder with denied search may still hold the library.
Tests added for both outcomes in libraryPackages.test.ts; libraryPackages and LibraryPackageInventory suites pass, tsc --noEmit and the repo lint/format checks are clean.
…ot open The earlier change kept an unprobeable candidate in the walk, but the walk's first null-identity failure breaks the candidate loop, so a denied RUNPATH directory still hid every later directory and cache entry. The dynamic linker skips a folder whose search it is denied and keeps looking, which I confirmed for this branch's shape: glibc continues after EACCES and loads the later file. Unprobeable candidates now trail the searchable ones and the cache, and are consulted only when nothing else provides the soname; they are then recorded as unreadable rather than missing, since a denied folder may still hold the library.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cfb3c4011
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ? [`${prefix}Release.gpg`, `${prefix}Release`] | ||
| : undefined; | ||
| const inRelease = await hostFileKind(`${prefix}InRelease`); | ||
| if (inRelease === null) return null; |
There was a problem hiding this comment.
The fresh case left after the earlier detached-pair fix is an unknown InRelease with a known-complete detached pair: when only ${prefix}InRelease is unprobeable (for example, it is a symlink into a denied directory) but readable ${prefix}Release.gpg and ${prefix}Release files are present, this early return never probes them. keyCheck consequently skips gpgv and loses a verifiable signing key even though either release representation satisfies this function's contract; probe the pair and return it when both members are known files, reserving null for cases where no representation can be established.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
Summary
Fixes #9703. A state-directory check, the shared host regular-file check, and apt's signed-release check each turned any
stat/lstatfailure into a filesystem fact, so a folder the capture could not reach through a denied parent was recorded asexists_at_start: false, a package file that could not be checked was called "not a regular file", and a release that could not be looked at was reported as missing.Problem and expected behavior
On a POSIX host where a parent folder denies search (
EACCES), on basec52a9b9d7:src/desktop/ApplicationStateDirectories.tscaught everylstaterror and producedexists_at_start: false, though the folder was present before and after the capture.isHostFileinsrc/desktop/HostFileRead.tscaught everystaterror, soDebianPackageFilesaid<path> is not a regular fileabout a regular file, andAptRepositoryInventorysaidapt holds no signed release fileabout a file that was there.AGENTS.mdrequires unresolved identity to remain unknown rather than be treated as absent;HomeStateWalker.statPathalready models this withnullfor absent andundefinedfor unknown. OnlyENOENT/ENOTDIRestablish absence.Change and scope
ApplicationStateDirectoriesrecordsexists_at_start: nullfor failures other thanENOENT/ENOTDIR. The inventory schema acceptsboolean().nullable(), the same conventionpackage_lists_presentalready uses.HostFileRead.hostFileKindreturnsboolean | nullwith the same rule.DebianPackageFileandAptRepositoryInventoryuse it to say the check failed, each with its own reason, instead of asserting a negative.isHostFileremains for the library-search caller, whose contract needs a boolean.docs/desktop-application-capture.mdsays existence at start stays unknown when the check cannot reach it.Not included: the package-file probe still discards the error kind between the kind check and the following
stat; that path already reports "could not be read", which is accurate, so it was left alone.Contract and boundary impact
true/falsefor existence or read the reason strings.exists_at_start: null, which JSON Schema represents as["boolean", "null"].Expected effects:
Evidence and regression coverage
ApplicationStateDirectories.test.ts(new),DebianPackageFile.test.ts,DebianPackageInventory.test.ts. Each builds a fixture under a folder chmod'd0o000and asserts the unknown outcome; they also keep the present/absent cases.HEADand the new tests kept, all three new tests fail (3 failed | 31 passed); with the fix they pass. The tests are skipped onwin32and when run as root, since root ignores the mode bits.staterror codes are covered by the same catch but were not exercised individually.Validation performed
npm run check:fast— typecheck and lint passed.npm run test:focused -- src/desktop/ApplicationStateDirectories.test.ts src/desktop/DebianPackageFile.test.ts src/desktop/DebianPackageInventory.test.ts src/desktop/LinuxApplicationInventory.test.ts src/desktop/LibraryPackageInventory.test.ts src/domain/desktop/desktopCapture.test.ts— 72 passed, 3 skipped.npx prettier --checkon the changed files — passed.Compatibility, safety, and release
nullexistence value;z.boolean().nullable()accepts both old and new captures.Author readiness
Human owner: @2SinceJuly