[llb555]

Related to #7105. This PR covers the sites that #7106, #7141 and #7164 leave out, and does not close it.

Reorganized after review at 4ed9c88. @willem-bd pointed out that extensions/manager.py::_write_bytes_atomically and app/gateway/routers/skills.py::_copy_uploaded_skill_archive were duplicated byte-for-byte in open #7164, and that two same-class siblings were still unfixed in no open PR. This revision drops the two overlapping files entirely (#7164 owns them, and it also covers the finally in the upload endpoint plus form.close()), and takes the siblings instead. No file in this diff appears in #7164.

Why

Three writers publish through a same-directory temporary and clean it up inside the handler for a failure they are already reporting. Unguarded, two distinct defects appear:

• a failing unlink replaces the error the caller sees — the real cause survives only as __context__, which the surrounding logger.warning("...: %s", exc) formats never print;
• a with os.fdopen(fd, ...) that never binds leaks fd, because the with cannot close a descriptor it never received.

uploads/companions.py::register_companion is the strongest case, and the one #7164 does not touch. Its cleanup was a bare finally: os.unlink(staged), which runs on the success path too: a failing unlink turned a completed registration into an exception, and on the failure path it replaced the original json.dump error. lark_broker.py::_write_text_atomically is the other: with os.fdopen(...) leaked on an fdopen failure, and its cleanup failure was swallowed by except OSError: pass, leaving an unobservable leftover temporary.

What changed

• integrations/lark_cli.py::_publish_bytes_atomically — fdopen outside the with, ownership transfer (fd = None), cleanup through utils.atomic_write.cleanup_failed_atomic_write.
• integrations/lark_broker.py::_write_text_atomically — same, replacing except OSError: pass so a leftover temporary is reported instead of silently dropped.
• uploads/companions.py::register_companion — same, replacing the unguarded finally.
• uploads/AGENTS.md records why the bare finally must not come back.

Successful publications are unchanged: same bytes, same modes, same rename. Only the failure path differs. utils/atomic_write.py is untouched.

Surface area

• Frontend UI
• Backend API — no endpoint, SSE event, or request/response shape moves
• Agents / LangGraph
• Sandbox
• Skills — no file under skills/
• Dependencies
• Default behavior change — a failed publication reports its own error instead of the cleanup's; a fdopen failure no longer leaks the descriptor; a failed companion registration now raises the original error instead of the unlink's
• Docs / tests / CI only

Bug fix verification

Test path: backend/tests/test_atomic_cleanup_preserves_primary_error.py (9 cases, no stubbing of the production cleanup helper).

• Red on main, green on this branch: yes. With the three production files reverted to eb7c342 and the tests kept, the file gives 5 failed, 4 passed; at this head, 9 passed. The five failures are exactly the two lark_cli cases, the two lark_broker cases and the companions case. The four that pass on main are deliberate controls: three success paths plus the "success stays quiet" case.
• Failure modes are injected at real seams — os.replace, os.fchmod/os.chmod, os.fdopen and Path.unlink — not by asserting on handler source.
• Each surviving-temporary case asserts the WARNING names the leftover path, so a leak is observable rather than becoming the caller's error. The lark_broker case is the one that used to be silent.

Validation

cd backend
PYTHONPATH=packages/harness:. python -m pytest \
  tests/test_atomic_cleanup_preserves_primary_error.py -q
#   9 passed

PYTHONPATH=packages/harness:. python -m pytest \
  tests/test_upload_companions.py tests/test_lark_broker.py \
  tests/test_lark_cli_atomic_publish.py tests/test_lark_shim_atomic.py \
  tests/test_lark_skill_manifest_atomic.py \
  tests/test_atomic_cleanup_preserves_primary_error.py -q
#   47 passed, 28 skipped

ruff check <changed files>          # All checks passed
ruff format --check <changed files> # 4 files already formatted

Coordination

#7164 owns extensions/manager.py and app/gateway/routers/skills.py; this PR does not touch either. Whichever merges first, neither conflicts on content. I have not rebased onto #7164 because the file sets are disjoint.

AI assistance

Tool(s) used: DeepSeek Harness (deepseek-v4.1-flash).

How you used it: I asked it to find the remaining unguarded cleanup handlers from the census #7105 published, check each against open PRs so the diff stays disjoint from #7164, and implement the shared-helper change against TDD tests. I reviewed the full diff and both the red-on-main and green-on-branch runs myself.

• I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

[CLAassistant]

All committers have signed the CLA.

[llb555]

