fix(notifications): exclude retained cleanup leak artifacts from endpoint health - #3729
fix(notifications): exclude retained cleanup leak artifacts from endpoint health#3729Yeachan-Heo wants to merge 2 commits into
Conversation
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
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.
|
The reviewed head is now behind current — |
|
The current head has passed exact-head re-review, but it is now conflicting with current — |
REQUEST_CHANGES — notification census exact-head reviewReviewed exact head The contract is not superseded by merged history: current The PR is nevertheless not merge-ready because GitHub reports it conflicting with current No source mutation or merge was performed. Current repair ownership remains with this PR. Signed: Yeachan-Heo via GitHub CLI |
|
GJC adversarial verdict — REQUEST_CHANGES for exact head Signed: GJC / Yeachan-Heo |
|
Signed GJC scope/owner handoff for the DIRTY exact head Current dev is
Owner: Signed: GJC / Yeachan-Heo |
|
REQUEST_CHANGES Signed exact-state owner disposition for PR #3729 on 2026-08-05. Exact head / base / CI
That CI is valid only for the stale exact head/base. It cannot be transferred to a conflict-resolved current- GPT-heavy hostile gateTwo independent
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 No source mutation, push, merge, CI control, or release action was performed by this review. Signed: GJC owner batch / Yeachan-Heo / 2026-08-05 |
yazzang-homelab
left a comment
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review.
Correct fix, and the structural reason it is correct: isNotificationLeakArtifactName reads NOTIFICATION_LEAK_ARTIFACT_PREFIXES, the same constant the cleanup path already consumes at telegram-daemon.ts:1620. So classification and cleanup cannot drift — a prefix added for one is automatically honoured by the other. A local regex or an inline string list here would have been the obvious shortcut and would have rotted the first time a new leak prefix appeared.
Collision risk is nil: all four prefixes are dot-leading (.gjc-delete-daemon-transition-, .gjc-exact-unlink-placeholder-, .gjc-delete-notification-endpoint-, .gjc-delete-notification-staging-temp-) while real endpoints are UUID-named .json files. Widening non-endpoint cannot swallow a legitimate endpoint by accident.
The test that earns its place is still reports a genuinely corrupt endpoint file as unreadable — {not json under a UUID name still classifies unreadable. Without it, this diff is indistinguishable from "stop reporting unreadable files", which would hide the exact condition notify health exists to surface. Pairing the widening with a negative control is the right instinct, and the third case (leak artifact sitting beside a live endpoint, both classified correctly in the same directory) covers the interaction rather than each name in isolation.
The failure being fixed is a good one to have caught: zero-byte-by-construction artifacts counted as unreadable endpoints pinned notify health to a WARN that notify recovery could never clear — a permanently stuck warning trains users to ignore the indicator, which is worse than no indicator.
gajae.pr-review-verdict.v1 merge-approved sha256:abe22990543f72eb2bb9c88f343e88c79c150408 reviewer:architect evidence:read of telegram-daemon.ts:1318-1323,1408-1410,1620 and the three added classification cases at this head
…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.
abe2299 to
a820681
Compare
![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: a820681c90
ℹ️ 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".
| @@ -1,1717 +1 @@ | |||
| # Changelog | |||
|
|
|||
There was a problem hiding this comment.
This change replaces the entire 1,716-line changelog with a blank line, deleting both the new notification entry and every released section. Besides losing the package's release history, scripts/release.ts will skip this canonical changelog because it no longer contains ## [Unreleased], so subsequent releases cannot add their version heading here. Restore the prior content and add this fix only under the existing Unreleased section.
AGENTS.md reference: AGENTS.md:L178-L178
Useful? React with 👍 / 👎.
경고 — 이 PR의 현재 head가 CHANGELOG 전체를 삭제한다머지하면 안 된다. 확인된 사실: 1바이트 — 개행 하나만 남았다. 원인은 내 쪽이다#3932(11:25:32Z 머지)가 그런데 그 결과 리베이스에서 CHANGELOG가 처음으로 진짜 충돌을 내기 시작했고, 그 충돌을 해소하는 과정에서 파일이 비워졌다. 시간대가 명확하다:
전환 비용을 예고하지 못한 건 내 잘못이다. 미안하다. 복구git fetch origin
git checkout origin/dev -- packages/coding-agent/CHANGELOG.md # 해당 패키지 경로로
# 그 다음 ## [Unreleased] 아래에 이 PR의 항목만 다시 추가
git add packages/coding-agent/CHANGELOG.md
git commit --amend --no-edit # 또는 새 커밋앞으로 리베이스에서 CHANGELOG 충돌이 나면 양쪽 항목을 모두 푸시 전에 다음으로 자가 점검할 수 있다: git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # 30만 바이트 근처여야 정상 |
yazzang-homelab
left a comment
yazzang-homelab
left a comment
There was a problem hiding this comment.
앞선 내 승인을 철회한다. 이 head는 머지하면 안 된다.
내가 승인한 시점 이후 head가 force-push됐고, 새 head에서 CHANGELOG가 파괴됐다. 승인은 그 이전 커밋에 대한 것이었으므로 현재 head에는 유효하지 않다.
코드 리뷰 내용 자체는 그대로 유효하다 — 로직에 대한 판단은 바뀌지 않았다. 되돌리는 것은 머지 가능 판정뿐이다.
원인은 내가 머지한 #3932다(union 드라이버 제거). 상세와 복구 명령은 이 PR에 이미 남긴 코멘트와 #3942 에 있다. CHANGELOG를 origin/dev에서 복원하고 이 PR의 항목만 다시 넣은 뒤 푸시하면, 그 head에 대해 즉시 재승인하겠다.
#3941(CI 가드)이 머지되면 이 상태는 CI에서 자동으로 걸린다.
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
Removing `packages/*/CHANGELOG.md merge=union` in #3932 was correct -- union never conflicts, it concatenates both sides of an overlapping hunk, which silently filed entries into versions that had already shipped (35 such entries audited on dev, #3929). What it did not account for is the transition: these files now conflict on rebase for the first time, and a bad resolution drops the whole history with no marker. That is not hypothetical. #3932 merged at 11:25:32Z. Between 11:29:29Z and 11:35:02Z, ten open pull requests across six authors force-pushed heads whose CHANGELOG was a single newline -- every released section gone. #3920 #3697 #3870 #3908 #3887 #3864 #3729 #3869 #3866 #3873. Nothing caught it: the files still parse, no test reads them, and the loss looks like a large deletion inside an otherwise legitimate diff. The guard asserts the one property that matters and nothing more: every `## [X.Y.Z]` heading present at the merge base must still be present at the head. Additions pass, rewording passes, and a release commit that consumes `## [Unreleased]` into a new version passes. Only losing a released section fails, and the message names the recovery command. Runs in `affected-plan`, which already checks out full history and carries the immutable event base sha, so it costs one bun invocation and needs no new job. Constraint: a release bump must still be able to add a version heading Constraint: must not depend on byte-size heuristics -- a legitimately small changelog is not a violation Rejected: threshold on deleted line count | fires on large legitimate edits and misses a small changelog emptied completely Rejected: restore merge=union | reinstates the silent misfiling this replaced, and GitHub ignores the driver anyway Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test scripts/changelog-history-guard.test.ts (11 pass); guard run against the three real broken heads (#3873 #3920 #3869) exits 1 and names the lost sections; clean range exits 0; bun run check:tools exit 0 Not-tested: a real release-bump PR end to end
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edd2410615
ℹ️ 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".
| // Retained native cleanup leak artifacts are not endpoint candidates. They are | ||
| // unparseable by construction (often zero-byte), so counting them as unreadable | ||
| // pins `notify health` to a WARN that `notify recovery` can never clear. | ||
| if (isCanonicalLifecycleArtifactName(name) || isNotificationLeakArtifactName(name)) return { kind: "non-endpoint" }; |
There was a problem hiding this comment.
When exact-unlink detaches an endpoint but scrub_regular_file_openat fails, the retained .gjc-delete-notification-endpoint-*.json quarantine still contains the original valid url/token endpoint JSON. Because the new name predicate is consulted only by unreadableEndpointResult, that payload parses successfully and is still classified as an endpoint, so health and recovery continue counting or processing the artifact this change intends to ignore. Apply the leak-artifact name check before reading/parsing the file.
Useful? React with 👍 / 👎.
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
2 participants
What
notify healthcounted retained native cleanup leak artifacts as endpoint files, pinning it to a WARN thatnotify recoverycould never clear.classifyNotificationEndpointroutes every unparseable file in the endpoint directory tounreadableunless 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
devbuild for release prep. On a real machine:The directory actually held 3 endpoints plus 10
.gjc-exact-unlink-placeholder-*files. Running the suggested recovery only flipped the wording and never converged:Why it could never clear
The retention is deliberate, so recovery is not at fault:
crates/pi-natives/src/path_identity.rs:2956returnsRetainedFailure(name, "cleanup_pending")even when the placeholder identity matches — there is nounlinkaton that path.ExchangePlaceholderRemoval::Removedis matched in 5 places and constructed in none (#[allow(dead_code, reason = "retained cleanup outcomes are platform-conditional")]).telegram-daemon.ts:1569reapNotificationLeakArtifactexplicitly 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
isNotificationLeakArtifactNamepredicate inunreadableEndpointResultso all fourNOTIFICATION_LEAK_ARTIFACT_PREFIXESclassify asnon-endpoint— a kind the scanner already skips before counting (notification-service.ts:801). Genuinely corrupt endpoint files still classify asunreadable.Classification only. The retention protocol in
path_identity.rsis 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
[warn] 1 dead / 10 unreadable of 13 endpoint file(s); run recovery[warn] 1 dead / 0 unreadable of 3 endpoint file(s); run recoverynotify recovery[ok] 2 live, 0 unverifiednotify recovery[ok] 2 live, 0 unverified(converges)Total is corrected from a phantom 13 to the real 3. The remaining
1 deadwas 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/75srcchange (verified by stashing it: 2 fail)bun run check:typesinpackages/coding-agent— cleanbun x biome checkon both touched files — cleanChangelog entry added under
### Fixed.