Skip to content

fix(session): report an over-budget managed scope as capacity_exceeded - #4185

Open
probepark wants to merge 2 commits into
Yeachan-Heo:devfrom
probepark:fix/managed-scope-capacity-misclassification
Open

fix(session): report an over-budget managed scope as capacity_exceeded#4185
probepark wants to merge 2 commits into
Yeachan-Heo:devfrom
probepark:fix/managed-scope-capacity-misclassification

Conversation

@probepark

Copy link
Copy Markdown
Collaborator

Fixes the diagnosability half of #4184.

The message pointed at the wrong file

gjc aborts at startup in a long-lived working directory:

Could not prepare managed session scope (binding_invalid: prepare:binding_publish).

The binding is byte-for-byte canonical. The real error is managed_replace_cleanup_receipt_limit_exceeded, thrown at managed-session-storage.ts:1249-1250 when #reconcileReplacementCleanupReceipts scans past REPLACEMENT_CLEANUP_RECEIPT_SCAN_LIMIT (50,000). So the error sent operators to delete a healthy binding instead of pruning the directory.

Confirmed by instrumenting the catch at managed-session-scope.ts:1168:

[P4] stage=binding_publish msg=managed_replace_cleanup_receipt_limit_exceeded ctor=Error

Two helpers had to agree

code comes from managedScopeErrorCode; the string the operator actually sees, cause.classification, comes from managedScopeFailureCause. Only the former knew about capacity messages — so fixing just that path still printed binding_invalid. That is why this took three attempts to land. Both now share one predicate.

Verification

run result
managed-scope-capacity-classification.test.ts 1 pass / 0 fail (+3 Linux-gated skips)
mutation — revert managedScopeFailureCause to always binding_invalid 0 pass / 1 fail
mutation — narrow the managedScopeErrorCode predicate 0 pass / 1 fail
restore 1 pass / 0 fail
neighbour managed-scope-owner-only-self-heal.test.ts 2 pass / 1 skip / 0 fail
check:types exit 0
end-to-end, rebuilt binary binding_invalidcapacity_exceeded

Both production paths are independently pinned — reverting either one alone fails the test.

A correction to my own first attempt

My initial test mocked native.openRecoveryFsRoot. That was theatre: openRecoveryFsRoot is Linux-only (managed-session-storage.ts:896) and never raises this error. The test also sat inside a describe.skipIf(platform !== "linux"), so it reported 0 pass / 4 skip on my machine — it would have passed even with the real path broken.

Rewritten to inject at fs.opendirSync, which is what the reconcile scan actually walks, and moved into a cross-platform describe so it runs everywhere. Recording this because a green-but-inert test is worse than no test.

Safety review

  • No security downgrade. Symlinks, hard links, oversize files and depth violations throw unsafe_artifacts (managed-session-storage.ts:3118-3135) — a different string, untouched here. Only the two capacity messages are reclassified.
  • managedSecurityFailureClassification is still checked first and returns early, so a security classification always wins over the capacity predicate.
  • No recovery path is disabled. The only binding_invalid consumer (managed-session-scope.ts:740) matches on the thrown message, not on this classification.
  • Genuinely unrecognized failures still classify as binding_invalid — covered by the existing test in the same file.

Deliberately not fixed here

The unbounded growth that causes this. The affected scope held 52,796 entries — 26,386 orphaned placeholders and 13,193 cleanup receipts against ~2 real transcripts. Raising the limit would only delay the crash, and pruning during startup is the wrong place for an unbounded delete. remove_exchange_placeholder (crates/pi-natives/src/path_identity.rs:3096-3127) never returns Removed on the identity-match path, which needs its own decision. Tracked in #4184.

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

OWNER_CONFIRMATION_REQUIRED

The classification change at this exact head is a sound partial fix: the managed receipt-scan capacity failure is mapped consistently to capacity_exceeded while preserving the real managed_replace_cleanup_receipt_limit_exceeded message. It does not, however, resolve the accepted growth half of #4184.