Two scope questions while this is in review

1. lark_cli.py's download path. #7106 landed the three app/channels
publishers and #7141 took projects/documents.py. After those, the only site
from #7105's census still unfixed on main@eb7c342 is
integrations/lark_cli.py:2853-2857: both handlers call
archive_path.unlink(missing_ok=True) unguarded, so a failing unlink replaces
the download error, and the second handler also collapses every cause into the
same fixed ValueError.

Is that path in scope for the same contract, or a deliberate exception? It is a
downloaded archive rather than a credential or a published artifact, so leaving
the collapse-to-one-message behaviour and only guarding the unlink would also be
a defensible answer.

2. Any other site you would rather see first. Re-running the AST pass for
"delete a file path, then re-raise" over backend/**/*.py now returns only the
two lark_cli lines above plus runtime/events/store/jsonl.py:356, which
already wraps each path's rollback in its own try and so is not a real gap.
Every other hit the original census listed is now compliant.

So this contract looks finished apart from the lark_cli download path. I would
rather ask than guess: is that path in scope, and if so would you prefer it as
its own PR or folded into this one while it is still open?

If the answer is "this contract is done after #7200", that is a fine answer and
I will stop here.

No code is proposed in this comment, and nothing here blocks the current diff.

[willem-bd]

Verified at head 4ed9c88: the mechanism is right at all three sites — the shared utils.atomic_write.cleanup_failed_atomic_write (already on main) keeps cleanup best-effort and log-only, the bare raise re-raises the original error, and the fdopen-outside-with ownership transfer closes the descriptor-leak gap. I reproduced the PR's red/green claim exactly (5 failed / 3 passed with the three production files reverted to eb7c342; 8 passed at head), and the new tests inject failures at real seams (os.replace/os.fdopen/Path.unlink), not handler source. Scope is tight (4 files). Two things to sort out before merge: (1) this duplicates two of open #7164's sites byte-for-byte (manager.py::_write_bytes_atomically, skills.py::_copy_uploaded_skill_archive — #7164 additionally covers the skill-storage writers, this PR additionally covers lark_cli.py), and neither PR references the other, so whichever lands second will conflict; (2) two same-class siblings are still unfixed on main at this head and appear in no open PR — most importantly uploads/companions.py, which has the full bug class this PR fixes, including the error-replacement half.

[willem-bd]

[coordination] This exact change to _write_bytes_atomically (fdopen outside the with, fd = None, cleanup_failed_atomic_write(temporary, fd)) is also pending, byte-for-byte, in open PR #7164 — which likewise rewrites app/gateway/routers/skills.py::_copy_uploaded_skill_archive identically to this PR. The two PRs otherwise partition the remaining sites (#7164 also takes the skill-storage writers; this one uniquely takes lark_cli.py::_publish_bytes_atomically), but neither references the other, despite this PR's body saying each site was verified "free of an open PR". Worth coordinating before merge: land one and rebase the other, or split so each owns disjoint sites — otherwise the second merge is a guaranteed conflict on both files.

[willem-bd]

[important] Same class, still unfixed on main at this head, in no open PR: integrations/lark_broker.py::_write_text_atomically (~line 476) still binds with os.fdopen(fd, "w", ...), so an fdopen failure there leaks the descriptor exactly as described in the PR body — and its cleanup failure is swallowed by a bare except OSError: pass with no log, so the leftover temporary is unobservable. Worse, uploads/companions.py::_write_companion_record (lines 42-55) has the full class including the error-replacement half: an unguarded finally: os.unlink(staged) after os.replace means an unlink failure there replaces the caller's error. If these fell outside the #7106 census, they are worth a follow-up PR (or extending this one) now that the ownership-transfer pattern is established right here.

[willem-bd]

[suggestion] Same file, same class, a few blocks away: the upload endpoint's finally (line 585 at head) discards the copied archive with an unguarded await asyncio.to_thread(temporary_path.unlink, missing_ok=True). An unlink failure there replaces the endpoint's outcome — a successful 200 becomes a 500, or an in-flight 413 is masked. The helper's own docstring covers "discard staging after a publication decision", so this site fits it; it is outside this PR's stated census, but it is cheap to fold in here or record as a follow-up.

[willem-bd]

[nit] Unlike _is_temporary_of above, this predicate matches by name at any directory depth, so during the patched window any concurrent Path.unlink of a deerflow-skill-* path anywhere would also raise. Scoping it with path.parent == temp_root (as the descriptor sites do) removes the cross-suite coupling.

[llb555]

Thanks — all four points addressed at bc0652c.

1. Coordination with #7164 (the blocking one). You were right, and the overlap was larger than my body claimed: #7164 also covers the finally in the upload endpoint and guards form.close(), which is a better shape than what I had. Rather than split, I dropped both overlapping files entirely — this diff no longer touches extensions/manager.py or app/gateway/routers/skills.py, so the two PRs are disjoint and neither waits on the other. I did not rebase onto #7164 because there is nothing left to conflict on.

2. lark_broker.py::_write_text_atomically — fixed in this PR. fdopen now happens outside the with with fd = None ownership transfer, and except OSError: pass is gone; the leftover temporary is now reported by cleanup_failed_atomic_write instead of vanishing. Pinned by test_lark_broker_launcher_closes_the_descriptor_when_fdopen_fails (the descriptor is still open after the failure on main) and test_lark_broker_launcher_preserves_primary_error_when_unlink_fails (the diagnostic now names the leftover).

3. uploads/companions.py::register_companion — fixed in this PR, and I think this is the worst of the set, worse than the body of my original PR suggested. The cleanup was a bare finally:, so it ran on the success path too: a failing unlink turned a completed registration into an exception even though os.replace had already published the record. On the failure path it replaced the json.dump error. Both halves are now covered (test_companion_registration_preserves_error_when_staged_unlink_fails, and a success case asserting it stays quiet). uploads/AGENTS.md records why the finally must not come back.

4. The nit. test_skill_archive_copy_* is gone with the file it tested, so that predicate no longer exists. The remaining temporary predicates are all scoped by path.parent == <target parent>, as you suggested.

Red/green at bc0652c, same method you reproduced before: with the three production files reverted to eb7c342 and the tests kept, 5 failed / 4 passed; at head 9 passed. Affected suites: 47 passed, 28 skipped.

One thing I should be explicit about, since it is the reason this is a smaller diff than 4ed9c88: the two sites I dropped are genuinely covered by #7164, so the valuable remaining work here is companions.py (success-path masking) and lark_broker.py (silent leftover) — both of which are outside that PR. If you would rather #7164 absorb these too and have me close this, that is also fine; I only kept them apart because they are in files #7164 does not open.

[llb555]

CI note: the Layer 2 failure was the runner image, not this diff

Replay E2E (front-back contract) → Layer 2 — full-stack render failed on bc0652c in the browser install step, before a single test ran:

npx playwright install chromium --with-deps
BEWARE: your OS is not officially supported by Playwright; installing dependencies for ubuntu26.04-x64 as a fallback.
Cannot install dependencies for ubuntu26.04-x64 with Playwright 1.59.1!
Error: ERROR: Playwright does not support chromium on ubuntu26.04-x64
##[error]Process completed with exit code 1.

That is the ubuntu-latest migration to Ubuntu 26.04 arriving under a pinned @playwright/test@1.59.1. Everything else on that head was green — backend-unit-tests (1)–(4), lint-backend, backend-blocking-io, frontend-unit-tests, and Layer 1 — and none of this PR touches frontend/.

It is already fixed upstream: #7219 moved @playwright/test to ^1.62.1. I rebased onto de7307e, so the head is now ae30b86 and the job will install a Playwright that supports the new image.

The code diff is unchanged by the rebase: same 5 files, +327/−14, and the 9 regression tests pass again at the rebased head (red/green unaffected). The new workflow runs sit at action_required, so this needs one more approval click — sorry for that, but the previous head could not have gone green.

[llb555]

Thanks — status on the four points at head ae30b86:

1. [coordination] fix(extensions): keep atomic-write cleanup from replacing the save error #7164 is still open, so this PR continues to leave extensions/manager.py to it; manager.py:110 is untouched here.
2. [important] Both remaining sites are now covered: lark_broker.py::_write_text_atomically moves os.fdopen outside the with and routes cleanup through cleanup_failed_atomic_write (line 480/489), same for uploads/companions.py::_write_companion_record (45/59).
3. [suggestion] skills.py:583 is still unguarded — I left it out because it's outside this PR's census, as you noted. Do you want it folded in here, or filed as a separate change? Happy either way, just don't want to widen the diff without a decision.
4. [nit] The predicate is scoped now: _is_temporary_of requires path.parent == target.parent, and _staged_predicate is limited to upload-companions.

CI is green on this head (3 checks, combined status success). Could you take another look when you get a chance?

[willem-bd]

Follow-up at head ae30b86 (prior review at 4ed9c88, 4 inline threads). Status of the four prior findings at this head — all four addressed:

1. [coordination] FIXED. The diff no longer touches extensions/manager.py or app/gateway/routers/skills.py; I verified open #7164 owns exactly those (manager.py +14, skills.py +14), so the two PRs are now genuinely disjoint and neither blocks the other.
2. [important] FIXED. Both census siblings this review flagged are now in the PR: lark_broker.py::_write_text_atomically (fdopen outside the with, fd = None transfer, cleanup_failed_atomic_write at :489 — the silent except OSError: pass is gone) and uploads/companions.py::register_companion (same pattern, cleanup at :59), each pinned by an fdopen-failure test, an unlink-failure test, and a quiet-success test.
3. [suggestion] STILL OPEN on main, correctly out of this diff. skills.py:583-584 is unchanged here and belongs to #7164 like the rest of that file. Answering your thread question: keep it in #7164 — that PR already covers both the finally-unlink and form.close() there — and don't widen this one.
4. [nit] FIXED. _is_temporary_of now requires path.parent == target.parent and _staged_predicate scopes to upload-companions.

Three new inline findings at this head: the lark_cli download path (the last unguarded census site, in a file this PR already fixes — answering your scope question inline), one factually overstated clause in the new uploads/AGENTS.md paragraph, and a likely .startswith/.endswith typo in the new success test. The merge-order concern from the first review is fully resolved; the diff is in good shape.

[willem-bd]

[scope — answering your question in the issue thread] This file still carries the last unguarded site from the census: _download_lark_archive (lines 2854-2859 at this head), where both handlers call archive_path.unlink(missing_ok=True) bare, so a failing unlink replaces the download error — the "too large" ValueError in the first handler, the network ValueError in the second. Since this PR already rewrites _publish_bytes_atomically in this same file and imports the helper, folding it in is cheap: make each cleanup best-effort (cleanup_failed_atomic_write(archive_path), or a try/except OSError that logs) and keep the raise bare. Guarding only the unlink while leaving the second handler's collapse-to-one-message as is, is a defensible split exactly as you suggested. If you would rather keep this PR frozen, close the question in the thread by filing the follow-up — it should not remain undecided, since it is the only site left from the #7105 census.

[willem-bd]

[nit — factual] This clause overstates the old bug: the old finally was gated by if os.path.exists(staged), and after a successful os.replace the staged path no longer exists, so the unlink never ran on the success path — a failing unlink could not turn a completed registration into an exception. The genuine defects were the failure-path replacement (an unlink failure after a json.dump or os.replace error replaced the primary error) and the fdopen leak. The "do not restore the finally" guidance itself is right; the recorded reason should be the failure-path one, otherwise a future reader who checks the history will discount the whole note. The new test module's docstring repeats the same overstatement ("even on the success path").

[willem-bd]

[nit] p.name.startswith(".json") looks like a typo for endswith: records are named {sha256}.json and staged files are .companion-*, so the first list is always empty and the assertion lives entirely on the glob("*.json") fallback. It still passes, but as written it reads as a check that can never fire.

[llb555]

All three addressed at d4402c3 (the branch was rebuilt on ae30b86, so the a3ccb88 head you reviewed no longer resolves; I re-applied the same three edits on top of the current commit).

1. lark_cli download path — folded in rather than filed, since this PR already imports the helper in that file. Both handlers in _download_lark_archive now call cleanup_failed_atomic_write(archive_path) and keep the bare raise, so the "too large" ValueError and the network ValueError reach the caller. Two tests in test_atomic_cleanup_preserves_primary_error.py fail before the change (the PermissionError escapes instead) and pass after. I left the second handler's collapse-to-one-message alone, as you suggested.
2. uploads/AGENTS.md — corrected to the failure-path reason: after a successful os.replace the staged name was already gone, so the unguarded finally only ever ran on the failure path, where its own error replaced the json.dump/os.replace one; the other defect was the with os.fdopen(...) descriptor leak. The new test module's docstring repeated the same overstatement and is fixed with it.
3. startswith(".json") — typo fixed to endswith, so the first assertion now actually fires. The glob("*.json") fallback is redundant now; say the word and I'll drop it.

skills.py:583-584 stays with #7164, as you confirmed.

Validation: pytest tests/test_atomic_cleanup_preserves_primary_error.py tests/test_lark_cli_atomic_publish.py tests/test_upload_companions.py → 24 passed; ruff check and ruff format --check clean on both touched Python files.