Repository navigation
Conversation
probepark
left a comment
There was a problem hiding this comment.
Review (head 40f857b, gajae-reviewer on behalf of probepark)
CI: PR-caused 1, unclassified 1 — the PR's own new test fails at this exact head on Linux (Affected path validation / test:packages/coding-agent/test/task-artifact-owner-access.test.ts, job 111278849579). This fails the Affected path validation aggregate and the evidence producer. Windows dev:doctor + session-path regression (job 111276212442) fails in packages/natives/test/walker-pool-unavailable.windows.test.ts:127 (threads 21 > 20). That test is outside this diff and probably unrelated, but it is not confirmed on dev.
Scope: +982 / -1, 4 files — packages/coding-agent/src/session/internal (managed-session-storage.ts, task-artifact-owner-access.ts) and tests/fixture. Reviewable (ocr) 337 lines.
Conventions: CHANGELOG none (internal API with no user-visible activation, per the PR summary), generated files none, labels none.
Notable:
packages/coding-agent/test/task-artifact-owner-access.test.ts:333: "rejects both real cross-process managed publication windows and acknowledges release" fails on ubuntu CI:staging writer failed: Error: task_owner_writer_staging_boundary_missingattest/fixtures/task-owner-access-writer.ts:156.- Root cause: the fixture's staging phase (
task-owner-access-writer.ts~L131-153) hooksfsp.openfor a*.stagingfile inownerRoot. On Linux,ManagedSessionDescendantStorekeeps a nativeRecoveryFsRootauthority (managed-session-storage.ts~L1561-1575,process.platform === "linux"). As a result,publishNoReplace(L2088-2098) goes through#publishRetainedNoReplace(L2478), which creates a.gjc-publish-<pid>-<uuid>temp throughauthority.createManagedand never callsfsp.open(...staging). The hook never fires andheldstays false. The PR body's localbun test … exit 0was most likely run on a platform without the retained authority (macOS/Windows path-backed branch), so it does not cover Linux. Either make the staging window platform-aware (hook the retained-authority path on Linux, or skip/branch that phase with an explicit reason) or show the test passing on Linux at the exact head.
Blocking: 1 (the PR's own new test fails on Linux CI at the exact head).
Body verdict line count=0 (no gajae.pr-review-verdict.v1 line in PR body), not updated. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:65873466a654cdf4db64867f4d313a5740346d4877dd8794a52b15ea0ccbe9cc reviewer:critic reviewer-id:gajae-reviewer evidence:own-test-fails-linux-ci;staging-hook-bypassed-by-retained-authority;windows-walker-pool-unclassified
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:65873466a654cdf4db64867f4d313a5740346d4877dd8794a52b15ea0ccbe9cc reviewer:critic reviewer-id:gajae-reviewer evidence:own-test-fails-linux-ci;staging-hook-bypassed-by-retained-authority;windows-walker-pool-unclassified
|
Old code-event37148091900 failure retained: Linux affected owner-access fixture called retained-store publication, which does not execute the JS UUID staging path; actual Linux native publication remained authoritative. Repaired fixture only: use existing checked managed path publisher/replacer on all platforms, real independent PID/open FD/inode/release acknowledgement, no native mock/success/skip. New exact e438111, production actual dev337 unchanged. Fresh exact clean byte-bound native QA tests and full package vendor/Biome/TypeScript exit0, body updated. Separate unchanged Windows walker thread count <=20 observed21 also preserved; no limit change or replay credited. New-head code-event/review required. |
Known owner roots must be verified before construction can repair security. Capture immutable owner authority only when actual managed staging and replacement writers are quiescent, and preserve exact native dispositions. Lore-id: 65e28dc3 Constraint: no owner producer activation or synthetic native completion Confidence: high Scope-risk: narrow Reversibility: easy Tested: actual independent managed staging and replacement fd processes Tested: 19 access and GC codec tests 105 assertions plus package types Tested: 139-pass storage codec access union with 8 existing skips Not-tested: full persistent owner and GC SDK consumer activation
Retained Linux stores publish inside native code, so their store method never opens the JavaScript path publisher's UUID staging window. Use the existing managed path publication and checked replacement APIs directly to hold genuine cross-process descriptors without faking native authority or skipping probes. Lore-id: 54fa128d Constraint: original native result and publication guards remain unchanged Confidence: high Scope-risk: narrow Reversibility: easy Tested: owner access tests on Darwin with genuine independent writer processes Not-tested: fresh Linux and Windows CI at the new head
e438111 to
cd7883c
Compare
probepark
left a comment
There was a problem hiding this comment.
Review (head cd7883c, gajae-reviewer on behalf of probepark)
CI: PR cause/infrastructure failure — Virtual integration validation failed while selecting an authoritative dev base because the workflow could not obtain the green dev run list (scripts/ci-virtual-integration.ts:357); no changed-code test assertion ran. Other completed affected-path checks passed.
Scope: +1002 / -1, 4 files — coding-agent session managed storage and task-owner access, plus fixtures/tests.
Conventions: CHANGELOG fragment missing; generated files none; labels none.
Notable:
packages/coding-agent/src/session/internal/task-artifact-owner-access.ts:181-381— owner identity capture, manifest validation, and parent-bound removal forwarding are covered by the added tests. I found no additional blocking defect in the reviewed source.packages/coding-agent/test/task-artifact-owner-access.test.ts:843-923— the cross-process publication-window coverage restores its spies and releases the child process; no test pollution issue found in this path.
Blocking: Add the required package changelog fragment for this functional coding-agent change.AGENTS.md:202-203requirespackages/<pkg>/changelog.d/<slug>.mdand says not to edit the shared package CHANGELOG directly. The PR body currently says there is no changelog fragment, so this release-history requirement is unmet.
D-TEST-POLICY: The PR adds real bounded cross-process publication tests rather than only mock-based unit tests. The virtual integration check did not reach its test phase, so that integration evidence remains unavailable.
Verdict: gaja.pr-review-verdict.v1 needs-human sha256:222fe089337384a4139fa1f081386cb91f88c13b9bd14ece692590cda144e48d reviewer:critic reviewer-id:gajae-reviewer evidence:virtual-integration-base-selection-failed;changelog-fragment-missing;generated-files-none
Body verdict line is absent and was not edited. Suggested verdict line: gaja.pr-review-verdict.v1 needs-human sha256:222fe089337384a4139fa1f081386cb91f88c13b9bd14ece692590cda144e48d reviewer:critic reviewer-id:gajae-reviewer evidence:virtual-integration-base-selection-failed;changelog-fragment-missing;generated-files-none
|
Current exact head: |
|
Follow-up on review 5402917375 (head
Remaining blocking: (2) only. The CHANGES_REQUESTED verdict stays on this head. Push the fragment and re-request review; the new head gets a fresh verdict. |
probepark
left a comment
There was a problem hiding this comment.
Review update (head cd7883c, unchanged; gajae-reviewer on behalf of probepark)
CI: resolved. The earlier blocker (Virtual integration validation) passed on this exact head: Dev CI run 37154515811 attempt 2 completed with success. Job: https://github.com/Yeachan-Heo/gajae-code/actions/runs/37154515811/job/111323739313
Blocking: none.
Nit (optional): there is no packages/coding-agent/changelog.d/<slug>.md fragment. The change only touches src/session/internal/* and tests, so it is not user-facing. CONTRIBUTING.md:84 asks for a fragment only for user-facing changes "when appropriate", so adding one is optional.
This replaces the blocking verdict in the earlier follow-up: #6292 (comment)
The earlier CHANGES_REQUESTED reviews (5402472571 @40f857b, 5402917375 @cd7883c) are superseded. A maintainer makes the final approval.
Digest: sha256:222fe089337384a4139fa1f081386cb91f88c13b9bd14ece692590cda144e48d (base 03c88bb...head cd7883c)
Superseded: CI blocker resolved on exact head cd7883c; see latest review
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The PR adds internal owner-access helpers, expected-identity managed-store checks, parent-identity-bound native removal, and subprocess tests. No merge-blocking defect is established. One non-blocking Linux publication-quiescence gap remains in the new deletion-evidence path; there is no production consumer at this head.
Findings / Required Changes
- [P2] Do not issue deletion evidence during retained-root publication —
packages/coding-agent/src/session/internal/task-artifact-owner-access.ts:198-203- At merge-base
03c88bb12413241127a3024c6ba70a68e68988be, the retained-root publisher already existed; this PR adds the owner-tree guard and deletion-evidence path. On Linux, an owner store's retained publisher creates.gjc-publish-${pid}-${uuid}before its later read, fsync, and rename operations, but the new guard recognizes only the path-based.staging/.replacementforms. If a writer is paused with that retained marker present, concurrent evidence capture includes it and can return a snapshot as quiescent. A caller can then pass that snapshot and captured parent identity to the new exact-removal operation; because the temporary is unchanged relative to the snapshot, native exact-tree validation accepts it and the removal scrubs/detaches it. When the publisher resumes, its later read/fsync/rename fails and the artifact is not published. - The parent-identity and exact-tree checks protect against replacement of the parent/tree, but do not establish writer quiescence when the active temporary is already part of the captured tree. The expected behavior is to coordinate all owner writers with evidence capture/retirement (or require writer-closure evidence held through retirement); filename recognition alone also cannot prevent a new writer starting after capture. Cover the retained Linux publication interval in regression coverage.
- Non-blocking for this merge: exact-head source search found no production consumer of this internal module, and the package export map blocks external internal-path imports. Fix before connecting this evidence path to lifecycle retirement.
- At merge-base
Non-blocking Observations
- The Windows-only expected-identity branch in
packages/coding-agent/test/task-artifact-owner-access.test.ts:415-418was not run on Windows: the exact-head owner-access task ran on Ubuntu, and the passing Windows session-path job uses a different test list. A focused Windows run would improve platform coverage; this is not evidence of a Windows defect and does not affect the verdict.
CI / Verification
- GitHub reports 16 successful and 17 skipped checks for the reviewed head. The required affected test task, coding-agent package check, native build, final affected aggregate, and PR-eligible virtual-integration gate passed on exact head
cd7883cf70a08e01fb86b1001b47cb896da846c2(Actions run37154515811, successful attempt 2). Attempt 1 failed virtual integration while selecting the authoritative terminal-greendevbase; attempt 2 passed. The planner/aggregate validation treats skipped jobs according to eligibility and rejects skipped required tasks. - The PR body also claims local targeted tests and package check passed; those commands were not independently rerun during this review. Review agents performed source/metadata analysis only; no PR code or tests were executed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
No repository/API contract found requiring these internal store returns to be immutable; explicit removal behavior is in scope and the module is not externally exported. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Finding 1 is a verified but currently non-blocking API-level race; expected-root identity checks and exact native result forwarding otherwise preserve the relevant ownership boundary. |
| A3 — Security / Privacy / Trust | APPROVED |
Locator/session/manifest/tree identities and captured parent identity are checked at the native removal boundary; no unauthorized path or activation was identified. |
| A4 — Verification / Tests / CI | APPROVED |
Exact-head selected tests and final validation gates passed; the Windows-only coverage gap is optional and non-blocking. |
| A5 — Context / Compatibility / Platform | APPROVED |
No production caller or external export is present; the implementation reuses the managed descendant store and native exact-tree contract. Finding 1 is the same A2 root cause, not a separate blocker. |
Limitations
- The race's practical impact requires an in-package caller to capture and use deletion evidence; no production caller exists at the reviewed head.
- GitHub Actions evidence and aggregate logic were inspected, but exact branch-protection status-check requiredness could not be independently certified: the ruleset response exposed required review, while the classic branch-protection endpoint returned HTTP 401.
|
Closing #6292 as superseded; no rebase or source changes were made in this lane. Evidence at disposition: PR head The exact-head PR CI run 37154515811 attempt 2 completed successfully; affected tests, package check, and virtual integration passed. No new tests were run because no source changed. Formal review decision remains REVIEW_REQUIRED; the latest reviewer follow-up left the changelog-fragment request outstanding. It is not being claimed as fixed here: this PR is closing solely because the implementation is already merged through #6303. — |
Summary
Complete readonly managed owner-access API and exact native removal forwarding, without owner creation or automatic session/task/SDK/GC activation. Expected subtree identity is verified before any security repair so a replacement's inode, mode, ctime and bytes stay untouched. Parent identity is passed to the actual native operation and its full result is preserved; success is not manufactured.
Exact review range
Base
devcaptured at audited advancing upstream03c88bb12413241127a3024c6ba70a68e68988be; externally merged codec parent9c54c08811128685e00cf5909a2ff575f1162e37is its ancestor.Head
cd7883cf70a08e01fb86b1001b47cb896da846c2.Own production AND actual dev production additions+deletions: 337 (229 access +108 managed-store churn). Test and subprocess fixture lines are counted separately. No unrelated codec or retention delta is reapplied; those are already in actual dev.
Exact clean-source verification
bun test packages/coding-agent/test/task-artifact-owner-access.test.ts packages/coding-agent/test/task-artifact-owner-codec.test.ts packages/coding-agent/test/session-storage.test.ts— exit 0.bun --cwd=packages/coding-agent run check— exit 0 at exactlycd7883cf70a08e01fb86b1001b47cb896da846c2, including vendor verification, Biome andtsc -p tsconfig.json --noEmit.Independent detached clean tracked QA; unchanged native source and actual addon match committed strict 0.18.5 metadata. Metadata not rewritten. Exact source stayed clean before/after. Report
05-owner-access-base03c88-qualification.jsonbinds those command exit codes to this exact SHA; full output retained alongside it.Real bounded independent Bun processes exercise existing checked managed path publication APIs (
publishManagedFileNoReplace,replaceManagedFileSync): opened.stagingand.replacementdescriptors, distinct PID and dev/inode matching the marker, refusal before deletion capture and acknowledged destination bytes after release. No native authority/success is mocked. The prior store-method fixture passed Darwin but failed real Linux CI because Linux retained stores publish inside native code and do not open the JS path publisher's UUID window. This new fixture tests the actual existing path APIs on every platform, with captured owner root and predecessor identity; it does not claim native-retained Linux writer pause instrumentation. Native forwarding tests preserve actual result identity rather than requireok:trueor inode disappearance.Retained CI failure history
Old head
40f857ba9e0ec7ef6062cf7fe055bb6f960afd9bcode-event run37148091900failed: affected access testtask_owner_writer_staging_boundary_missing(fixture-specific Linux implementation mismatch, corrected above), plus unchanged Windows walker thread count expected <=20 / observed21 (walker-pool-unavailable.windows.test.ts:127). Both failures preserved; no rerun, gate weakening, skip or thread-limit change. Affected aggregate correctly failed closed. Fresh CI/review are required on this new exact head; old checks are not transferred.Pre-rebase corrected head
e4381115fb469b8e63afe3db8b6bf4819fd5eefecode-event37151725860completed SUCCESS; metadata37151797592skipped/uncredited. Current head is an owned-only rebase onto audited advancing dev; old green/review is not transferred. Fresh current exact-clean tests/package passed, fresh current CI/review pending.Boundaries
Darwin runtime proof, no arbitrary FD revocation, Linux payload-scrub parity, physical namespace reclamation or final task-move E2E claim. Existing skips/warnings/info retained uncredited. Locator data never grants permission. No fake native success, unsafe unlink or writer/coordinator redesign.
No changelog fragment: inert internal APIs, no automatic runtime producer change. Current-head code-event CI and write-access maintainer review required; no old approval transferred or leader merge. Original44 staged work and consolidated #6240/reference untouched.