fix(session): fail closed when tool-output eviction artifacts are unavailable - #3969
Conversation
8184cdc to
c1038e3
Compare
c1038e3 to
b537d85
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: b537d851e9
ℹ️ 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".
| // ephemeral store (or a visible install failure) instead of silently pruning | ||
| // without durable eviction. Fail closed when tool-output eviction is planned | ||
| // but no artifact store can be established. | ||
| const artifactManager = await this.sessionManager.ensureArtifactManager(); |
There was a problem hiding this comment.
This changes the user-visible mid-run maintenance outcome and artifact-preservation behavior in packages/coding-agent, but the commit adds no entry under packages/coding-agent/CHANGELOG.md → Unreleased, so the fix will be absent from the package's release notes. Add a concise Fixed entry for the fail-closed behavior.
AGENTS.md reference: AGENTS.md:L178-L178
Useful? React with 👍 / 👎.
ee5daf5 to
1e1aab1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e1aab10cb
ℹ️ 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".
| if (!artifactManager && pruneEstimate.prunableCount > 0) { | ||
| return undefined; |
There was a problem hiding this comment.
When a session already has an ArtifactManager but its storage is unavailable—for example, the artifact directory becomes read-only, the disk is full, or a managed-store operation fails—this guard is bypassed. The following allocatePath() catch (and the later allocateId() catch) merely disables artifact references, after which pruneToolOutputs still commits truncation notices and rewrites the canonical entries, permanently discarding the original output without an artifact. Treat reservation/allocation failure like a missing manager and abort the prune before committing any mutations.
Useful? React with 👍 / 👎.
…vailable Port the midrun fail-closed contract onto current dev's prune path: use ensureArtifactManager (so ephemeral install failures are visible) and return undefined from #pruneToolOutputs when tool-output eviction is planned but no artifact store can be established. Prevents reporting a successful prune that skipped durable eviction. Regression: force ephemeral install failure via SessionManagerTestHooks and assert mid-run maintenance outcomes "failed" with original tool text retained. Lore-id: midrun-fc1-devbase Confidence: high Scope-risk: narrow Reversibility: reversible Tested: not run locally (node-pty unavailable); exact-head CI will verify Not-tested: full coding-agent shard matrix offline
Drop the extra blank line so check:@gajae-code/coding-agent passes.
1e1aab1 to
20d8125
Compare
… contention Root cause: the first Bun probe child absorbs cold compile of the probe + env module graph. Under coding-agent shard contention that cold start can exceed the 60s per-test budget (observed 60001ms on Yeachan-Heo#3969 exact-head) while the four sibling cases complete in ~300ms once the module graph is warm. Fix (harness isolation, assertions unchanged): - beforeAll warmup spawn (120s suite budget) moves cold-start outside it() - resolveIn kills stalled children at 45s with a diagnostic instead of leaking pipes until the outer it() timeout Not a third blind per-test timeout bump; the 60s it() budgets stay. Lore-id: smithery-spawn-isolation-1 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: not fully offline (local natives missing); exact-head CI verifies Not-tested: full parallel shard matrix offline
1 participant
Problem
#3846 CI shard-7 fails:
AgentSession mid-run maintenance outcomes > fails closed when tool-output artifact persistence is unavailablefailedprunedCause
Non-persisted sessions have no durable session-bound artifact store (
getArtifactManager()is null). Mid-run#pruneToolOutputsstill committed partial non-tool prunes and reported success, so maintenance returned"pruned"instead of fail-closing.(Prior #3969 draft was incorrectly based on
feat/jcode-research/ a different prune implementation. This rewrite targets currentdevonly.)Fix
prunableCount > 0) but no durable artifact manager exists, return without committing."failed"and original tool content retained.Non-goals
Verification