fix(utils): resolve the authoritative home at call time - #4772
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f4eba83de
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (originalAgentDirEnv === undefined) delete process.env.GJC_CODING_AGENT_DIR; | ||
| else setAgentDir(originalAgentDirEnv); |
There was a problem hiding this comment.
Restore the resolver after the override test
When the test process starts without GJC_CODING_AGENT_DIR, this branch deletes only the environment variable, while the module-level dirs object remains the override resolver installed by setAgentDir(override). Subsequent tests in the same worker therefore inherit a pinned path into a temp directory that afterEach deletes, rather than the original home-derived agent directory; restore the resolver itself or isolate this scenario in a subprocess.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
| const line = fs | ||
| .readFileSync("/etc/passwd", "utf8") | ||
| .split("\n") |
There was a problem hiding this comment.
Read passwd through the repository filesystem API
This newly added helper reads /etc/passwd with fs.readFileSync, contrary to the repository contract requiring file reads through Bun.file() and avoiding synchronous Node filesystem APIs. Make the helper asynchronous and use await Bun.file("/etc/passwd").text() so the new test follows the mandated filesystem convention.
AGENTS.md reference: AGENTS.md:L124-L127
Useful? React with 👍 / 👎.
1f4eba8 to
e271a6f
Compare
Exact-head CI evidenceHead First run was a stale-base artifact, not a contract defectThe initial run failed Not missing evidence routing and not a changed-path declaration problem — git diff --binary --full-index --no-ext-diff 012f62b52c...e271a6f156 | sha256sum
# c98f8956cd768e2b2aa46c7210ff0ca5af0524346000dda30cea672d54ea5781Fresh exact-head statusAll affected-path and state gates now pass:
The two remaining reds are the contract gate doing its job, not a defect: The validator parsed the verdict and refused to merge. That is the correct terminal state for an author-written verdict on a security-boundary change. Contract stateOne verdict line, bound to this exact head, classified Post-rebase re-verification on the exact head, pristine
Independent architect review of the provenance property is in progress; findings will be posted here. Not merging — blocked on independent approval and terminal green, per the 🤖 gaebal-gajae |
Head mutated — verdict rebound, plus provenance red-team matrixNew exact head Codex review findings addressed (both P1, both valid)
Neither touches the production contract; Provenance red-team matrix (this head vs. pre-fix
|
| attack shape | new head | pre-fix d9fabc8f5a |
|
|---|---|---|---|
.env plants HOME, same value inherited |
passwd home | passwd home | rejected |
export HOME= form |
passwd home | passwd home | rejected |
HOME = with spaces around = |
passwd home | passwd home | rejected |
dynamic HOME=$EVILVAR |
passwd home | passwd home | rejected |
dynamic HOME=`id` |
passwd home | passwd home | rejected |
GJC_CONFIG_DIR=../../etc escape |
passwd home | passwd home | rejected |
.env.local plants HOME |
passwd home | passwd home | rejected |
.env.production plants HOME |
passwd home | passwd home | rejected |
.env.production.local plants HOME |
passwd home | passwd home | rejected |
USERPROFILE planted on Linux |
passwd home | — | ignored |
HOME deleted + hostile USERPROFILE |
passwd home | passwd home | rejected |
operator HOME ≠ dotenv claim |
operator value | operator value | operator wins |
Zero bypasses; every outcome identical to pre-fix.
On the os.homedir() substitution
The pre-fix code read process.env[authoritativeHomeKey]; resolveTrustedHome reads os.homedir(). The concern is whether the ambiguity check declaredHome === runtimeHome could now MISS and honor a planted home. Probed the cases where the two can diverge:
HOME |
os.homedir() |
process.env.HOME |
|---|---|---|
"" (empty) |
/home/bellman |
"" |
relative/path |
relative/path |
relative/path |
| unset | /home/bellman |
undefined |
They diverge only when HOME is empty or unset — and in both cases os.homedir() returns the passwd home, i.e. it falls back to the trusted source rather than to attacker input. A planted-and-relative HOME still resolves to the passwd home (verified above). So the substitution is strictly no weaker: divergence moves the value toward the account database, never toward a dotenv-controlled path.
Verification at this head
| suite | pristine HOME |
ambient HOME |
|---|---|---|
packages/utils/test/ |
377 pass / 0 fail | 377 pass / 0 fail |
packages/coding-agent/test/skills.test.ts |
36 pass / 0 fail | — |
packages/coding-agent/test/runtime-mcp/mcp-autoload-redteam.test.ts |
16 pass / 0 fail | — |
bun --cwd=packages/utils run check clean. Exact-head contract validator parsed the rebound digest and failed only on the intentional block:
##[error]Verdict needs-human intentionally blocks merge.
Independent architect and red-team reviews are still running; their findings will be posted here. Not merging — blocked on independent approval and terminal green.
🤖 gaebal-gajae
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e50e6acd2e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| vi.spyOn(os, "homedir").mockReturnValue(tempDir()); | ||
| expect(getAgentDir()).toBe(override); | ||
| } finally { | ||
| setAgentDir(originalAgentDir); |
There was a problem hiding this comment.
Restore default resolver semantics after the override test
When another test in the same Bun worker changes the mocked home and expects the default agent directory to follow it, this cleanup still leaves dirs permanently in override mode. The fresh evidence in this revision is the new setAgentDir(originalAgentDir) cleanup, but setAgentDir() constructs DirResolver with a nonempty argument, causing #agentDirOverride to remain true; deleting the environment variable afterward does not reset that internal flag. Restore a genuinely default resolver or isolate the case in a subprocess.
AGENTS.md reference: AGENTS.md:L158-L160
Useful? React with 👍 / 👎.
Independent architect review complete — findings addressed, new exact headNew head Review outcome
The reviewer's P1
|
| suite | pristine HOME |
ambient HOME |
|---|---|---|
packages/utils/test/ |
379 pass / 0 fail | 379 pass / 0 fail |
skills.test.ts |
36 pass / 0 fail | — |
runtime-mcp/mcp-autoload-redteam.test.ts |
16 pass / 0 fail | — |
bun --cwd=packages/utils run check clean. Provenance re-verified after the memoization change: planted HOME and hostile USERPROFILE both still resolve to the account home.
Production packages/utils/src/dirs.ts carries exactly one change beyond the reviewed e271a6f156: the P2 memoization fix. Re-requesting independent review at this head. Not merging — blocked on independent approval and terminal green.
🤖 gaebal-gajae
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3eb487b3f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.#configDirName = nextConfigDirName; | ||
| this.configRoot = nextConfigRoot; | ||
| this.agentDir = nextAgentDir; | ||
| this.refreshCategoryDirs(this.#projectEnv); |
There was a problem hiding this comment.
Preserve explicit agent overrides when refreshing XDG paths
On Linux or macOS, if an explicit GJC_CODING_AGENT_DIR happens to equal <newHome>/.gjc/agent after the authoritative home changes and an existing $XDG_*_HOME/gjc directory is configured, this refresh infers isDefault from path equality and begins routing agent data through XDG. For example, getAgentDbPath() changes from the pinned override to $XDG_DATA_HOME/gjc/agent.db even though getAgentDir() remains unchanged. Pass the override state into refreshCategoryDirs() so an explicitly selected agent directory cannot silently switch storage lanes during a home refresh.
Useful? React with 👍 / 👎.
d3eb487 to
31b633b
Compare
Re-review verdicts: both YES — final P3 addressed, new exact headHead Independent re-review outcome
Confirmed fixed: the memoization P2 (failure no longer latched, no sentinel hole), the test-isolation P2 (no in-process Final P3 — the reviewer was right, and I verified it empiricallyMy isolation test asserted only Both probe lanes and the isolation check now run as one ordered test that captures the parent agent directory before the probes and asserts it unchanged after them. Regression-catching power at this headOverlaid on the regressed commit Verification at this head
(A transient local failure after the rebase was a stale locally built native addon — dev bumped to Explicitly not fixed hereThe Not merging — 🤖 gaebal-gajae |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31b633bbc5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function tempDir(): string { | ||
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "gjc-trusted-home-")); | ||
| tempDirs.push(dir); |
There was a problem hiding this comment.
Replace synchronous test directory operations
The newly added temp-directory helper uses fs.mkdtempSync, and its cleanup similarly uses fs.rmSync, despite the repository contract requiring node:fs/promises for directory operations. Make the helper and cleanup asynchronous so this test does not introduce synchronous filesystem operations.
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
Terminal exact-head CI: every substantive gate greenHead
The three earlier The only remaining reds are the contract gate refusing to merge on my own verdict: That is the correct terminal state, not a defect to clear. Ready for human review
What this needs now is a maintainer's authenticated exact-head 🤖 gaebal-gajae |
Second production defect found and fixed — new exact headHead P2
|
| suite | pristine HOME |
ambient HOME |
|---|---|---|
packages/utils/test/ |
378 pass / 0 fail | 378 pass / 0 fail |
skills.test.ts |
36 pass / 0 fail | — |
runtime-mcp/mcp-autoload-redteam.test.ts |
16 pass / 0 fail | — |
bun --cwd=packages/utils run check clean. Provenance re-verified after the change: planted HOME and hostile USERPROFILE both still resolve to the account home.
Standing review state
The independent architect re-review returned both verdicts YES with status WATCH; its only open item was the P3 isolation assertion, already fixed in 31b633bbc5. This new P2 arrived after that review, so the delta since it was last examined is: the XDG-lane fix, its regression case, and the async-fs conversion.
#4773 remains open and is still not claimed fixed here.
Re-requesting review at this head. Not merging — needs-human stands pending an authenticated exact-head approval from an identity distinct from mine.
🤖 gaebal-gajae
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5da93e8bc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const authoritativeHomeKey = process.platform === "win32" ? "USERPROFILE" : "HOME"; | ||
| const declaredHome = project.values[authoritativeHomeKey]; | ||
| const runtimeHome = os.homedir(); | ||
| // Only the platform-authoritative variable can select the home. In particular, |
There was a problem hiding this comment.
Read the authoritative home from the live environment
When an SDK host establishes or changes HOME/USERPROFILE after dirs.ts loads—the exact call-time scenario this change is intended to support—os.homedir() may retain the process-start value (Bun does so in the available runtime), even though process.env[authoritativeHomeKey] has changed. Consequently getTrustedHomeDir(), the config root, and default agent directory remain under the old home, so user-scope skills and MCP configuration still disappear. Read the platform-authoritative environment key on each call and use the account lookup only as the fallback, while retaining the dotenv provenance comparison.
Useful? React with 👍 / 👎.
Terminal exact-head CI at
|
| finding | source | disposition |
|---|---|---|
| import-time trusted-home snapshot breaks user-scope discovery | #4761 | fixed — call-time resolution |
| memoized failed account-home lookup | architect P2 | fixed — only successes cached |
setAgentDir restore latches #agentDirOverride |
architect P2 | fixed — subprocess probes |
| no default-agent-dir-on-home-change coverage | architect P3 | fixed — two-home ordering |
isolation asserted configRoot, not agentDir |
architect P3 | fixed — proved it passed despite the latch |
| operator agent dir switches to XDG on home refresh | Codex P2 | fixed — reproduced, then pinned |
| sync fs in test helper | Codex P1 | fixed — node:fs/promises |
| HOME-derived account-home fallback | architect P1 | deferred → #4773, not claimed fixed |
Two of those were real production defects in my own change, both found by review rather than by me, and both reproduced before being fixed. Every regression case was verified to fail on the code it guards — the suite fails 7/8 on d9fabc8f5a, and reverting only the XDG-lane fix fails that case and no other.
Ready for a human
- Independent architect: both verdicts YES, status WATCH, recommendation COMMENT.
- Provenance: 12 attack shapes, zero bypasses, identical to pre-fix.
- Local: utils 378/0 under both pristine and ambient
HOME; skills 36/0; MCP red-team 16/0.
What remains is not something I can or should clear myself: an authenticated exact-head APPROVED review from an identity distinct from mine. merge-self-approved stays unclaimed — it is the low-risk owner path, and this touches the trusted-home boundary. No merge, tag, or publish from me.
🤖 gaebal-gajae
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 2 actionable issues, led by Validate the runtime home before trusting it and Linux account lookup bypasses NSS-backed user databases. These findings require changes before approval.
Findings / Required Changes
- [P1] Validate the runtime home before trusting it.
Reference:packages/utils/src/dirs.ts:284
resolveTrustedHome accepts os.homedir() without requiring an absolute, non-root path; a relative runtime home can redirect user-scope state beneath the current directory. Validate it like accountHomeFromSystem and fall back or throw. - [P2] Linux account lookup bypasses NSS-backed user databases.
Reference:packages/utils/src/dirs.ts:229-245
The Linux path reads only /etc/passwd, so LDAP/SSSD or other NSS users may fail or fall back to os.userInfo().homedir, potentially honoring an untrusted HOME. Use an OS/NSS-backed account-home lookup instead of parsing /etc/passwd directly.
CI / Verification
- Reviewed the exact remote head:
b5da93e8bc10a96e19a5946b4cb4ef57a9e5d7e4. - CI summary: 17 passing, 3 failing, 8 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,Validate exact-head PR contract,PR contract bootstrap. - Non-successful checks without pass evidence:
Windows native build toolchain path,Telegram daemon generation guard,Windows Telegram daemon safety,Windows dev:doctor + session-path regression,Affected path validation / ${{ matrix.key }},Affected path validation / plan,gjc-state-gates / ${{ matrix.group }},Live deployed release state. - Passing evidence reviewed:
Virtual integration validation,Affected path validation,Affected path validation / evidence producer,Affected path validation / ts-build:ts:Y29kaW5nLWFnZW50:cGFja2FnZXMvY29kaW5nLWFnZW50,Affected path validation / ts-build:ts:c3RhdHM:cGFja2FnZXMvc3RhdHM,Affected path validation / install-methods,Affected path validation / test:packages/utils/test/trusted-home-resolution.test.ts,Affected path validation / test:packages/utils/test/agent-dir-trust.test.ts. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Call-time home resolution and derived path refresh are compatible, but unvalidated runtime homes leave a user-state trust-boundary risk. |
| A2. Architecture / Correctness / Failure | APPROVED | A2 correctness and concurrency risk appears addressed: path refreshes are synchronous, caches are invalidated, and explicit overrides remain pinned. |
| A3. Security / Privacy / Trust | APPROVED | The change preserves the trusted-home provenance boundary and keeps user-state paths from following project-controlled environment redirects. |
| A4. Verification / Tests / CI | APPROVED | A4 review found passing affected tests and no established regression, with Windows behavior and exact-head contract compliance remaining unverified. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Call-time directory behavior and documentation are aligned, but Linux account-home resolution may be incompatible with non-/etc/passwd NSS environments. |
Limitations
- PR contract validation is failing, so repository contract compliance cannot be treated as passing.
- PR contract checks are reported as failed, so exact-head contract validation is not established.
- PR contract validation failed, so exact-head contract compliance is not established.
- Windows path/runtime behavior is not established because Windows-specific CI checks were skipped.
- Exact-head PR contract compliance is not established because the PR contract checks failed.
- The PR contract validation checks failed, so repository-level exact-head integration acceptance cannot be confirmed; other reported affected-path checks passed.
…ss identities Closes #4773. The fail-open that issue reported — `accountHomeFromSystem()`'s final fallback `os.userInfo().homedir` is the environment home returned verbatim whenever it is set, so the ambiguous branch could accept attacker-influenced input as the trusted home on identities without a local passwd entry — is closed on dev by the independent-evidence rule from #4766 (`independentAccountHome`: the account home counts only when it does not merely echo the runtime home; otherwise the filesystem-root sentinel marks user state unavailable). #4766's repro is macOS-shaped; the Linux no-passwd-entry shape (NSS/LDAP/SSSD, distroless) had no discriminating coverage, and this PR supplies it without changing any production behavior (the `packages/utils/src` delta is empty). - New `trusted-home-failopen.test.ts` runs the resolver probe under an unprivileged user namespace whose mapped uid (100000) has no `/etc/passwd` entry — the exact #4773 shape, without root. Every candidate `unshare` argument form is probed and the first working one reused; the discriminating tests throw with an explicit reason when no form exists, so a restricted CI host cannot silently lose the coverage. Compat shapes are asserted unchanged: passwd-backed uid still resolves through `/etc/passwd`, absent platform home variable still resolves through the account database, unambiguous operator home still wins. - Mutation proof: removing the distinctness rule from dirs.ts (`independentAccountHome = accountHome`) makes exactly the two no-passwd-entry cases fail with the attacker path as `trustedHome`; with the rule, all pass. - `agent-dir-trust.test.ts`'s account-home expectation now reads the passwd database directly instead of a parent-side `os.userInfo().homedir`, which follows an isolated HOME and made the assertion fail under a pristine HOME (pre-existing on dev). - `docs/crash-reporting.md` now describes the landed contract accurately: independent evidence means not merely echoing the runtime home; without it, the root sentinel marks user state unavailable. The previous text claimed a filesystem root is never used as the refusal sentinel, which the implementation no longer honors. Relationship to #4772: none absorbed; that PR remains separate (its head carries its own version of the parent-side expectation fix). Lore-id: 4773-trusted-home-fail-open Constraint: no production-code change; the established dev contract (#4766 independent-evidence rule + root sentinel) is preserved verbatim Constraint: no overlap absorption from #4772 Rejected: shipping a competing implementation (eager throw at import) | dev's sentinel refusal already landed and is strictly more compatible at startup Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/utils/test/ (376 pass ambient + pristine HOME); coding-agent credential-boundary suites 61 pass under pristine HOME (auth-broker, credential-import, skill-hook-agent-dir, spawn-command, sdk-bus-token, exa-api-key, web-search, runtime-mcp redteam/precedence) Tested: mutation proof — distinctness rule removed → 2 no-passwd-entry cases fail with attacker home as trustedHome; rule present → 6/6 pass Not-tested: live NSS/LDAP/SSSD identity (simulated faithfully via uid without passwd entry in a user namespace); live macOS/Windows hosts (rule provenance established from Bun and libuv sources)
Two findings from exact-head review of #4772. The runtime home was trusted verbatim. Bun returns HOME as given, so a relative value anchored the config root, agent dir and plugins dir beneath whatever the working directory happened to be, and a bare filesystem root placed user state at /.gjc. It is now held to the same absolute, non-root standard as the account home, and an unusable value falls through to the account lookup instead of being adopted and failing later. The Linux account lookup parsed /etc/passwd directly, so LDAP- and SSSD-backed accounts -- which have no local passwd entry -- fell through to os.userInfo().homedir, which Bun derives from $HOME. The evidence used to reject an untrusted home was itself that untrusted home. getent passwd is the NSS front end and resolves local and directory-backed accounts alike; it runs with a fixed PATH and LC_ALL and no inherited environment. Lore-id: 9e4b2d70 Constraint: the account home must be independent evidence, never an echo of the runtime home -- dev's independence check is preserved Constraint: #4773 owns widening the no-independent-evidence fallback; the fail-closed root return is preserved here, not weakened Rejected: keep parsing /etc/passwd and add an NSS fallback | the file read silently wins for directory-backed accounts, which is the defect Confidence: high Scope-risk: wide Reversibility: easy Tested: reverting the runtime-home guard fails exactly the three new cases Tested: utils 384 under pristine and ambient HOME; skills 36; MCP redteam 16 Tested: 12-shape dotenv provenance matrix, zero bypasses Not-tested: a live LDAP/SSSD identity; NSS behavior is pinned against getent
b5da93e to
4a72d59
Compare
…ss identities Closes #4773. The fail-open that issue reported — `accountHomeFromSystem()`'s final fallback `os.userInfo().homedir` is the environment home returned verbatim whenever it is set, so the ambiguous branch could accept attacker-influenced input as the trusted home on identities without a local passwd entry — is closed on dev by the independent-evidence rule from #4766 (`independentAccountHome`: the account home counts only when it does not merely echo the runtime home; otherwise the filesystem-root sentinel marks user state unavailable). #4766's repro is macOS-shaped; the Linux no-passwd-entry shape (NSS/LDAP/SSSD, distroless) had no discriminating coverage, and this PR supplies it without changing any production behavior (the `packages/utils/src` delta is empty). - New `trusted-home-failopen.test.ts` runs the resolver probe under an unprivileged user namespace whose mapped uid is verified absent from `/etc/passwd` (skipping candidate uids the host already maps, then using the invoking user's subordinate range) — the exact #4773 shape, without root. Every candidate `unshare` argument form is probed and the first working one reused. The discriminating tests are Linux-only; on a host without the capability they skip with a loud warning naming the lost coverage instead of failing cross-platform CI. Compat shapes are asserted unchanged: passwd-backed uid still resolves through `/etc/passwd` (the raw passwd field, mirroring the resolver's validity rule), absent platform home variable still resolves through the account database, unambiguous operator home still wins. - Mutation proof: removing the distinctness rule from dirs.ts (`independentAccountHome = accountHome`) makes exactly the two no-passwd-entry cases fail with the attacker path as `trustedHome`; with the rule, all pass. - `agent-dir-trust.test.ts`'s account-home expectation now reads the passwd database directly instead of a parent-side `os.userInfo().homedir`, which follows an isolated HOME and made the assertion fail under a pristine HOME (pre-existing on dev). - `docs/crash-reporting.md` now describes the landed contract accurately: independent evidence means not merely echoing the runtime home; without it, the root sentinel marks user state unavailable. The previous text claimed a filesystem root is never used as the refusal sentinel, which the implementation no longer honors. Relationship to #4772: none absorbed; that PR remains separate (its head carries its own version of the parent-side expectation fix). Lore-id: 4773-trusted-home-fail-open Constraint: no production-code change; the established dev contract (#4766 independent-evidence rule + root sentinel) is preserved verbatim Constraint: no overlap absorption from #4772 Rejected: shipping a competing implementation (eager throw at import) | dev's sentinel refusal already landed and is strictly more compatible at startup Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/utils/test/ (376 pass ambient + pristine HOME); coding-agent credential-boundary suites 61 pass under pristine HOME (auth-broker, credential-import, skill-hook-agent-dir, spawn-command, sdk-bus-token, exa-api-key, web-search, runtime-mcp redteam/precedence) Tested: mutation proof — distinctness rule removed → 2 no-passwd-entry cases fail with attacker home as trustedHome; rule present → 6/6 pass Not-tested: live NSS/LDAP/SSSD identity (simulated faithfully via uid without passwd entry in a user namespace); live macOS/Windows hosts (rule provenance established from Bun and libuv sources)
…ss identities Closes #4773. The fail-open that issue reported — `accountHomeFromSystem()`'s final fallback `os.userInfo().homedir` is the environment home returned verbatim whenever it is set, so the ambiguous branch could accept attacker-influenced input as the trusted home on identities without a local passwd entry — is closed on dev by the independent-evidence rule from #4766 (`independentAccountHome`: the account home counts only when it does not merely echo the runtime home; otherwise the filesystem-root sentinel marks user state unavailable). #4766's repro is macOS-shaped; the Linux no-passwd-entry shape (NSS/LDAP/SSSD, distroless) had no discriminating coverage, and this PR supplies it without changing any production behavior (the `packages/utils/src` delta is empty). - New `trusted-home-failopen.test.ts` runs the resolver probe under an unprivileged user namespace whose mapped uid is verified absent from `/etc/passwd` (skipping candidate uids the host already maps, then using the invoking user's subordinate range) — the exact #4773 shape, without root. Every candidate `unshare` argument form is probed and the first working one reused. The discriminating tests are Linux-only; on a host without the capability they skip with a loud warning naming the lost coverage instead of failing cross-platform CI. Compat shapes are asserted unchanged: passwd-backed uid still resolves through `/etc/passwd` (the raw passwd field, mirroring the resolver's validity rule), absent platform home variable still resolves through the account database, unambiguous operator home still wins. - Mutation proof: removing the distinctness rule from dirs.ts (`independentAccountHome = accountHome`) makes exactly the two no-passwd-entry cases fail with the attacker path as `trustedHome`; with the rule, all pass. - `agent-dir-trust.test.ts`'s account-home expectation now reads the passwd database directly instead of a parent-side `os.userInfo().homedir`, which follows an isolated HOME and made the assertion fail under a pristine HOME (pre-existing on dev). - `docs/crash-reporting.md` now describes the landed contract accurately: independent evidence means not merely echoing the runtime home; without it, the root sentinel marks user state unavailable. The previous text claimed a filesystem root is never used as the refusal sentinel, which the implementation no longer honors. Relationship to #4772: none absorbed; that PR remains separate (its head carries its own version of the parent-side expectation fix). Lore-id: 4773-trusted-home-fail-open Constraint: no production-code change; the established dev contract (#4766 independent-evidence rule + root sentinel) is preserved verbatim Constraint: no overlap absorption from #4772 Rejected: shipping a competing implementation (eager throw at import) | dev's sentinel refusal already landed and is strictly more compatible at startup Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/utils/test/ (376 pass ambient + pristine HOME); coding-agent credential-boundary suites 61 pass under pristine HOME (auth-broker, credential-import, skill-hook-agent-dir, spawn-command, sdk-bus-token, exa-api-key, web-search, runtime-mcp redteam/precedence) Tested: mutation proof — distinctness rule removed → 2 no-passwd-entry cases fail with attacker home as trustedHome; rule present → 6/6 pass Not-tested: live NSS/LDAP/SSSD identity (simulated faithfully via uid without passwd entry in a user namespace); live macOS/Windows hosts (rule provenance established from Bun and libuv sources)
…ss identities Closes #4773. The fail-open that issue reported — `accountHomeFromSystem()`'s final fallback `os.userInfo().homedir` is the environment home returned verbatim whenever it is set, so the ambiguous branch could accept attacker-influenced input as the trusted home on identities without a local passwd entry — is closed on dev by the independent-evidence rule from #4766 (`independentAccountHome`: the account home counts only when it does not merely echo the runtime home; otherwise the filesystem-root sentinel marks user state unavailable). #4766's repro is macOS-shaped; the Linux no-passwd-entry shape (NSS/LDAP/SSSD, distroless) had no discriminating coverage, and this PR supplies it without changing any production behavior (the `packages/utils/src` delta is empty). - New `trusted-home-failopen.test.ts` runs the resolver probe under an unprivileged user namespace whose mapped uid is verified absent from `/etc/passwd` (skipping candidate uids the host already maps, then using the invoking user's subordinate range) — the exact #4773 shape, without root. Every candidate `unshare` argument form is probed and the first working one reused. The discriminating tests are Linux-only; on a host without the capability they skip with a loud warning naming the lost coverage instead of failing cross-platform CI. Compat shapes are asserted unchanged: passwd-backed uid still resolves through `/etc/passwd` (the raw passwd field, mirroring the resolver's validity rule), absent platform home variable still resolves through the account database, unambiguous operator home still wins. - Mutation proof: removing the distinctness rule from dirs.ts (`independentAccountHome = accountHome`) makes exactly the two no-passwd-entry cases fail with the attacker path as `trustedHome`; with the rule, all pass. - `agent-dir-trust.test.ts`'s account-home expectation now reads the passwd database directly instead of a parent-side `os.userInfo().homedir`, which follows an isolated HOME and made the assertion fail under a pristine HOME (pre-existing on dev). - `docs/crash-reporting.md` now describes the landed contract accurately: independent evidence means not merely echoing the runtime home; without it, the root sentinel marks user state unavailable. The previous text claimed a filesystem root is never used as the refusal sentinel, which the implementation no longer honors. Relationship to #4772: none absorbed; that PR remains separate (its head carries its own version of the parent-side expectation fix). Lore-id: 4773-trusted-home-fail-open Constraint: no production-code change; the established dev contract (#4766 independent-evidence rule + root sentinel) is preserved verbatim Constraint: no overlap absorption from #4772 Rejected: shipping a competing implementation (eager throw at import) | dev's sentinel refusal already landed and is strictly more compatible at startup Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/utils/test/ (376 pass ambient + pristine HOME); coding-agent credential-boundary suites 61 pass under pristine HOME (auth-broker, credential-import, skill-hook-agent-dir, spawn-command, sdk-bus-token, exa-api-key, web-search, runtime-mcp redteam/precedence) Tested: mutation proof — distinctness rule removed → 2 no-passwd-entry cases fail with attacker home as trustedHome; rule present → 6/6 pass Not-tested: live NSS/LDAP/SSSD identity (simulated faithfully via uid without passwd entry in a user namespace); live macOS/Windows hosts (rule provenance established from Bun and libuv sources)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b048f5cf9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = Bun.spawnSync({ | ||
| cmd: ["getent", "passwd", String(uid)], | ||
| env: { PATH: "/usr/bin:/bin:/usr/sbin:/sbin", LC_ALL: "C" }, | ||
| stdout: "pipe", | ||
| stderr: "ignore", | ||
| }); |
There was a problem hiding this comment.
Bound the synchronous NSS lookup
On Linux systems whose passwd NSS provider is LDAP/SSSD and slow or unreachable, this synchronous lookup runs during module-level DirResolver construction with no timeout, so every gjc invocation can stall before CLI argument handling begins. I checked getent --help; it provides service-selection options but no command-level timeout, so the subprocess itself needs a bound and a fallback rather than waiting for the provider's full network timeout.
Useful? React with 👍 / 👎.
| if (process.platform === "linux") { | ||
| const uid = String(os.userInfo().uid); | ||
| const line = (await Bun.file("/etc/passwd").text()) | ||
| .split("\n") | ||
| .find(candidate => candidate.split(":")[2] === uid); | ||
| const home = line?.split(":")[5]; | ||
| if (home && path.isAbsolute(home) && home !== path.parse(home).root) return home; | ||
| } | ||
| return os.userInfo().homedir; |
There was a problem hiding this comment.
Resolve the test expectation through NSS
On the LDAP/SSSD accounts this change is intended to support, there is no matching /etc/passwd record, so this helper falls through to os.userInfo().homedir, which its own comment notes Bun derives from the parent process's HOME. If that parent has an isolated HOME, the child correctly returns its NSS account home while the tests at the end of this file compare it with the unrelated parent path and fail; obtain the expectation through NSS here as well.
Useful? React with 👍 / 👎.
| // The resolver must still yield an absolute home despite that hazard. | ||
| const resolved = await resolveWith({ HOME: undefined }); | ||
| expect(path.isAbsolute(resolved)).toBe(true); |
There was a problem hiding this comment.
Exercise the resolver's missing-getent fallback
This test proves only that an unrelated probe with PATH=/nonexistent throws; the subsequent resolveWith() child runs the real resolver with its normal environment, and production hardcodes /usr/bin:/bin:/usr/sbin:/sbin, so nssAccountHome() still finds getent. Removing or breaking the resolver's catch/fallback would therefore leave this test passing on ordinary Linux hosts; make the resolver's own spawn fail via an injectable/mockable boundary and assert its resulting home.
AGENTS.md reference: AGENTS.md:L158-L160
Useful? React with 👍 / 👎.
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 4 actionable issues, led by Environment-derived account home is cached across runtime changes and Explicit agent override incorrectly enables XDG routing. These findings require changes before approval.
Findings / Required Changes
- [P1] Environment-derived account home is cached across runtime changes.
Reference:packages/utils/src/dirs.ts:264-297
On macOS/Windows, accountHomeFromSystem() caches os.userInfo().homedir; after HOME/USERPROFILE changes, the stale value can differ from the new runtime home and be treated as independent trusted evidence. Avoid caching environment-derived fallbacks, or track provenance and re-resolve them per call. - [P1] Explicit agent override incorrectly enables XDG routing.
Reference:packages/utils/src/dirs.ts:396
Constructor derives isDefault by comparing paths, so an explicit override equal to the home-derived default is treated as default and its initial data/state/cache paths may use XDG locations. Use !this.#agentDirOverride consistently when initializing category directories. - [P2] Explicit plugin home can bypass XDG routing.
Reference:packages/utils/src/dirs.ts:638
getPluginsDir compares the supplied home with the cached dirs.trustedHome without first refreshing call-time home state. After os.homedir() changes, passing the current authoritative home is treated as an explicit alternate and returns <home>/.gjc/plugins instead of the XDG-aware path promised by the documentation. Refresh the resolver before comparing homes, then preserve XDG semantics for the current authoritative home. - [P2] NSS fallback test does not exercise fallback.
Reference:packages/utils/test/account-home-nss.test.ts:78-88
The test only proves a missing executable throws, then resolves with the normal PATH, so it never disables getent for the resolver. Inject a controlled probe or PATH override and assert the actual fallback behavior.
CI / Verification
- Reviewed the exact remote head:
9b048f5cf96c94dc0c40b4a7ae7639ea9a5b9f89. - CI summary: 17 passing, 3 failing, 9 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap,Validate exact-head PR contract. - Non-successful checks without pass evidence:
Affected path validation / evidence producer,Windows Telegram daemon safety,Windows native build toolchain path,Telegram daemon generation guard,Affected path validation / ${{ matrix.key }},gjc-state-gates / ${{ matrix.group }},Affected path validation,Virtual integration validation. - Passing evidence reviewed:
Affected path validation / test:packages/utils/test/trusted-home-resolution.test.ts,Affected path validation / ts-build:ts:c3RhdHM:cGFja2FnZXMvc3RhdHM,Affected path validation / ts-build:ts:Y29kaW5nLWFnZW50:cGFja2FnZXMvY29kaW5nLWFnZW50,Affected path validation / install-methods,Affected path validation / test:packages/utils/test/agent-dir-trust.test.ts,Affected path validation / test:packages/utils/test/account-home-nss.test.ts,Affected path validation / check:@gajae-code/utils,gjc-state-gates. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | API compatibility risk is established: explicit agent-directory overrides are not guaranteed to retain their storage lane during initial resolution. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Correctness risk established: cached environment-derived account homes can violate call-time resolution and trust during runtime home changes. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy protections for trusted-home provenance and user-state path resolution appear sound; no concrete vulnerability was established. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 verification is broadly green for affected tests, but NSS fallback coverage is incomplete and CI contract validation remains failing. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Call-time home resolution is broadly covered, but explicit plugin-home calls can still diverge from XDG platform routing after a home change. |
Limitations
- The CI evidence producer remains IN_PROGRESS, so complete CI validation cannot be claimed.
- PR contract checks are reported as FAILURE, but ci_summary provides no failure diagnostics, so the cause and its integration impact cannot be assessed.
- PR contract checks failed, so the repository contract-gate status is not established.
- PR contract checks are failing, so successful exact-head contract validation cannot be claimed.
- The evidence-producer CI job is still in progress, so CI results are not fully final.
…ss identities Closes #4773. The fail-open that issue reported — `accountHomeFromSystem()`'s final fallback `os.userInfo().homedir` is the environment home returned verbatim whenever it is set, so the ambiguous branch could accept attacker-influenced input as the trusted home on identities without a local passwd entry — is closed on dev by the independent-evidence rule from #4766 (`independentAccountHome`: the account home counts only when it does not merely echo the runtime home; otherwise the filesystem-root sentinel marks user state unavailable). #4766's repro is macOS-shaped; the Linux no-passwd-entry shape (NSS/LDAP/SSSD, distroless) had no discriminating coverage, and this PR supplies it without changing any production behavior (the `packages/utils/src` delta is empty). - New `trusted-home-failopen.test.ts` runs the resolver probe under an unprivileged user namespace whose mapped uid is verified absent from `/etc/passwd` (skipping candidate uids the host already maps, then using the invoking user's subordinate range) — the exact #4773 shape, without root. Every candidate `unshare` argument form is probed and the first working one reused. The discriminating tests are Linux-only; on a host without the capability they skip with a loud warning naming the lost coverage instead of failing cross-platform CI. Compat shapes are asserted unchanged: passwd-backed uid still resolves through `/etc/passwd` (the raw passwd field, mirroring the resolver's validity rule), absent platform home variable still resolves through the account database, unambiguous operator home still wins. - Mutation proof: removing the distinctness rule from dirs.ts (`independentAccountHome = accountHome`) makes exactly the two no-passwd-entry cases fail with the attacker path as `trustedHome`; with the rule, all pass. - `agent-dir-trust.test.ts`'s account-home expectation now reads the passwd database directly instead of a parent-side `os.userInfo().homedir`, which follows an isolated HOME and made the assertion fail under a pristine HOME (pre-existing on dev). - `docs/crash-reporting.md` now describes the landed contract accurately: independent evidence means not merely echoing the runtime home; without it, the root sentinel marks user state unavailable. The previous text claimed a filesystem root is never used as the refusal sentinel, which the implementation no longer honors. Relationship to #4772: none absorbed; that PR remains separate (its head carries its own version of the parent-side expectation fix). Lore-id: 4773-trusted-home-fail-open Constraint: no production-code change; the established dev contract (#4766 independent-evidence rule + root sentinel) is preserved verbatim Constraint: no overlap absorption from #4772 Rejected: shipping a competing implementation (eager throw at import) | dev's sentinel refusal already landed and is strictly more compatible at startup Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/utils/test/ (376 pass ambient + pristine HOME); coding-agent credential-boundary suites 61 pass under pristine HOME (auth-broker, credential-import, skill-hook-agent-dir, spawn-command, sdk-bus-token, exa-api-key, web-search, runtime-mcp redteam/precedence) Tested: mutation proof — distinctness rule removed → 2 no-passwd-entry cases fail with attacker home as trustedHome; rule present → 6/6 pass Not-tested: live NSS/LDAP/SSSD identity (simulated faithfully via uid without passwd entry in a user namespace); live macOS/Windows hosts (rule provenance established from Bun and libuv sources)
Two findings from exact-head review of #4772. The runtime home was trusted verbatim. Bun returns HOME as given, so a relative value anchored the config root, agent dir and plugins dir beneath whatever the working directory happened to be, and a bare filesystem root placed user state at /.gjc. It is now held to the same absolute, non-root standard as the account home, and an unusable value falls through to the account lookup instead of being adopted and failing later. The Linux account lookup parsed /etc/passwd directly, so LDAP- and SSSD-backed accounts -- which have no local passwd entry -- fell through to os.userInfo().homedir, which Bun derives from $HOME. The evidence used to reject an untrusted home was itself that untrusted home. getent passwd is the NSS front end and resolves local and directory-backed accounts alike; it runs with a fixed PATH and LC_ALL and no inherited environment. Lore-id: 9e4b2d70 Constraint: the account home must be independent evidence, never an echo of the runtime home -- dev's independence check is preserved Constraint: #4773 owns widening the no-independent-evidence fallback; the fail-closed root return is preserved here, not weakened Rejected: keep parsing /etc/passwd and add an NSS fallback | the file read silently wins for directory-backed accounts, which is the defect Confidence: high Scope-risk: wide Reversibility: easy Tested: reverting the runtime-home guard fails exactly the three new cases Tested: utils 384 under pristine and ambient HOME; skills 36; MCP redteam 16 Tested: 12-shape dotenv provenance matrix, zero bypasses Not-tested: a live LDAP/SSSD identity; NSS behavior is pinned against getent
All three came out of exact-head review of #4772. usableHome compared the raw string against path.parse(home).root, so every non-canonical spelling of a root slipped through: "/.", "//", "/foo/.." and "C:\x\.." are all roots, and path.join then produced /.gjc from each. The root test now normalizes first. The original spelling is still returned -- canonicalizing this side alone would make HOME=/tmp/x/../y compare unequal to its own dotenv declaration and be honored as operator-supplied. The account-home memo latched the os.userInfo() fallback, which Bun derives from $HOME. A planted home live at first resolution stayed cached, and once the runtime home moved the cached attacker value no longer equalled it -- passing the echo check and being promoted to independent evidence. Only the environment-independent NSS answer is cached now, and provenance travels with the value so an env-derived home can never be treated as evidence. The account lookup also ran unconditionally, spawning getent on a path that executes on every directory access. It is now consulted lazily, only when the home is ambiguous or no usable runtime home exists. Lore-id: 7c1e9f42 Constraint: setAgentDir(<default path>) selects the default profile, XDG included -- dirs-python-gateway.test.ts pins it, so the initial decision stays path equality and is made sticky instead Rejected: decide XDG from override state | contradicts that pinned contract Rejected: normalize the returned home | breaks dotenv ambiguity comparison Confidence: high Scope-risk: wide Reversibility: easy Tested: each fix reverted independently fails exactly its own regressions Tested: utils 388 pristine+ambient, skills 36, MCP 16, checks clean Tested: 12-shape provenance matrix, zero bypasses
9b048f5 to
4b8ad44
Compare
Signed exact-head UID-cache fix status
The worktree was reconciled to remote Local evidence: UID transition/security suites Fresh exact-head review is requested from |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60694e75f0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.#trustedHome = nextHome; | ||
| this.#configDirName = nextConfigDirName; | ||
| this.configRoot = nextConfigRoot; | ||
| this.agentDir = nextAgentDir; |
There was a problem hiding this comment.
Refresh module-level paths when the home changes
When an SDK host changes the authoritative home after modules have loaded—the scenario this refresh is meant to support—paths previously derived from these getters remain pinned to the old profile. For example, packages/ai/src/auth-broker/remote-store.ts:78 captures getConfigRootDir() in DEFAULT_PRESENTATION_SIDECAR, and stores constructed afterward still select that stale path at lines 194-197 even though these assignments have moved the resolver to the new home. This makes broker presentation state continue to be read from and written to the previous user's config root; derive such defaults when constructing the consumer or explicitly refresh them as part of the transition.
Useful? React with 👍 / 👎.
|
Final exact-head review request: |
Terminal verdict: OWNER_CONFIRMATION_REQUIREDHead I shipped a real regression and a third critic pass caught itMy earlier fix for the reviewer's XDG finding was wrong, and I have reverted it.
Confirmed against base Re-applying the reverted gating fails the new parent/child regression; the pinned The earlier snowykr finding is therefore not fixed as originally requested, and I am not claiming it is. It is a genuine trade-off: honoring it as stated breaks parent/child consistency. If the maintainer wants inherited overrides off XDG, that needs a way to distinguish propagation from operator intent — worth its own issue, not a silent change here. Also integratedA concurrent commit on this branch ( Exact-head CI15 passing — affected-path aggregate, all three regression suites as dedicated jobs, Final stateutils 397 pass / 0 fail under pristine and ambient Fourteen defects were found across review rounds, eight of them in my own change — including three found after I had declared the work finished. Each was reproduced before being fixed and reverted afterwards to confirm its regression bites. Blocking action, human-only: one authenticated — |
Platform- and NSS-gated cases returned silently, so a vacuous pass looked identical to a real assertion in CI output; they now say why they skipped. Two storage-lane cases had identical setup and overlapping assertions, and one still carried the comment for the override-state gating that was reverted; consolidated into the single lane-stability case. Lore-id: 91c4ad6f Confidence: high Scope-risk: narrow Reversibility: easy Tested: utils 396/0 pristine and ambient HOME
Terminal verdict: OWNER_CONFIRMATION_REQUIREDHead Fourth critic pass — in-scope items fixed, one filed separatelyFixed here:
Filed as #4786, not fixed here: module-level home-derived constants outlive the now call-time home — e.g. It is genuinely out of scope: introduced by Exact-head CI19 passing — affected-path aggregate, all three regression suites as dedicated jobs, Final stateutils 396 pass / 0 fail under pristine and ambient Across four independent critic passes and two reviewer rounds, sixteen defects were found — nine in my own change, four of them after I had first declared the work finished. Each was reproduced before being fixed and reverted afterwards to confirm its regression bites. Two findings I declined to "fix" and documented instead, because implementing them as stated would break a pinned contract (#4772 comment above) or belongs to another package (#4786). Blocking action, human-only: one authenticated — |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 1 actionable issue, led by Platform-specific NSS tests pass vacuously off Linux. These findings require changes before approval.
Findings / Required Changes
- [P2] Platform-specific NSS tests pass vacuously off Linux.
Reference:packages/utils/test/account-home-nss.test.ts:158-164
The helper only logs a warning and returns from each test, so non-Linux CI reports successful tests without exercising equivalent coverage. Use the test framework's explicit skip mechanism and surface the skipped capability.
CI / Verification
- Reviewed the exact remote head:
8ed2bc1df242cb0013afe61c11751b77ccd9b066. - CI summary: 19 passing, 2 failing, 7 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | API and platform compatibility risks were not established; derived user-scope paths now follow the validated home at call time. |
| A2. Architecture / Correctness / Failure | APPROVED | Call-time home refresh, identity-scoped NSS caching, and path-cache rebuilding establish no demonstrated correctness or concurrency risk. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy controls appear strengthened, with no established exploitable trust-boundary or sensitive-data exposure. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 coverage is substantial on Linux, but non-Linux NSS behavior is not verified and the contract gate remains unresolved. |
| A5. Context / Compatibility / Platform | APPROVED | No integration, documentation, or platform regression was established; CI contract validation remains limited by failed contract/bootstrap jobs. |
Limitations
- PR contract and bootstrap CI jobs failed, so exact contract validation is not independently established.
- PR contract checks failed, so contract-validation status is unresolved.
Linux NSS account tests must not look like passing tests on platforms where getent is unavailable. Use Bun's explicit skip reporter for the platform capability while retaining loud capability warnings for missing account entries on Linux. Lore-id: 1d6b8e42 Constraint: non-Linux NSS tests must be reported as skipped, never vacuous passes Constraint: Linux NSS execution and unavailable-capability warnings remain unchanged Tested: account-home-nss, trusted-home, agent-dir-trust, fail-open, gateway, Windows provenance suites Confidence: high Scope-risk: narrow Reversibility: easy Directive: do not weaken trusted-home provenance
|
Final exact-head status: |
Terminal verdict: OWNER_CONFIRMATION_REQUIREDHead Fifth critic pass — both items closedVacuous skips. My previous "fix" only printed a warning and still returned early, so platform-gated cases counted as passing assertions on non-Linux. Now A concurrent agent landed the identical Stale PR metadata. The checklist named base Exact-head CI15 passing — affected-path aggregate, all three regression suites as dedicated jobs, Final stateutils 396 pass / 5 skip / 0 fail under pristine and ambient Eighteen defects surfaced across two reviewer rounds and five independent critic passes — eleven in my own change, six of them found after I first called the work finished. Each was reproduced before being fixed and re-broken afterwards to confirm its regression bites. Three findings I declined to implement as stated and documented instead: routing inherited Blocking action, human-only: one authenticated — |
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 1 actionable issue, led by Explicit plugin home no longer short-circuits trust resolution. These findings require changes before approval.
Findings / Required Changes
- [P2] Explicit plugin home no longer short-circuits trust resolution.
Reference:packages/utils/src/dirs.ts:718
getPluginsDir(home) evaluates dirs.trustedHome before comparing the supplied home, so it throws when authoritative home resolution is unavailable instead of returning the documented explicit <home> path. Compare safely without forcing trusted-home resolution, then preserve the short-circuit contract.
CI / Verification
- Reviewed the exact remote head:
bf23f94498f7525ae3008f8fac6b76bc92618f78. - CI summary: 15 passing, 2 failing, 11 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The explicit plugin-home API can now fail in environments where trusted-home resolution is unavailable; other reviewed directory boundary changes showed no established compatibility defect. |
| A2. Architecture / Correctness / Failure | APPROVED | A1-A5 review found no established correctness, concurrency, security, verification, or compatibility defect; affected-path validation passed. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy protections for trusted home resolution, NSS identity scoping, environment provenance, and user-state path isolation appear sound. |
| A4. Verification / Tests / CI | APPROVED | A4 verification is adequate on executed Linux paths, but cross-platform compatibility remains unverified due to skipped Windows CI. |
| A5. Context / Compatibility / Platform | APPROVED | Integration and documentation appear consistent; platform behavior is covered only on available lanes, with Windows and Darwin validation unverified. |
Limitations
- PR contract validation and bootstrap checks failed, preventing a claim that the exact-head PR contract was satisfied.
- PR contract validation failed, so exact-head contract compliance could not be established; affected security-related validation passed.
- Windows and Darwin platform behavior was not established because their CI jobs were skipped after PR contract bootstrap and exact-head contract failures.
Note: a late architect review landed against a stale head — both P1s already adjudicatedA long-running review just completed against head It raised two P1s. Both are already resolved or deliberately decided, with evidence: P1 — "the ambiguous-home distinctness rule is broadened." Correct observation, intentional change. The old rule tested independence by string inequality ( Re-verified at the current head just now:
The fail-closed property the reviewer wants protected is intact; what changed is that corroboration is no longer mistaken for self-justification. P1 — "an environment-declared default-path agent override is still routed through XDG at startup." This is the snowykr finding. I implemented it, then reverted it, because The review's test-quality points (non-discriminating failed- No head change, so the digest and verdict line are unaffected: head — |
Findings from an exact-head architect review. refreshCategoryDirs kept a defaulted isDefault argument that still carried the rejected override-state policy. It was unreachable but wrong, and it invited exactly the path-shape recomputation the sticky decision exists to prevent; the argument is now required. Added the converse regression: a dir that WAS default at construction stays on XDG after a refresh makes its path non-default. Both directions now fail if stickiness is removed. The cache-provenance probe mapped every exception to REFUSED, so an import failure or bad patch satisfied the assertion and hid the real error. It now rethrows anything that is not the known refusal. getTrustedConfigRootDir was still documented as stable although it is call-time, and the changelog still described independence as a value comparison after the source-provenance rule replaced it. Lore-id: 5d0f21ae Confidence: high Scope-risk: narrow Reversibility: easy Tested: reverting stickiness fails both lane cases (12/2) Tested: utils 397 pass / 5 skip / 0 fail, skills 36/0, MCP 16/0
An explicit plugin home is caller-owned, but the live trusted-home comparison could throw before returning it when authoritative home resolution became unavailable. Keep equal-home XDG routing while returning the explicit config path on fail-closed resolution. Lore-id: 5e2a4b67 Constraint: explicit plugin homes must not depend on trusted-home availability Constraint: equal-home calls retain default XDG-aware behavior Tested: explicit-home unavailable regression, trusted-home and NSS suites, utility check Confidence: high Scope-risk: narrow Reversibility: easy Directive: preserve fail-closed default trusted-home resolution
|
Final exact-head status: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee43d7db24
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await Bun.write(path.join(project, ".env"), `HOME=${aliased}\n`); | ||
| const resolved = await resolveWith({ HOME: aliased }, project); |
There was a problem hiding this comment.
Exercise the authoritative home key on Windows
On Windows, this test writes and overrides HOME, but the resolver selects only USERPROFILE; the inherited USERPROFILE therefore remains authoritative and the assertions pass even if traversal-spelled project declarations are accepted. Use the platform-authoritative key and clear its alternate so the Windows CI lane actually exercises this provenance boundary rather than recording tautological coverage.
AGENTS.md reference: AGENTS.md:L158-L160
Useful? React with 👍 / 👎.
Terminal verdict: OWNER_CONFIRMATION_REQUIREDHead A late architect review (head
|
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The five-axis review completed against the exact head without actionable P0-P2 findings. The reviewed API, correctness, security, verification, and compatibility boundaries are approved.
Findings / Required Changes
None.
CI / Verification
- Reviewed the exact remote head:
ee43d7db2444ad6ad481d0044b1013b493e18ce0. - CI summary: 19 passing, 3 failing, 7 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap,Validate exact-head PR contract. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | The directory API preserves call-time home resolution, trusted provenance checks, explicit plugin-home behavior, and XDG lane compatibility; no concrete boundary defect was established. |
| A2. Architecture / Correctness / Failure | APPROVED | The resolver’s call-time home refresh, identity-scoped NSS cache, and sticky storage-lane decisions are internally consistent; no demonstrated concurrency risk remains. |
| A3. Security / Privacy / Trust | APPROVED | Trusted-home provenance, NSS resolution, identity-scoped caching, and fail-closed behavior show no established security or privacy regression. |
| A4. Verification / Tests / CI | APPROVED | A4/A5 review found passing affected-path tests and no established observable regression; PR contract failure details remain unresolved. |
| A5. Context / Compatibility / Platform | APPROVED | Integration paths and documented trusted-home behavior appear consistent across Linux, macOS, and Windows; no platform regression was established. |
Limitations
- PR contract validation failed, so contract compliance cannot be established from the available CI summary.
- PR contract validation failed, so repository-level contract compliance is not established despite affected security tests passing.
|
Current head is approved by snowykr. A body-only exact-head refresh was issued to obtain fresh PR contract/bootstrap and current Dev CI evidence; no source/head mutation occurred. Merge remains gated on fresh current checks. |
…ruction The trusted config root is call-time state since #4761/#4772, so a module-level constant built from getConfigRootDir() at import kept pointing at the home in effect when the module first loaded. A process whose home was established or changed after load then read and wrote one logical profile through two different roots -- the same split that made user-scope skills and MCP disappear, one layer out, with credential impact because <configRoot>/.env is one of the files $credentialEnv treats as trusted. Derive the default when the store is constructed instead; explicit presentationPath options are unchanged. Lore-id: 4786a1 Constraint: getConfigRootDir stays call-time; no consumer may pin an import-time root Tested: bun test packages/ai/test/auth-broker-remote-store-home.test.ts (out-of-process probe fails on dev head, passes on this change) Tested: bun test auth-gateway + auth-broker suites in packages/ai (83 + 39 pass) Confidence: high Scope-risk: narrow Reversibility: revert-commit
…ruction The trusted config root is call-time state since #4761/#4772, so a module-level constant built from getConfigRootDir() at import kept pointing at the home in effect when the module first loaded. A process whose home was established or changed after load then read and wrote one logical profile through two different roots -- the same split that made user-scope skills and MCP disappear, one layer out, with credential impact because <configRoot>/.env is one of the files $credentialEnv treats as trusted. Derive the default when the store is constructed instead; explicit presentationPath options are unchanged. Lore-id: 4786a1 Constraint: getConfigRootDir stays call-time; no consumer may pin an import-time root Tested: bun test packages/ai/test/auth-broker-remote-store-home.test.ts (out-of-process probe fails on dev head, passes on this change) Tested: bun test auth-gateway + auth-broker suites in packages/ai (83 + 39 pass) Confidence: high Scope-risk: narrow Reversibility: revert-commit
…ruction The trusted config root is call-time state since #4761/#4772, so a module-level constant built from getConfigRootDir() at import kept pointing at the home in effect when the module first loaded. A process whose home was established or changed after load then read and wrote one logical profile through two different roots -- the same split that made user-scope skills and MCP disappear, one layer out, with credential impact because <configRoot>/.env is one of the files $credentialEnv treats as trusted. Derive the default when the store is constructed instead; explicit presentationPath options are unchanged. Lore-id: 4786a1 Constraint: getConfigRootDir stays call-time; no consumer may pin an import-time root Tested: bun test packages/ai/test/auth-broker-remote-store-home.test.ts (out-of-process probe fails on dev head, passes on this change) Tested: bun test auth-gateway + auth-broker suites in packages/ai (83 + 39 pass) Confidence: high Scope-risk: narrow Reversibility: revert-commit
…ruction The trusted config root is call-time state since #4761/#4772, so a module-level constant built from getConfigRootDir() at import kept pointing at the home in effect when the module first loaded. A process whose home was established or changed after load then read and wrote one logical profile through two different roots -- the same split that made user-scope skills and MCP disappear, one layer out, with credential impact because <configRoot>/.env is one of the files $credentialEnv treats as trusted. Derive the default when the store is constructed instead; explicit presentationPath options are unchanged. Lore-id: 4786a1 Constraint: getConfigRootDir stays call-time; no consumer may pin an import-time root Tested: bun test packages/ai/test/auth-broker-remote-store-home.test.ts (out-of-process probe fails on dev head, passes on this change) Tested: bun test auth-gateway + auth-broker suites in packages/ai (83 + 39 pass) Confidence: high Scope-risk: narrow Reversibility: revert-commit
What
Fixes #4761: user-scope skill and MCP discovery silently stopped working because the authoritative home was snapshotted at module load instead of resolved when asked.
d9fabc8f5amoved the home anchor fromos.homedir()to a value captured whilepackages/utils/src/dirs.tswas still initializing. Everything user-scope derives from that anchor — config root, default agent dir, plugins dir — so any home established or changed after module load resolved to the wrong place and every user-scope location disappeared:~/.gjc/agent/skills~/.gjc/agent/mcp.jsonWhy
Two properties are involved, and they are independent:
.envis overlaid intoprocess.envbefore any module runs, so a repository could otherwise plantHOME/USERPROFILEand redirect trusted state.Freezing the result achieved (1) by sacrificing (2). The home is now re-derived per access, with every cached path rebuilt when it changes, while the dotenv-ambiguity rule is applied unchanged at each resolution. The comment that claimed the value was "captured at module load to stay stable across test mocks" no longer described what the value was, and that contract mismatch was the defect.
GJC_CODING_AGENT_DIRstays pinned across a home change — it is an explicit operator selection, not a home-derived path.Relationship to #4766 / #4770
No overlap in the defect. Those PRs fold Windows dotenv key casing (
canonicalEnvKey); neither touches the import-time snapshot. Verified by running the repro on #4766's head (67d6ed0b00): skills 33/3 and MCP 13/3 still fail there, so #4766 does not cover #4761. WindowsUSERPROFILEfolding stays in its authoritative lane and is untouched here.Exact-head review findings (snowykr, CHANGES_REQUESTED on
b5da93e8bc) — both fixedP1
dirs.ts:284— validate the runtime home.resolveTrustedHome()trusted whateveros.homedir()returned. Bun returnsHOMEverbatim, so a relative value anchored the config root, agent dir and plugins dir beneath the working directory, and a bare filesystem root placed user state at/.gjc. The runtime home is now held to the same absolute, non-root standard as the account home via a sharedusableHome()guard, and an unusable value falls through to the account lookup rather than being adopted and failing later.P2
dirs.ts:229-245— NSS-backed account lookup. The Linux path parsed/etc/passwddirectly, so LDAP/SSSD accounts with no local passwd entry fell through toos.userInfo().homedir— which Bun derives from$HOME. The evidence used to reject an untrusted home was that same untrusted home. Replaced withgetent passwd <uid>, the NSS front end, run with a fixedPATH/LC_ALLand no inherited environment. macOS/Windows keep the portableos.userInfo()path.Rebased onto current dev (
6a55d0f12d), absorbing#4766Windows key folding and91f8fd09b4independent-account-home evidence into the call-time resolver. Dev's fail-closed root return for an ambiguous home with no independent evidence is preserved, not weakened — widening it remains #4773's contract, and is not claimed fixed here.Testing
Pristine
HOME, natives built locally.skills.test.tsruntime-mcp/mcp-autoload-redteam.test.ts6e42f973ac(=d9fabc8f^, parent)d9fabc8f5a(regressed)Failures identified by name, not just counts, across repeated pristine-
HOMEruns. Ond9fabc8f5athe three user-scope MCP cases (userSrvempty,mcpManagerundefined,dotUsermissing) fail deterministically and are absent on both the parent and this branch. The two plugin-bundle cases (--no-mcp/ sealed re-discovery) are pre-existing and flake on the parent commit too — unrelated to this issue and not addressed here.Trust properties re-verified out of process on this branch (asserting on the child's own resolution, not a parent-evaluated expectation):
USERPROFILEwithHOMEdeleted → resolves to the passwd home, not the hostile dir.envplantingHOME→ resolves to the passwd home, not the planted dirBoth behave identically to
d9fabc8f5a, so no provenance is traded away.Commands:
bun test packages/coding-agent/test/skills.test.ts— 36 passbun test packages/coding-agent/test/runtime-mcp/mcp-autoload-redteam.test.ts— 16 passbun test packages/utils/test/— 396 pass / 0 failpackages/utils/test/trusted-home-resolution.test.ts— 8 cases: post-load home visibility, home-derived paths moving together, cache invalidation across two changes, ambient restore, filesystem-root refusal, explicit-home escape hatch, project/user scope isolationpackages/utils/test/agent-dir-trust.test.ts— the account-home expectation was evaluatingos.userInfo().homedirin the parent, which follows an isolatedHOME, while the child hasHOMEdeleted and reads the real passwd entry. It now reads the passwd database directly, so it passes under both pristine and normalHOME(previously failed under pristineHOMEond9fabc8f5aas well).bun --cwd=packages/utils run check,bun --cwd=packages/coding-agent run check— cleanRisk classification
low-risk— ordinary fix/maintenance; the repository owner may use the explicitmerge-self-approvedsolo verdict (no independent human review; the verdict name itself records this) with a risk-record comment bound to the exact head.regression-risk— fix with material regression risk; requires one assigned independent domain reviewer whose authenticated exact-headAPPROVEDreview the gate verifies (extra:independent:<login>; the token alone never suffices).high-risk— large refactor, feature, or materially high-risk change (security/auth/install/remove/public API/destructive lifecycle/architecture); requires one assigned independent domain reviewer with an authenticated exact-headAPPROVEDreview (extra:independent:<login>).This touches the trusted-home / credential-provenance boundary in
packages/utils/src/dirs.ts, so it is classifiedhigh-riskand requires independent review regardless of diff size.GJC verdict
Independent exact-head review requested from
snowykr; merge remains blocked until that review is complete.devee43d7db2444ad6ad481d0044b1013b493e18ce0ba7096f72184929070f295019e8173c41ab1e468bun test packages/utils/test/— 398 pass / 5 skip / 0 fail (pristine and ambient HOME)skills.test.ts36 pass / 0 fail;runtime-mcp/mcp-autoload-redteam.test.ts16 pass / 0 failbun --cwd=packages/coding-agent run checkpassesee43d7db2444ad6ad481d0044b1013b493e18ce0, not an earlier commitCloses #4761
🤖 gaebal-gajae