Repository navigation
fix(discovery): compare contained-root identity at plain-stat precision (Windows #4834 flake) - #6634
Conversation
Codex Review SummaryThis 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. |
![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: f536bffc95
ℹ️ 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 expectedDev = typeof expectedRootIdentity.dev === "bigint" | ||
| ? expectedRootIdentity.dev | ||
| : BigInt(expectedRootIdentity.dev); |
There was a problem hiding this comment.
For production calls, FileIdentity.dev and .ino are typed as number and populated by fs.stat() without { bigint: true }, so these new branches still execute BigInt(expectedRootIdentity.dev/ino), exactly as the old code did. When Windows returns an identity above Number.MAX_SAFE_INTEGER, it has already been rounded before this conversion and still fails to match the exact bigint from captureDirectoryIdentity(), causing readContainedFile() to return null and omit the explicit agent directory's SYSTEM/RULES/AGENTS content. Capture the authorized identity as bigint at its source (and add a regression that exercises the mixed-representation boundary) rather than type-checking an already numeric value.
AGENTS.md reference: AGENTS.md:L169-L173
Useful? React with 👍 / 👎.
The user agent directory identity is captured with a plain fs.stat, whose dev/ino are doubles. readContainedFile widened them to bigint and compared against an exact bigint stat, so any NTFS file ID above 2^53 never matched and native user-agent instructions were silently skipped on Windows. The #4834 explicit-agent-dir case failed whenever the temp dir got such an ID.
f536bff to
607dfec
Compare
Yeachan-Heo
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607dfecc75
ℹ️ 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".
| actual: { readonly dev: bigint; readonly ino: bigint }, | ||
| expected: FileIdentity, | ||
| ): boolean { | ||
| return Number(actual.dev) === expected.dev && Number(actual.ino) === expected.ino; |
There was a problem hiding this comment.
When the authorized inode exceeds 2^53, narrowing the current exact inode makes multiple distinct identities compare equal—for example, Number(2n ** 60n + 1n) === Number(2n ** 60n + 2n). Consequently, if an explicit agent directory is replaced after loadCapabilityForHome() captures it by a different directory whose NTFS ID falls in the same rounding bucket, this comparator accepts the replacement and readContainedFile() can load its SYSTEM/RULES/AGENTS content, defeating the identity fence. This collision is fresh evidence beyond the earlier false-negative report: preserve the initially authorized identity as bigint and compare exactly, with a replacement test through readContainedFile() rather than only testing the helper.
AGENTS.md reference: AGENTS.md:L169-L173
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 607dfec, gajae-reviewer on behalf of probepark)
Blocking — P1: preserve the initially authorized identity exactly.
packages/coding-agent/src/discovery/helpers.ts:L1006— narrowing both identity fields accepts distinct directories in the same floating-point rounding bucket. For example, inode1152921504606846977nand1152921504606846978nboth become the same Number.capability/index.ts:L513-L514captures the authorized directory before provider loading. If that directory is replaced beforereadContainedFilecaptures its root at L1018, the new comparator accepts the replacement. The later exact checks only pin that replacement, not the originally authorized directory.discovery/builtin.ts:L296can then load its SYSTEM.md. Confidence: high; this also confirms the current-head connector finding.
Capture the authorized dev/ino with bigint stat at the source. Preserve that exact identity through the context and compare it exactly. Add a regression through readContainedFile that rejects a replacement whose ID shares the authorized ID's rounding bucket.
CI: no failures observed; Virtual integration validation was still pending. The affected precision/helpers tests and TypeScript build passed. At 2026-10-12 08:30 UTC, gajae-approve-gate.py 6634 --head 607dfecc75d0e1bbd0e5d43fca6fd2dfb68914cf returned HOLD_CI (exit 10); it also reported missing cd packages/coding-agent && bun run check from the head CI plan. Neither approval gap is counted as this code finding.
Scope: +43 / -5, 3 files — discovery helper, pure precision regression tests, release fragment. OCR selected one source file (+16 / -5).
Conventions: release fragment present; no generated files or labels changed; no released changelog edits.
ocr: blocking 1 / nit 0
Specification: the rounding false-negative is addressed, but the PR claim that this still rejects every different identity is false.
Checked and clean: same-directory rounding comparison, post-capture exact root/file checks, null/error handling, release fragment.
Not verified: real NTFS replacement behavior and local package checks; no builds or repository tests were run in this pod. A Python IEEE-754 collision probe exited 0 and confirmed two distinct integer identities compare equal after narrowing. The new tests cover an out-of-bucket mismatch, not this collision.
Blocking: 1 — authorized-root identity collision.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:b72475e75b0179bd77db9b3393511988bfb24ed6fefc24942d3e3d8db41c7099 reviewer:critic reviewer-id:gajae-reviewer evidence:authorized-root-identity-rounding-collision
PR body verdict line count=0, not updated. Body is owned by Yeachan-Heo; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:b72475e75b0179bd77db9b3393511988bfb24ed6fefc24942d3e3d8db41c7099 reviewer:critic reviewer-id:gajae-reviewer evidence:authorized-root-identity-rounding-collision
…nded Number comparison
Replace the lossy bigintIdentityMatchesNumeric function with exact bigint
comparisons. FileIdentity now uses bigint for both dev and ino, captured
via stat({ bigint: true }). All identity stability checks now compare
bigint === bigint directly, preventing a security regression where
replaced directories in the same double-rounding bucket could pass
isolation checks on Windows NTFS file IDs exceeding 2^53.
Changes:
- Update FileIdentity interface to use bigint for dev and ino
- Change all fs.stat calls capturing FileIdentity to use { bigint: true }
- Update all fs.stat calls in identity stability checks to use { bigint: true }
- Remove bigintIdentityMatchesNumeric and replace with direct bigint comparison
- Make sameFileIdentity handle both regular Stats and BigIntStats objects
- Update type signatures for functions working with file identities
- Rewrite discovery-identity-precision.test.ts to verify exact ID matching
- Update test files to capture identities with { bigint: true }
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a99d654838
ℹ️ 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".
| it("captures exact large file IDs that exceed 2^53", async () => { | ||
| // Use the actual filesystem to verify exact ID capture. | ||
| // Create a temporary directory and stat it with bigint: true. | ||
| const tempDir = await fs.promises.mkdtemp(path.join("/tmp", "gjc-id-precision-")); |
There was a problem hiding this comment.
On Windows hosts where the current drive has no \tmp directory, mkdtemp(path.join("/tmp", ...)) fails with ENOENT before this regression runs; the second test repeats the same assumption. Use os.tmpdir() so the new suite can run on the platform whose file-identity behavior it targets.
Useful? React with 👍 / 👎.
| expect(large1).not.toBe(large2); | ||
|
|
||
| // With exact bigint comparison, they should not match | ||
| expect(large1 === large2).toBe(false); |
There was a problem hiding this comment.
This assertion only proves that two bigint literals differ and never invokes capturePathIdentity() or readContainedFile(), so restoring the vulnerable rounded-Number comparison in production would leave the test green. The first filesystem test also does not assert that its actual inode exceeds 2^53, and the checked Windows job in .github/workflows/dev-ci.yml does not run this new file; replace this tautology with an observable identity-collision regression through the production read path and include it in the Windows lane.
AGENTS.md reference: AGENTS.md:L169-L173
Useful? React with 👍 / 👎.
|
Fixed probepark's P1 (same as codex's): the Number narrowing is gone and the identity is now exact from capture through comparison.
Local: — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Code review (CI still running on f3ac1bc; approval decision follows once CI finishes).
Review (head f3ac1bc, gajae-reviewer on behalf of probepark)
Blocking: none found in the code review. The earlier authorized-root identity collision is resolved.
packages/coding-agent/src/capability/index.ts:L485-L486,L513-L514captures both authorized identities with bigint stat. The context preserves those values.packages/coding-agent/src/discovery/helpers.ts:L1003-L1010captures the current root exactly and rejects unequal authorized dev/ino before opening the file.discovery/builtin.ts:L296passes the original user-agent identity into that fence.packages/coding-agent/src/capability/fs.ts:L191-L215,L288-L291,L360-L362preserves exact identities across capture, descriptor validation, and directory revalidation. The bigint link-count check at L155-L156 still rejects hard links.packages/coding-agent/src/discovery/agents-md.ts:L43-L49rechecks the home identity in the same bigint domain.
CI: observed checks pass, but Virtual integration validation remains pending. Exact-head run: https://github.com/Yeachan-Heo/gajae-code/actions/runs/38226248672 . The package check, TypeScript build, precision regression, containment helpers, home-isolation regression, and Windows regression passed. No CI failures observed.
Scope: +110 / -26, 8 files — capability identity capture/validation, discovery containment, filesystem regressions, release fragment. OCR selected all four changed source files.
Conventions: release fragment present; no generated files, released changelog edits, or labels changed. Real-filesystem regression coverage replaces the earlier literal-only assertion. Existing test changes only migrate identity capture to bigint; the author explained this in #6634 (comment) .
ocr: blocking 0 / nit 1
Note: packages/coding-agent/test/discovery-identity-precision.test.ts:L9,L59-L68 does not guarantee an inode above 2^53 or a shared rounding bucket. Keep the real-filesystem cases, and consider a deterministic large-ID boundary regression. This is a coverage suggestion, not a code blocker.
Specification: the Windows precision fix now preserves the initially authorized root exactly, instead of narrowing it. The PR description still describes the superseded Number comparator; update it to describe bigint capture and equality.
Checked and clean: all changed source paths, identity propagation to contained reads, null/error paths, descriptor cleanup, hard-link checks, release history, filesystem regression presence, and resolution of the previous finding.
Not verified: forced same-rounding-bucket NTFS replacement on real hardware. No repository builds or tests were run in this pod; execution evidence is the exact-head CI above.
Approval gate: python3 /opt/data/scripts/gajae-approve-gate.py 6634 --head f3ac1bce2b83247c85e436bccd14b03345570904 returned HOLD_CI (exit 10) during this review on 2026-10-12. Pending: Virtual integration validation. Missing local checks: none. Resume the approval decision after that check finishes; no approval is submitted now.
PR body verdict line count=0, not updated. The PR body is owned by Yeachan-Heo and is unchanged.
Blocking findings resolved by exact bigint capture and comparison; superseded by the follow-up code review.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR changes filesystem identity capture and revalidation to use exact bigint values end-to-end, fixing lossy comparisons for large filesystem IDs. No actionable merge-blocking defect was found across the contract, correctness, security, verification, and compatibility reviews.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
packages/coding-agent/test/discovery-identity-precision.test.ts:9-18 compares the temporary file’s identity with bigint stat results but does not assert that either ID exceeds Number.MAX_SAFE_INTEGER. The focused test runs on Ubuntu, so it may not catch a future conversion through number; optionally add a deterministic boundary case above 2^53. The separate Windows home-isolation CI suite passed.
CI / Verification
At reviewed head f3ac1bce2b83247c85e436bccd14b03345570904, GitHub reported 23 successful and 7 skipped check-runs, with no failures or cancellations. The relevant Windows pr-4834-home-isolation.test.ts job, changed-test jobs, protected affected-path aggregate, gjc-state-gates, and virtual integration validation passed. The opt-in WSLv2/NTFS DrvFS qualification was skipped. No local tests were run during this read-only review.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Exact-head bigint producer/consumer contracts address the base mismatch; discovery remains fail-closed and the changelog follows repository policy. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Exact identity checks and existing root/open-file revalidation remain intact; no new failure or race defect was established. |
| A3 — Security / Privacy / Trust | APPROVED |
The change does not expand read authority or introduce a fail-open path; existing containment and identity guards remain. |
| A4 — Verification / Tests / CI | APPROVED |
Observable read-path tests and exact-head CI passed; the optional boundary-test improvement is non-blocking. |
| A5 — Context / Compatibility / Platform | APPROVED |
In-repository producers and consumers use bigint consistently; identity is not persisted, and no materially preferable duplicate abstraction was found. |
Limitations
No intent_projection artifact was present in the checkout. External consumers outside this repository were not observable; the inspected repository did not establish a compatibility guarantee for these internal identity helpers.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head f3ac1bc, gajae-reviewer on behalf of probepark)
CI finished; no blocking findings. This gate-only verdict follows the exact-head code review: #6634 (review) . The diff was not reviewed a second time.
CI: python3 /opt/data/scripts/gajae-approve-gate.py 6634 --head f3ac1bce2b83247c85e436bccd14b03345570904 returned ALLOW (exit 0) at 2026-10-12T09:32:26Z. Pending, failed, and missing local checks: none. The plan covers check:@gajae-code/coding-agent. Exact-head execution evidence: https://github.com/Yeachan-Heo/gajae-code/actions/runs/38226248672 . No repository builds or tests were run in this pod.
Scope: +110 / -26, 8 files — capability identity capture, discovery containment, filesystem regressions, release fragment.
Conventions: release fragment present; no generated files, released changelog edits, or labels changed.
ocr: blocking 0 / nit 1
Notable:
packages/coding-agent/src/capability/index.ts:L485-L486,L513-L514captures authorized identities as bigint.packages/coding-agent/src/discovery/helpers.ts:L1003-L1010rejects an unequal root identity before opening the file. The previous rounding-collision finding is resolved.- The earlier non-blocking large-ID boundary-test suggestion remains. See the linked code review for source-path coverage and limitations.
Checked and clean: prior exact-head code review has blocking=0; no subsequent peer changes-requested review or new human finding observed; approval gate has no missing checks. Real NTFS forced-collision behavior was not locally exercised.
Blocking: none.
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:edf424d22b30ea09864e3bff07dbbdddc5fe3d53eaf581e6624d60b733f32bef reviewer:human reviewer-id:probepark evidence:exact-bigint-identity-fence;prior-code-review-blocking-0;ci-gate-allow
PR body verdict line count=0, not updated.
Body verdict line is owned by Yeachan-Heo; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:edf424d22b30ea09864e3bff07dbbdddc5fe3d53eaf581e6624d60b733f32bef reviewer:human reviewer-id:probepark evidence:exact-bigint-identity-fence;prior-code-review-blocking-0;ci-gate-allow
3 participants
Problem
Windows dev:doctor + session-path regressionintermittently failspr-4834-home-isolation.test.ts› "explicit agent directory remains authoritative when the default agent root escapes" on PRs that don't touch it. The test expects["# explicit system"]and gets[].772628c, run 38193244595c74a87a, run 38211459954 (attempt 3)Root cause
capability/index.tscapturesuserAgentIdentitywith a plainfs.stat, sodevandinoare doubles.readContainedFilewidened those to bigint withBigInt(expected.ino)and compared them against the exact bigint stat fromcaptureDirectoryIdentity. NTFS file IDs are 64-bit and often exceed 2^53. When the temp agent dir's ID does, the plain stat has already rounded it, so the widened value can't equal the exact one. The read then returnsnulland the native user-agent instructions are silently dropped. Whether it fails depends on which file ID the directory happens to get.Fix
bigintIdentityMatchesNumericconverts the exact bigint value to a double, the same rounding the plain stat applied, and compares in that domain. It still rejects any different identity.Tests
test/discovery-identity-precision.test.ts: covers a 2^60-scale ID that a plain stat rounds (matches), different dev/ino (rejected), and a large ID beyond rounding distance (rejected). Result 3/0.test/pr-4834-home-isolation.test.ts40/0 on linux.check:typesand biome are clean.This replaces the earlier lane commit, which didn't change behavior for numeric input and touched
diagnostic-artifact.json.—
[repo owner's gaebal-gajae (clawdbot) 🦞]