Skip to content

fix(notifications): exclude retained cleanup leak artifacts from endpoint health - #3729

Open
Yeachan-Heo wants to merge 1 commit into
devfrom
fix/notify-endpoint-leak-artifact-classification
Open

fix(notifications): exclude retained cleanup leak artifacts from endpoint health#3729
Yeachan-Heo wants to merge 1 commit into
devfrom
fix/notify-endpoint-leak-artifact-classification

Conversation

@Yeachan-Heo

Copy link
Copy Markdown
Owner

What

notify health counted retained native cleanup leak artifacts as endpoint files, pinning it to a WARN that notify recovery could never clear.

classifyNotificationEndpoint routes every unparseable file in the endpoint directory to unreadable unless the name matches a canonical lifecycle artifact. Retained exact-unlink leak artifacts are zero-byte by construction, so each one was counted as an unreadable endpoint.

Found while dogfooding the current dev build for release prep. On a real machine:

[warn] endpoints: 1 dead / 10 unreadable of 13 endpoint file(s); run recovery

The directory actually held 3 endpoints plus 10 .gjc-exact-unlink-placeholder-* files. Running the suggested recovery only flipped the wording and never converged:

health   -> 10 dead / 0  unreadable of 13; run recovery
recovery -> (placeholders remain)
health   -> 0  dead / 10 unreadable of 13; run recovery
recovery x3 -> still "10 unreadable"

Why it could never clear

