Fix ACTZipper::descend_first_k_path not backtracking - #76
Open
imlvts wants to merge 1 commit into
Open
Conversation
`descend_first_k_path` followed the first byte `k` times and gave up if that chain ran out, so it found a path of length `k` only when the leftmost chain happened to be that long: a trie whose first branch is short reported "no such path" with plenty of them to the right. Its own doc promises "continuing with depth-first exploration until a path that is `k` bytes from the focus has been found", which is what the caller needs and what the model specifies. Now a dead end backs up to the nearest ancestor with an unvisited sibling and carries on, and running out of those leaves the focus where it started, as the doc promises for `false`. Each movement is reported to the `PathObserver`; a sibling step is one byte up and one byte down. ACT's own `to_next_k_path` override goes with it. It assumed the focus it resumes from has at most one child, which the old descent guaranteed by accident; from a focus the fixed descent can now reach, it computed `k - depth` with `depth > k` and overflowed. The `ZipperIteration` default is correct from any focus and, measured on the bugfixes branch, about 2000x faster, since it steps along the arena's sequential layout with `to_next_sibling_byte` instead of re-indexing from the first child on every step. `descend_first_k_path` must stay overridden: the default resolves its common ancestor differently from the native `ReadZipper`, which the model follows. Regression test: `act_zipper_descend_first_k_path_backtracks`, against the PathMap zipper for k in 1..=4 including the full `to_next_k_path` walk, and from a focus below the root. Fails before this change. lean/FINDINGS.md ACT finding "first_k_path_no_backtrack" on the lean-fuzzer-restage branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZmoASqM5FUuzvJeJaYQjR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
descend_first_k_pathfollowed the first bytektimes and gave up if that chain ran out, so it found a path of lengthkonly when the leftmost chain happened to be that long: a trie whose first branch is short reported "no such path" with plenty of them to the right. Its own doc promises "continuing with depth-first exploration until a path that iskbytes from the focus has been found", which is what the caller needs and what the model specifies. Now a dead end backs up to the nearest ancestor with an unvisited sibling and carries on, and running out of those leaves the focus where it started, as the doc promises forfalse. Each movement is reported to thePathObserver; a sibling step is one byte up and one byte down.ACT's own
to_next_k_pathoverride goes with it. It assumed the focus it resumes from has at most one child, which the old descent guaranteed by accident; from a focus the fixed descent can now reach, it computedk - depthwithdepth > kand overflowed. TheZipperIterationdefault is correct from any focus and, measured on the bugfixes branch, about 2000x faster, since it steps along the arena's sequential layout withto_next_sibling_byteinstead of re-indexing from the first child on every step.descend_first_k_pathmust stay overridden: the default resolves its common ancestor differently from the nativeReadZipper, which the model follows.Regression test:
act_zipper_descend_first_k_path_backtracks, against the PathMap zipper for k in 1..=4 including the fullto_next_k_pathwalk, and from a focus below the root. Fails before this change.lean/FINDINGS.md ACT finding "first_k_path_no_backtrack" on the lean-fuzzer-restage branch.