Skip to content

fix(notifications): resolve intermediate notifications dir only for exact unlink - #3834

Merged
Yeachan-Heo merged 2 commits into
Yeachan-Heo:devfrom
innocarpe:fix/issue-3761-notify-symlink-successor
Aug 5, 2026
Merged

fix(notifications): resolve intermediate notifications dir only for exact unlink#3834
Yeachan-Heo merged 2 commits into
Yeachan-Heo:devfrom
innocarpe:fix/issue-3761-notify-symlink-successor

Conversation

@innocarpe

@innocarpe innocarpe commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Successor to closed PR #3832 addressing exact-head REQUEST_CHANGES.

Bounded slice of #3761 (does not auto-close the issue): multi-account layouts that share notifications/ via a directory symlink could not retire transition locks because native exact_unlink rejects intermediate reparse points. That left telegram-daemon.steal behind and failed setup with provisional ownership that could not be retired safely while durable settings stayed armed.

Changes vs #3832

  1. Final-component fail-closed under race — canonicalize only dirname(file) via realpathSync, then rejoin the original basename before native.exactUnlink. Native still applies AT_SYMLINK_NOFOLLOW to the final path component at mutation time. Full-file realpathSync(file) from fix(notifications): activate daemon through symlinked notifications dirs (#3761) #3832 is removed.
  2. TOCTOU regression — after JS parent realpath preflight, swap the final component to a symlink pointing at the moved regular target; expect reparse_point with target and alias retained.
  3. Generation 50 closure — bump DAEMON_GENERATION 49→50, document generation 50 in the contract, pin test, regenerate scripts/telegram-daemon-generation-manifest.json after guarded edits.
  4. Formatting — Biome-clean test imports; bun --cwd=packages/coding-agent run check green.

Test plan

  • bun test packages/coding-agent/test/issue-3761-symlinked-notifications-activation.test.ts — 4 pass
  • generation-50 pin in notifications-telegram-daemon.test.ts
  • bun test scripts/telegram-daemon-generation-guard.test.ts — 48 pass
  • bun scripts/telegram-daemon-generation-guard.ts --validate-current-tree
  • bun scripts/telegram-daemon-generation-guard.ts <upstream/dev> <head> (immutable revisions)
  • bun --cwd=packages/coding-agent run check
  • Live Linux multi-account Telegram bot/inbound round-trip (not run; no maintainer credentials)

Scope

@innocarpe
fix(notifications): resolve intermediate notifications dir only for e…
a4c8526 
…xact unlink

Successor to closed Yeachan-Heo#3832. Multi-account layouts that share notifications/
via a directory symlink still need intermediate reparse points resolved
before native exact unlink can retire transition locks. Full-path realpath
followed final components and could race a final-component symlink swap
past native AT_SYMLINK_NOFOLLOW.

Canonicalize only the parent directory and rejoin the original basename so
native mutation still no-follows the final path component. Bump
DAEMON_GENERATION 49→50 and refresh the generation guard manifest for the
protected exactUnlinkNotificationFile change.

Lore-id: 3761act02
Constraint: final-component file symlink must remain reparse_point under TOCTOU
Constraint: do not claim full Yeachan-Heo#3761 closure -- bounded activation slice only
Rejected: full-file realpathSync | weakens final-component no-follow under race
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: issue-3761-symlinked-notifications-activation; generation-50 pin; generation-guard suite; coding-agent check
Not-tested: live Linux multi-account Telegram bot inbound round-trip

Refs Yeachan-Heo#3761
@Yeachan-Heo

Yeachan-Heo commented Aug 5, 2026

Copy link
Copy Markdown
Owner

REQUEST_CHANGES — GLM-DeepSeek exact-head hostile review

Signed terminal disposition for PR #3834, the successor to closed #3832, against exact dev base 732856b3ccb3fade6e9fbc17908a4fbca5a7682f. Continues the retired GPT/Qwen lane from its visible evidence; no source mutation, push, merge, CI control, or release action performed.

Exact head / base / CI

Source review — all three prior #3832 blocking findings resolved

exactUnlinkNotificationFile now canonicalizes only dirname(file) via realpathSync, rejoins the original basename, and passes path.join(resolvedParent, base) to native.exactUnlink. The native contract (crates/pi-natives/src/path_identity.rs:3262-3335, Linux; mirrored Windows branch :7103-7166) walks each ancestor component with O_NOFOLLOW/AT_SYMLINK_NOFOLLOW and rejects any intermediate reparse point, then applies a final fstatat(..., AT_SYMLINK_NOFOLLOW) + S_IFLNK check on the terminal component at mutation time. Because the terminal basename is the original un-resolved name, the no-follow check is preserved; resolving only the parent is the correct bounded repair and does not weaken final-component fail-closed semantics. The JS preflight lstatSync early-return for a known symlink is a fast path, not the security boundary — the native call remains authoritative.

  1. HIGH (prior fix(notifications): activate daemon through symlinked notifications dirs (#3761) #3832 Integrate native gjc team runtime #1 — final-component TOCTOU): RESOLVED. Full-file realpathSync(file) is removed. The added exactUnlink refuses final component swapped to symlink after parent realpath preflight test intercepts fs.realpathSync for the parent, swaps the resolved final file to a symlink pointing at the moved regular target, and asserts { ok: false, code: "reparse_point" } with both the moved target (retained, readable) and the alias (a symlink) surviving. This deterministically exercises the TOCTOU window between JS parent preflight and native terminal-component AT_SYMLINK_NOFOLLOW. Correct.

  2. MERGE-BLOCKING (prior fix(notifications): activate daemon through symlinked notifications dirs (#3761) #3832 Remove bundled telemetry reporting surfaces #2 — generation closure): RESOLVED. DAEMON_GENERATION advanced 49→50, documented in telegram-daemon-contract.ts, generation-pin test name/comment/body updated to 50, and telegram-daemon-generation-manifest.json regenerated (exactUnlinkNotificationFile 723c9b0...→d63a4f1e...; DAEMON_GENERATION 13e924c...→51f8d5a...). Natural CI generation guard job 92201657938 is SUCCESS — the exact manifest now matches the exact tree.

  3. Direct diff hygiene (prior fix(notifications): activate daemon through symlinked notifications dirs (#3761) #3832 GJC rebranding plan: red-claw UI redesign #3 — Biome): RESOLVED. check:@gajae-code/coding-agent job 92202531980 is SUCCESS.

Scope adherence

CI failures on this head — classified

Three exact-head CI jobs failed: test:@gajae-code/coding-agent:shard-1-of-8 (92202531973), evidence producer (92203644680), and the aggregate Affected path validation (92203679884). Investigation:

  • The shard-1 failure is SDK operation inventory > accepts the committed generated matrix (packages/coding-agent/test/sdk-operation-inventory.test.ts:64-66): Pending review source seam: agent_session:registerToolSessionTransitionCleanup. Add it to SEAM_TO_SDK or LOCKED_EXCLUSIONS.
  • That seam was introduced by fix(session): share one artifact store across the subagent tree #3813 (fix(session): share one artifact store across the subagent tree), which is the current dev HEAD — i.e. this PR's exact base. git log -S "registerToolSessionTransitionCleanup" -- packages/coding-agent/src/session/agent-session.ts attributes it solely to 732856b (fix(session): share one artifact store across the subagent tree #3813), and the SEAM_TO_SDK/LOCKED_EXCLUSIONS maps contain no entry for it at this base.
  • I reproduced the failure on the clean base checkout independently: bun test packages/coding-agent/test/sdk-operation-inventory.test.ts → 16 pass, 1 fail, identical Pending review source seam: agent_session:registerToolSessionTransitionCleanup error. This PR's diff touches only notification-service.ts / telegram-daemon-* / manifest / test files and neither introduces nor classifies that seam.
  • evidence producer and the aggregate Affected path validation are the fail-closed downstream consequence of the shard failure (CI_DEV_TELEGRAM_GUARD_RESULT: success; the failure is the upstream shard, not this PR's generation guard).

Verdict: the three red jobs are a pre-existing dev-base regression from #3813, not a defect of #3834. They block the mergeability signal but are not attributable to this PR's changes. That pre-existing failure is a separate owner-lane concern (the #3813 seam needs classification in SEAM_TO_SDK or LOCKED_EXCLUSIONS, or the source refactored to a reviewed name) and is out of scope for this bounded #3761 symlink repair.

Verdict: REQUEST_CHANGES

The implementation direction is admitted and all three prior #3832 blocking findings are correctly resolved; the source change is sound and the TOCTOU + generation + formatting closures are genuine. This PR is not merge-ready solely because exact-head CI is red. Per owner batch policy the mergeability signal must be green on the exact current head before approval; the red jobs cannot be transferred as "pre-existing" across a merge.

Required path for the successor: (a) resolve the pre-existing registerToolSessionTransitionCleanup seam in the SDK operation inventory on dev (classify or rename — that is a #3813 follow-up, not part of this PR's #3761 scope, but it must land on dev so this PR's exact head goes green), then (b) re-trigger exact-head CI on a current-dev-based head. Once the shard/evidence/aggregate jobs pass on the exact head, the notification change is review-approvable on its own merits. No live Telegram credentials are requested for this review.

Contributor ledger / external policy

Credit does not create overlapping mutation ownership.

Signed: GJC owner batch (GLM-DeepSeek lane) / Yeachan-Heo / 2026-08-05

@Yeachan-Heo Yeachan-Heo mentioned this pull request Aug 5, 2026
Closed
@innocarpe innocarpe mentioned this pull request Aug 5, 2026
Merged
3 tasks
@innocarpe
fix(sdk): lock registerToolSessionTransitionCleanup out of public inv…
7ac7d34 
…entory

Yeachan-Heo#3813 added AgentSession.registerToolSessionTransitionCleanup for shared
artifact-manager ownership on session transitions. The method is an internal
lifecycle registration seam, parallel to registerToolSessionCleanup, but was
never added to LOCKED_EXCLUSIONS. Exact-head Dev CI then fails the generated
SDK operation inventory check on any PR based on current dev.

Classify the seam as a locked exclusion and regenerate the committed matrix.

Lore-id: 3813seam01
Constraint: do not expose as a public SDK control
Rejected: map to a new SDK operation | no user-facing control exists
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: sdk-operation-inventory.test.ts (17 pass); inventory --check
Not-tested: full coding-agent shard matrix on CI

Refs Yeachan-Heo#3813
@innocarpe

innocarpe commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Pushed an additional commit on this head to clear the pre-existing dev inventory failure called out in review:

Notification fix commits are unchanged. Exact-head CI should now pass the SDK operation inventory / shard-1 path.

@yazzang-homelab yazzang-homelab mentioned this pull request Aug 5, 2026
Closed
5 tasks
@Yeachan-Heo
Yeachan-Heo merged commit 66209ba into Yeachan-Heo:dev Aug 5, 2026
30 checks passed
@yazzang-homelab yazzang-homelab mentioned this pull request Aug 6, 2026
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@innocarpe @Yeachan-Heo