The retention is deliberate, so recovery is not at fault:

  • crates/pi-natives/src/path_identity.rs:2956 returns RetainedFailure(name, "cleanup_pending") even when the placeholder identity matches — there is no unlinkat on that path. ExchangePlaceholderRemoval::Removed is matched in 5 places and constructed in none (#[allow(dead_code, reason = "retained cleanup outcomes are platform-conditional")]).
  • telegram-daemon.ts:1569 reapNotificationLeakArtifact explicitly returns "retained" for this prefix.

So the artifacts are meant to persist. The bug is that the health scanner misreads them as endpoints.

Change

Reuse the existing isNotificationLeakArtifactName predicate in unreadableEndpointResult so all four NOTIFICATION_LEAK_ARTIFACT_PREFIXES classify as non-endpoint — a kind the scanner already skips before counting (notification-service.ts:801). Genuinely corrupt endpoint files still classify as unreadable.

Classification only. The retention protocol in path_identity.rs is untouched — it is intentional fail-closed behaviour from #3596 and not something to change in a release-prep window. The 271 placeholders sitting in a real session store are a separate concern and are left for their own issue.

Verified on the real notifications directory

endpoints check
before [warn] 1 dead / 10 unreadable of 13 endpoint file(s); run recovery
after [warn] 1 dead / 0 unreadable of 3 endpoint file(s); run recovery
after notify recovery [ok] 2 live, 0 unverified
after a second notify recovery [ok] 2 live, 0 unverified (converges)

Total is corrected from a phantom 13 to the real 3. The remaining 1 dead was a genuinely dead endpoint, which recovery then cleared properly.

Tested

  • bun test packages/coding-agent/test/notifications-service.test.ts packages/coding-agent/test/notifications-telegram-daemon-self-heal.test.ts packages/coding-agent/test/notifications-telegram-daemon-staging-temp-leak.test.ts — 75/75
  • The 3 added tests fail without the src change (verified by stashing it: 2 fail)
  • bun run check:types in packages/coding-agent — clean
  • bun x biome check on both touched files — clean

Changelog entry added under ### Fixed.

…oint health

`classifyNotificationEndpoint` routed every unparseable file in the endpoint
directory to `unreadable` unless its name matched a canonical lifecycle
artifact. Retained native exact-unlink leak artifacts are zero-byte by
construction, so each one was counted as an unreadable endpoint. On a real
machine that inflated the scan from 3 endpoints to 13 and reported
"1 dead / 10 unreadable of 13 endpoint file(s); run recovery" — a WARN that
`notify recovery` could never clear, because the reaper deliberately retains
those artifacts (telegram-daemon.ts reapNotificationLeakArtifact) and the
native protocol never constructs ExchangePlaceholderRemoval::Removed.

Reuse the existing `isNotificationLeakArtifactName` predicate so all four
`NOTIFICATION_LEAK_ARTIFACT_PREFIXES` classify as `non-endpoint`, which the
health scanner already skips before counting. Genuinely corrupt endpoint
files still classify as `unreadable`.

This changes classification only. The retention protocol in
crates/pi-natives/src/path_identity.rs is deliberate fail-closed behaviour
from #3596 and is left untouched.

Verified on the real notifications directory:
  before: [warn] endpoints: 1 dead / 10 unreadable of 13 endpoint file(s)
  after:  [warn] endpoints: 1 dead / 0 unreadable of 3 endpoint file(s)
  after `notify recovery`: [ok] endpoints: 2 live, 0 unverified — and it
  now converges across repeated runs instead of oscillating dead<->unreadable.

Tested: notifications-service, telegram-daemon-self-heal, and
staging-temp-leak suites (75/75); coding-agent tsc --noEmit; biome check.
The three added tests fail without the src change.

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terminal maintainer red-team review at exact head abe22990543f72eb2bb9c88f343e88c79c150408 (base dev c1bf3be50e2cf0dfdf51b57eb43f7a99521c94b8; base is an ancestor of head).

Scope: notification endpoint/leak-artifact classification, security/data exposure, redaction/health classification, and user-facing contract. No source mutation.

Evidence: exact-head GitHub checks 16 success / 6 skipped / 0 failure; local exact-head bun test packages/coding-agent/test/notifications-service.test.ts 57 pass; notification #2956 regression/red-team tests 6 pass. Retained cleanup prefixes are excluded from endpoint candidates while genuinely corrupt non-artifact files remain unreadable; no new secret exposure observed.

Decision: no blocking findings; terminal review is favorable. GitHub APPROVE state cannot be emitted because the authenticated terminal account Yeachan-Heo is the PR author. Signed via terminal GitHub CLI.

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

The reviewed head is now behind current dev and GitHub cannot produce a clean merge commit. Please rebase onto current dev, resolve conflicts, and push a fresh head. The prior favorable review applies only to the reviewed SHA; it will be revalidated on the rebased exact head.


[repo owner's gaebal-gajae (clawdbot) 🦞]

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

The current head has passed exact-head re-review, but it is now conflicting with current dev. Please rebase onto current dev, resolve conflicts, and push a fresh head; the approval will be revalidated against that exact rebased SHA before merge.


[repo owner's gaebal-gajae (clawdbot) 🦞]

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

REQUEST_CHANGES — notification census exact-head review

Reviewed exact head abe22990543f72eb2bb9c88f343e88c79c150408 against the current merged dev head 57577acd7ea2da991024604e92bb209f3a30c7d5 after #3768 merged.

The contract is not superseded by merged history: current dev still does not classify retained exact-unlink/notification leak artifacts as non-endpoints, so the endpoint-health repair remains relevant. Exact-head CI for the stale PR base is green (16 pass, 6 skipped), and fresh exact-head red-team tests pass (63 tests, 0 failures).

The PR is nevertheless not merge-ready because GitHub reports it conflicting with current dev; its base is c1bf3be50e2cf0dfdf51b57eb43f7a99521c94b8, while current dev is 57577acd7ea2da991024604e92bb209f3a30c7d5. Please rebase onto current dev, resolve conflicts without broadening beyond the notification endpoint-classification contract, and rerun exact-head CI and review.

No source mutation or merge was performed. Current repair ownership remains with this PR.

Signed: Yeachan-Heo via GitHub CLI

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

GJC adversarial verdict — REQUEST_CHANGES for exact head abe22990543f72eb2bb9c88f343e88c79c150408. The focused notification change is bounded and its tests cover retained leak-artifact prefixes and corrupt-endpoint discrimination. GitHub reports the PR as CONFLICTING with current dev; rebase and rerun current-head validation before merge.

Signed: GJC / Yeachan-Heo

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

Signed GJC scope/owner handoff for the DIRTY exact head abe22990543f72eb2bb9c88f343e88c79c150408.

Current dev is aebb0f6d119acf178a9c710f460f83e9735bb8e6; the merge base is c1bf3be50e2cf0dfdf51b57eb43f7a99521c94b8, so this PR is not based on current dev. The bounded conflict set is exactly:

  • packages/coding-agent/CHANGELOG.md
  • packages/coding-agent/src/sdk/bus/notification-service.ts
  • packages/coding-agent/test/notifications-service.test.ts

Owner: Yeachan-Heo (PR author / existing notifications batch lane). Scope: rebase this PR onto current dev, preserve the leak-artifact classification behavior and current-dev changelog/tests, resolve only the three listed conflicts, then rerun the existing exact-head validation. No production scope expansion is authorized. The existing review snapshots are /tmp/gjc-review-3729 and /tmp/review-3729-exact; no duplicate mutation worktree was created by this handoff.

Signed: GJC / Yeachan-Heo

@Yeachan-Heo

Copy link
Copy Markdown
Owner Author

REQUEST_CHANGES

Signed exact-state owner disposition for PR #3729 on 2026-08-05.

Exact head / base / CI

  • PR head: abe22990543f72eb2bb9c88f343e88c79c150408
  • recorded PR base / merge base: dev at c1bf3be50e2cf0dfdf51b57eb43f7a99521c94b8
  • exact live dev: 732856b3ccb3fade6e9fbc17908a4fbca5a7682f
  • containment: diverged, 1 ahead / 66 behind
  • GitHub: open, non-draft, mergeable=false, mergeable_state=dirty, rebaseable=false
  • exact-head check runs: 22 terminal — 16 success, 6 skipped, 0 failed, 0 pending
  • successful workflows: Dev CI 30736448622; Public site sync 30736448630

That CI is valid only for the stale exact head/base. It cannot be transferred to a conflict-resolved current-dev head.

GPT-heavy hostile gate

Two independent layofflabs/gpt-5.5 xhigh, read-only leaf reviews returned VERDICT: REQUEST_CHANGES. Reconciled findings:

  1. HIGH — readable retained endpoint quarantines still classify as endpoints.
    isNotificationLeakArtifactName() is consulted only by unreadableEndpointResult() after readEndpointFile or JSON.parse fails (notification-service.ts:382-412 on this head). But endpoint exact-unlink uses .gjc-delete-notification-endpoint-${uuid}.json (notification-service.ts:185-217), and that retained quarantine can contain the detached original endpoint JSON. If it remains readable and has url/token, classification reaches the ordinary endpoint branch (notification-service.ts:414-428) and the false endpoint-health/recovery count remains. The reserved leak-artifact basename must be excluded at candidate classification before read/parse outcome determines the result.

  2. BLOCKER — the branch is not based on current dev.
    Exact virtual-merge inspection shows overlapping current-dev changes in exactly:

    • packages/coding-agent/CHANGELOG.md
    • packages/coding-agent/src/sdk/bus/notification-service.ts
    • packages/coding-agent/test/notifications-service.test.ts

    This needs a synthesized same-PR rebase, not an old-head approval or owner confirmation.

  3. MEDIUM — the tests do not prove the advertised notify health result.
    The test named keeps notify health OK... calls classifyNotificationEndpoint() directly and uses a placeholder name without .json; listEndpointFiles() would not enumerate that name. Add an observable checkNotificationHealth() regression containing a live endpoint plus a readable .gjc-delete-notification-endpoint-*.json with endpoint-shaped JSON, and assert overall level, totals, and unreadable count. Preserve a normal corrupt non-artifact .json case as unreadable.

  4. Current-dev preservation requirement.
    Conflict resolution must retain current-dev diagnostic API/test additions and the newer .gjc-delete-notification-staging-temp- entry in NOTIFICATION_LEAK_ARTIFACT_PREFIXES; the shared predicate should continue to be the source of truth.

The implementation direction remains relevant and bounded, but this head is not mergeable and misses a retained-readable quarantine class.

Contributor ledger / owner path

Required path: update this existing PR only, rebase onto exact current dev, resolve the three overlaps, fix the readable-quarantine classification, add the direct health regression, and obtain fresh exact-head CI/review. No duplicate PR or scope expansion.

No source mutation, push, merge, CI control, or release action was performed by this review.

Signed: GJC owner batch / Yeachan-Heo / 2026-08-05

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant