Skip to content

fix(runtime): count Edit recovery failures by canonical file path - #1452

Merged
vastsa merged 5 commits into
vastsa:mainfrom
caulif:fix/edit-recovery-path-identity
Oct 8, 2026
Merged

vastsa merged 5 commits into
vastsa:mainfrom
caulif:fix/edit-recovery-path-identity

Conversation

@caulif

@caulif caulif commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Edit recovery tracks failures by path spelling rather than file identity. Changing the spelling of the same file can grant separate failure budgets and per-error-code grace. A successful Edit or Write through another alias also fails to clear the original budget.

For example, three EDIT_PARSE_FAILED results for these paths currently do not trigger the three-failure limit:

  1. src/example.ts
  2. src/./example.ts
  3. The absolute path to the same file

Each spelling receives its own counter. This allows repeated unsuccessful edits to continue beyond the intended limit. Conversely, stale counts can cause premature termination after a successful mutation through another alias.

Cause and fix

The old key only replaces backslashes and removes a leading ./; it does not resolve workspace-relative paths or filesystem aliases.

Resolve a bookkeeping identity before the mutation runs:

  • Normalize the absolute path against the project root, or the scratch root for temporary sessions.
  • Use filesystem realpath when the target exists.
  • Use the same identity for failure counts, per-code grace and successful-mutation reset.

Capturing the identity before execution also allows a successful removal to clear the original counter after the file disappears.

The original tool arguments still go to the Host, which remains authoritative for permissions and execution. Parent-turn and delegate-run budgets remain isolated. Canonical paths are not lowercased, preserving distinct files on case-sensitive filesystems.

Known unavailable-path errors fall back to normalized absolute spelling; unexpected resolution errors propagate. Missing paths behind directory links and Host automatic path rebinding remain outside canonical identity unification.

Validation

  • Seven initial runtime regressions failed before the fix and passed afterward.
  • Coverage includes alias accumulation, per-code grace, successful Edit/Write reset, independent files, temporary-session roots and removal through a linked path.
  • Original full agent-runtime suite: 1,314 passed; two POSIX-only cases skipped on Windows.
  • Seven real-filesystem assertions passed under WSL, including case-distinct files and symlink aliases.
  • Runtime build/typecheck and all three controlled subagent-isolation E2E modes passed.
  • All five fork CI checks passed on the original reviewed revision.

The recovery contract and E2E scenarios are updated. The failure threshold, Host permissions, path mutex and parent/delegate isolation contracts are unchanged.

macOS review follow-up

The helper tests now probe actual directory case sensitivity and use a canonical root for POSIX backslash filenames. The contract documents the lexical-to-canonical transition after creation. Follow-up runtime/helper tests: 299 passed, one POSIX-only skip on Windows. Linux checks cover the case-sensitive branch and a backslash filename beneath a symlinked root; build/typecheck and all three delegate E2E modes pass. Independent follow-up review passed. macOS has not been rerun locally; reviewer revalidation was requested. Validation applies to 5f8dfac.

Path aliases must share the existing edit recovery budget and successful
mutation reset. Resolve bookkeeping identity before tool execution while
preserving the original Host arguments and delegate ownership boundaries.

Cover filesystem aliases, temporary-session roots, resets and independent
files with runtime regressions, and document the lexical fallback.

@muzimu217 muzimu217 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified locally on macOS (arm64, current main 278e929ca + this branch).

The mechanism is correct and the design is careful. Pre-mutation identity via realpath with a documented lexical-absolute fallback for missing targets, one key shared across failure counts, per-code grace and success reset; the submitted path is never rewritten and host authority is untouched — the assertion that the exact alias sequence reaches tools.execute unmodified is a nice touch. The REM-through-link identity-clearing test covers the subtlest edge in this design. Results: vitest run src/mutation-recovery.test.ts src/runtime.test.ts → 2 failed | 297 passed; runtime.test.ts is fully green at 296/296 including all eight new alias tests. CI green on Linux.

Two new unit tests in mutation-recovery.test.ts fail on macOS. CI is ubuntu-only, so this won't surface there — but pnpm --filter @pi-desktop/agent-runtime test fails on a stock macOS checkout:

  1. keeps existing case-distinct files independent — macOS mkdtemp under /var/folders sits on case-insensitive APFS, so a.ts and A.ts are the same file; both spellings realpath identically, which is correct per your own contract ("filesystem-supported case aliases share the budget"). The guard needs a runtime case-sensitivity probe (write a.ts, stat A.ts), not skipIf(process.platform === "win32").
  2. preserves POSIX backslashes in file names — /var is a symlink to /private/var, so realpath returns the /private prefix while the expectation uses lexical join(root, …). Compare against await realpath(root) instead.

Related implementation nit, non-blocking: under a symlinked root, a target that doesn't exist keys on the lexical absolute path (/var/...) while the same target after creation keys on realpath (/private/var/...) — the budget splits across the creation boundary. Fine for bookkeeping, but worth one sentence in the new contract paragraph.

Another non-blocking: mutationFailureKey rethrows unexpected errno codes; since the key is bookkeeping-only and computed before the tool runs, treating unknown codes as the lexical fallback would be strictly safer than rejecting before execution.

Docs: this PR and #1451 both touch 18-line-anchored-edit-contract.md and 04-e2e-test-plan.md — whoever merges second will need a rebase.

fix scope, fits the current contribution window; runtime-side changes get a clean pass from me once the two macOS test assumptions are fixed.

caulif added 2 commits October 7, 2026 23:32
Keep the published repair branch current without rewriting its history.
Test actual directory case sensitivity rather than OS labels, and use
canonical temporary roots when asserting POSIX filenames. This removes
macOS assumptions while retaining assertions on both filesystem modes.

Document that lexical fallback counters need not survive a later
transition to canonical identity.
@caulif

caulif commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks—both test assumptions are fixed in 5f8dfac9f. The case test now probes the directory and asserts the appropriate identity behavior in either mode; the POSIX backslash expectation uses realpath(root). The contract now documents the lexical-to-canonical transition after creation.

Independent review passed. Runtime/helper tests: 299 passed, one POSIX-only skip on Windows; Linux case-sensitive and symlink-root checks, typecheck and all three delegate E2E modes pass. Unexpected-error propagation remains unchanged. I haven't run macOS locally—could you please recheck there?

@muzimu217 muzimu217 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirmed on macOS (arm64): vitest run mutation-recovery runtime passes 351/351 across 6 files on 5f8dfac9f. The runtime case probe with both branches asserted is the right shape (it now also proves the case-insensitive path intentionally shares one identity), and the realpath(root) expectation fixes the backslash test. Nothing blocking from my side.

@vastsa
vastsa merged commit b0a73ae into vastsa:main Oct 8, 2026
5 checks passed
@vastsa

vastsa commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Reviewed and merged. The failure-count key now follows the filesystem canonical path for existing targets, with a lexical fallback for missing or inaccessible targets; the original path still reaches Host unchanged. Candidate validation passed: agent-runtime tests (1,315), typecheck, subagent Edit isolation E2E, host-core Edit tests (15), and GitHub CI (all 5 checks). The PR integration merge-ref tree matched the tested candidate.

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.

3 participants