The attempted generic native cleanup cannot ship safely. POSIX has no descriptor-bound unlink. A fstatat(..., AT_SYMLINK_NOFOLLOW) identity check followed by unlinkat leaves a pathname race: a same-UID concurrent writer can replace .gjc-exact-unlink-placeholder-<dev>-<ino> after validation and before unlink, causing cleanup to delete the replacement. Linux renameat2 exchange/noreplace and macOS renameatx_np swap/excl can isolate names, but the final deletion is still pathname-based and recreates the same race. The current fail-closed retained/cleanup_pending behavior is therefore the safe generic contract.

Owner direction is required for the bounded-growth boundary before this issue can be completed in one coherent PR:

  1. Declare a cooperative scope-global lease/fence that covers every placeholder/receipt producer, then permit fixed-budget, lease-aged, no-follow, identity/evidence-bound collection only inside that fenced owner-only scope; ambiguous or noncooperative entries still fail with capacity.
  2. Introduce a distinct private/privileged namespace coordinator that can provide stronger deletion authority.
  3. Explicitly retain the current fail-closed capacity failure and scope #4185 to classification only, acknowledging that #4184's growth remediation remains blocked.

No unsafe unlink/GC experiment was committed or pushed, and the contributor branch has not been mutated.


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

@probepark
probepark force-pushed the fix/managed-scope-capacity-misclassification branch from 1cffb7a to 9afad3d Compare August 11, 2026 01:30
@probepark

Copy link
Copy Markdown
Collaborator Author

Owner direction: option 3

Confirming the third option. Retain the current fail-closed capacity failure, and scope this PR to the classification fix only. #4184's growth remediation stays blocked pending a separate design.

Your safety analysis is accepted as the reason, not worked around: POSIX has no descriptor-bound unlink, an fstatat(..., AT_SYMLINK_NOFOLLOW) identity check followed by unlinkat leaves the pathname race open for a same-UID concurrent writer, and while renameat2 exchange/noreplace on Linux and renameatx_np swap/excl on macOS can isolate names, the final deletion is still pathname-based and recreates the same race. A generic native cleanup therefore does not ship here.

Options 1 and 2 are both new design work — a cooperative scope-global lease/fence covering every placeholder/receipt producer, or a privileged namespace coordinator with stronger deletion authority. Neither belongs in this PR, and neither should be prototyped on this branch.

No unsafe unlink or GC experiment has been committed or pushed on this branch. Head is unchanged at 9afad3d42.

What this leaves in scope

The PR body already draws this boundary — the two capacity messages are reclassified and nothing else:

  • managedScopeErrorCode and managedScopeFailureCause now agree, so the operator-visible string and the code stop disagreeing. That mismatch is what made this take a while to pin down.
  • managedSecurityFailureClassification is still checked first and returns early, so a security classification always wins over the capacity predicate.
  • No recovery path is disabled — the only binding_invalid consumer (managed-session-scope.ts:740) matches on the thrown message, not on this classification.
  • Genuinely unrecognized failures still classify as binding_invalid, covered by the existing test in the same file.

The unbounded growth that triggers the crash is deliberately not fixed here, as stated in the PR body: the affected scope held 52,796 entries — 26,386 orphaned placeholders and 13,193 cleanup receipts against ~2 real transcripts. Raising the limit only delays the crash, and startup is the wrong place for an unbounded delete. remove_exchange_placeholder (crates/pi-natives/src/path_identity.rs:3096-3127) never returning Removed on the identity-match path needs its own decision.

I will record the blocked status and this rationale on #4184 so the growth half is not silently dropped.

Exact-head CI at 9afad3d42: 19 successful, 5 skipped, 0 failed.

Re-review requested against the narrowed scope.

A managed scope directory that outgrows the replacement-cleanup receipt
scan limit throws `managed_replace_cleanup_receipt_limit_exceeded`. The
scope resolver did not recognize that message, so it collapsed it to
`binding_invalid` and the CLI aborted at startup with:

  Could not prepare managed session scope (binding_invalid: prepare:binding_publish).

The binding is byte-for-byte canonical in this state -- only the
surrounding entry count is over budget -- so the error sent operators to
delete a healthy binding file instead of pruning accumulated receipts.

`cause.classification` is produced by managedScopeFailureCause while
`code` comes from managedScopeErrorCode. Only the latter knew about
capacity messages, so even a corrected code still printed
`binding_invalid` to the operator. Both now share one predicate.

Lore-id: capfix9a2
Constraint: keep binding_invalid for genuinely unrecognized failures -- only known capacity messages are reclassified
Rejected: raise the receipt scan limit | hides unbounded receipt growth instead of naming it
Rejected: prune receipts during prepare | startup is the wrong place for an unbounded delete
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: reproduced from a clean cwd by filling a scope past the limit; error goes binding_invalid -> capacity_exceeded with the real message, verified through the compiled binary
Not-tested: why the receipts accumulate unbounded in the first place -- filed separately
@probepark
probepark force-pushed the fix/managed-scope-capacity-misclassification branch from 9afad3d to 74fe4fb Compare August 11, 2026 05:50

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Signed exact-head verdict: MERGE_READY

Reviewed commit: 9afad3d4221d533c2ecd308ef2d8aa580ba38f75
Base: dev at e8081462a5e6befb1ec6f4b805dcb787136d85aa

The frozen diff changes exactly two paths:

  • packages/coding-agent/src/session/internal/managed-session-scope.ts
  • packages/coding-agent/test/managed-scope-capacity-classification.test.ts

Adversarial review confirms:

  • Receipt-scan sentinels (artifact_capacity_exceeded and managed_replace_cleanup_receipt_limit_exceeded) yield capacity_exceeded both as result.code and operator-visible cause.classification.
  • managedScopeFailureCause evaluates managedSecurityFailureClassification first, preserving security precedence.
  • Unknown Error messages and non-Error failures still fall through to binding_invalid.
  • The exact two-path diff contains no unlink, garbage-collection, deletion, pruning, or recovery behavior. git diff --check is clean.
  • Exact-head GitHub checks are terminal and non-failing (0 incomplete/failing check runs).

The local focused test command could not execute in the detached review worktree because this checkout has no installed @gajae-code/natives workspace dependency; this is an environment limitation, not a test failure. Exact-head CI is green.

Verdict: MERGE_READY. The later recorded owner direction resolves the former OWNER_CONFIRMATION_REQUIRED: retain fail-closed capacity behavior, merge this classification-only correction, and hold bounded-growth remediation as separate design work.


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

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Signed updated-head verdict: REQUEST_CHANGES

The approved exact head 9afad3d4221d533c2ecd308ef2d8aa580ba38f75 was replaced before merge. Current head is 74fe4fbf01eb11c38b42e95c5cfb26ee7d25899a.

The rebase introduced unrelated cleanup behavior into the PR's changed source: compared with the previously reviewed head, managed-session-scope.ts adds the reapScrubbedProtocolRemnantsSync import and invokes reapScrubbedProtocolRemnantsSync(scope.directoryPath) in two paths. This violates the explicit classification-only/no unsafe unlink-or-GC scope. The PR must contain only the classification correction and its test.

Remove the unrelated reaping changes from this PR (or land them independently with their own safety design and review), then publish a new exact head for fresh review. No merge occurred and #4184 remains open.


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

The rebased PR included protocol-remnant reaping outside the approved classification scope.\n\nRemove those cleanup calls so the branch changes only failure reporting and its regression test.\n\nLore-id: 4185-scope-repair\nConstraint: retain fail-closed capacity behavior\nConstraint: do not add unlink or GC behavior\nConfidence: high\nScope-risk: narrow\nReversibility: simple-revert\nTested: git diff --check\nNot-tested: focused Bun test unavailable because workspace dependencies are absent
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.

2 participants