feat(browser): retain cleanup evidence for failed Agent Tasks - #139
feat(browser): retain cleanup evidence for failed Agent Tasks#139seonghobae wants to merge 12 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Large unrelated documentation revert in scope
Beyond the Agent Task cleanup-evidence change, the PR deletes many docs (DOCUMENTATION_FITNESS.md, ADR 0013/0014, evidence dossiers, extension-authority UML, several traceability files, six doc test files) and reverts PRD/TRD/traceability/ADR indexes to a simpler earlier state, dropping the maturity vocabulary and the ADR index-completeness rule. The PR description covers only cleanup evidence, so this scope needs confirmation as intentional rather than a rebase artifact.
Was this helpful? React with 👍 or 👎 to provide feedback.
| | [0004](0004-resolved-destination-policy.md) | Logical origin and resolved destination safety | Accepted | SSRF/rebinding/special-purpose address and redirect authority | | ||
| | [0005](0005-direct-socket-binding.md) | Exact direct TCP peer binding | Accepted | explicit socket authority and operating-system peer proof | | ||
| | [0006](0006-tls-server-identity.md) | TLS service identity over the verified peer | Accepted | WebPKI identity, roots, time, ALPN and stream binding | |
There was a problem hiding this comment.
🟡 Accepted ADRs dropped from index but files remain
The ADR index in README.md and the list in README.md now stop at ADR 0006, yet docs/adr/0007–0010 still exist on disk. These Accepted decisions become undiscoverable, and the same change removes the index-completeness rule that would have flagged them.
(Refers to this code)
Prompt for agents
The ADR files docs/adr/0007-purpose-bound-sensitive-data-authority.md, 0008-leaf-validity-horizon.md, 0009-hourly-agent-credential-boundary.md and 0010-session-context-bound-node-authority.md still exist in the tree but are no longer referenced by either canonical index (docs/adr/README.md 'Current protected-main decisions' table and docs/README.md 'Protected-main architecture decisions' list, both of which now stop at ADR 0006). Either restore index rows/links for 0007-0010 with their correct lifecycle status, or, if these ADRs are intentionally being retired, remove/supersede the files themselves so the tree and the indexes agree. Confirm this orphaning is intended given the PR's stated scope is Agent Task cleanup evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
| duration_ms = round((time.monotonic() - trial_started) * 1000) | ||
| if failure_type is not None: | ||
| return { | ||
| "trial_number": trial_number, | ||
| "passed": False, | ||
| "failure_type": failure_type, | ||
| "profile_cleaned": True, | ||
| "duration_ms": duration_ms, | ||
| } | ||
| if result is None: | ||
| raise RuntimeError("Agent Task browser pass returned no result") |
There was a problem hiding this comment.
📝 Info: Cleanup-evidence-on-failure change is consistent
Both trial wrappers catch the reviewed failure classes, verify profile removal after the TemporaryDirectory exits, and return only failure_type/profile_cleaned (no exception text). The new agent_task_profiles_cleaned/forced_close_profiles_cleaned gates use .get() over all trials, so a genuine cleanup failure (which raises and yields a dict lacking profile_cleaned) still fails closed. Evidence is emitted before the gate raises.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Partial implementation of #28, stacked on current exact PR #136 head
1cffb2e23d4002f12e8462c4c8c24f404a4eeee7.Buyer/reliability gap
The controlled Agent Task runner previously discarded temporary-profile cleanup evidence whenever either the ordinary browser pass or forced-close probe failed. Outer loops retained only trial number, failure state, and exception type, while cleanup/isolation checks filtered to successful trials. A failed or interrupted task could therefore lack credential-free teardown evidence even though #28 requires deterministic failure/recovery and no leaked task profile.
A later exact-path audit found an additional owned exceptional-condition gap: bounded ChromeDriver teardown already used
wait(timeout=5)withkill()fallback, butsubprocess.TimeoutExpiredfrom that reviewed teardown path was not among the trial wrappers' classified failures. A real teardown timeout could therefore escape the trial boundary instead of producing the same bounded failed-trial cleanup evidence.Dependency and writer alignment
This Draft targets current exact #136 head
1cffb2e23d4002f12e8462c4c8c24f404a4eeee7and must remain Draft while #136/#132 and their prerequisites remain active. Current exact #139 head is10a60f59046c1a29a6e73084804aee0203798bbb. The current base is preserved as ancestry; no force-push, destructive rebase, base mutation, or predecessor evidence transfer occurred.Test-first repair sequence
Earlier RED/GREEN cycles established ordinary and forced-close failure cleanup evidence plus separate all-trial cleanup gates. The latest resilience repair was also test-first:
0e66b24395aae595c569ad86b46e76ca3aad3a4aadded ordinary and forced-close regressions that injectsubprocess.TimeoutExpiredwith private synthetic command paths. The required result ispassed: false,failure_type: "TimeoutExpired",profile_cleaned: true, with no raw command path in returned evidence.32443705971, Rust-contracts job96659262887, checked out that exact RED head and failed at exactly those two intended Python boundaries becauseTimeoutExpiredescaped instead of becoming bounded trial evidence.10a60f59046c1a29a6e73084804aee0203798bbbadds onlysubprocess.TimeoutExpiredto the reviewed failure classes in_run_agent_task_trialand_run_agent_task_forced_close_trial.Implemented contract
Both Agent Task trial wrappers now:
OSError,ValueError,RuntimeError,json.JSONDecodeError,subprocess.TimeoutExpired) inside the temporary-profile lifecycle;TemporaryDirectorycontext exits;passed: false, failure type,profile_cleaned: true, and duration when reviewed browser/teardown work fails but profile cleanup succeeds;profile_cleaned: true; andAggregate evidence computes cleanup across every ordinary and forced-close trial and applies distinct fail-closed cleanup gates before repeatability/recovery acceptance. A teardown timeout is therefore classified as failure evidence rather than success, and it does not weaken the later repeatability/recovery gate.
Exact-current-head validation
On unchanged exact head
10a60f59046c1a29a6e73084804aee0203798bbbagainst exact current prerequisite1cffb2e23d4002f12e8462c4c8c24f404a4eeee7:32444083415: success;32444083543: success;32444083450: success;32444083490: success;No predecessor-head, prerequisite-head, synthetic-merge, skipped, cancelled, absent, stale, status-only, or model-only evidence is promoted as current proof.
Truth boundary
This lane records controlled temporary-profile teardown evidence only. It does not prove process termination, adversarial filesystem erasure, deletion of every browser artifact, operating-system keychain cleanup, renderer-crash attribution, arbitrary-task recovery, browser/adapter authenticity, policy authorization, durable incident evidence, or #28 completion. A production runtime still needs trusted task/process ownership, durable recovery evidence, and separately reviewed cleanup/retention semantics.