Repository navigation
fix(firmware): recover after workspace removal failures - #1616
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Maintainer self-review of 90ce2448: cleanup owners are retained independently; filesystem-only failures allow disjoint work after verified process stop, while unverified ownership still blocks launches. Completed observations keep their producer data and original artifact identity; report and cleanup failures both survive projection. Recovery tests verify every retained workspace and preserve published files/source bytes.
Passed locally: 42 focused firmware/ownership tests, full static/architecture checks, docs/test discovery, real Binwalk 3.1.0 + Unblob 26.6.4 CLI/MCP verification, and six native Qwen/Pi/Hermes/DSH client cases. Cloud model APIs and real Hopper/Ghidra were not tested. GitHub CI is still pending.
Broader local shard reproduction also hit inherited CDP/EVM/Qwen CI failures now addressed by #1618. The old CI run was cancelled to stop its large failure-log delay; this PR will be refreshed after that fix reaches main and will not merge before fresh CI is green. Stale local catalog and temporary-directory fixture failures were checked separately.
|
The current CI shard also contains a stale unverified-worker assertion: the Your actual verified-worker/workspace recovery implementation and stronger recovery scenarios remain separate; they were not copied into #1617. The overlapping unverified-worker assertion is credited in its description. Once that test-only repair lands, please preserve this PR's recovery coverage when reconciling the test-file overlap. I have not independently repeated your real Binwalk/Unblob verification. |
…e-recovery-20261010 # Conflicts: # tests/boundary/firmware/firmwareIntegration.test.ts
|
Independent review of The three helper-only tests are replaced by stronger production-boundary workflows rather than waived. No additional provider fix was needed in this review. My validation does not establish independent real Binwalk/Unblob behavior; the contributor’s stated real-engine verification remains separate. #1618 now supplies the shared CI cleanup corrections in main, so the final source should include that base before its last CI gate. Final-source follow-up: |
N0zoM1z0
left a comment
There was a problem hiding this comment.
Maintainer self-review of 9e976d62: main/#1618 integration keeps unverified-worker admission blocked and preserves the same-provider recovery test. Filesystem-only cleanup retains every owned root without suppressing a new disjoint operation; completed results keep producer data/original target identity, and primary report failures survive cleanup failure.
Passed on this head: 45 focused firmware/ownership tests, real Binwalk3.1 + Unblob26.6.4 CLI/MCP, and six native Qwen/Pi/Hermes/DSH cases (complete170 tools; verified cleanup). Loopback model fixtures only. Fresh CI is pending; the obsolete run was cancelled.
A failed private-workspace removal must not replay an old extraction error onto a new firmware inspection after its worker has been verified stopped. Retain each cleanup owner separately, allow subsequent operations in disjoint workspaces, and retry/report every retained root at shutdown. Unverified process cleanup still blocks new launches; recovery uses the original owned-process identity.
Preserve completed extraction results together with raw producer data, artifact identity and source metadata when cleanup fails. Preserve both the primary report failure and the cleanup failure through the existing shared error projection. The existing
partial_observation.resultshape remains intact; provenance is additive.Replace three helper-only cleanup tests with real subprocess/filesystem boundaries: EACCES after two completed extractions, subsequent Binwalk work on the same provider, MCP error serialization, all-root cleanup after permission recovery, and combined report/cleanup failures. Correct the stale assertion that an unverified worker allows another launch.
Fixes #1501.
Validation: 45 focused firmware/process-ownership tests; typecheck/lint/static checks; generated documentation and test-discovery checks; real Binwalk 3.1.0 and Unblob 26.6.4 CLI/stdio MCP verification (independent offsets/digests, selected ranges, unknown chunks and depth limits). Qwen 0.25.0, Pi 1.1.0, Hermes (normal/sticky profiles) and DeepSeek Harness 0.2.0-rc.2 native flows also passed against all 170 tools; loopback model fixtures only. Linux x64, Node 24.18.0. No real Hopper/Ghidra verification; their code is unchanged. No new tools or catalog filtering.