feat(goals): track line changes per goal - #415
Conversation
f73fd20 to
4241b44
Compare
4241b44 to
2b30fde
Compare
andrei-hasna
left a comment
There was a problem hiding this comment.
REVIEW_RESULT: request_changes
GitHub rejected a CHANGES_REQUESTED review because the active account (andrei-hasna) authored this PR. These findings apply to commit 2b30fde:
[P1] Prevent cross-goal double counting in shared worktrees - codex-rs/ext/goal/src/line_changes.rs:58
stats_since_baseline diffs the entire repository from a per-goal baseline, without a thread/goal ownership boundary. If two active threads use the same cwd (which manual/shared worktree sessions support), each snapshot includes both threads' edits, so both goals record the combined additions/deletions and a plan aggregate double-counts them. The result depends on interleaving rather than goal ownership. Add per-worktree exclusion/unique-worktree enforcement or tool-owned delta attribution, with a same-cwd concurrent-goals regression test.
[P1] Avoid a full repository snapshot after every completed tool - codex-rs/ext/goal/src/accounting.rs:291, codex-rs/ext/goal/src/extension.rs:445, codex-rs/git-utils/src/worktree_snapshot.rs:120
Once a line-change baseline exists, progress_snapshot always returns line_changes even when token and time deltas are zero. on_tool_finish therefore snapshots after every completed non-goal tool, including read-only tools, and each snapshot initializes a temporary index then executes git add --all and write-tree across the repository. On this 5,779-file checkout, one no-op snapshot took about 0.77 seconds, so tool-heavy turns repeatedly pay that latency and larger repositories will be worse. Gate this work on a mutation/change signal or use a cached/incremental strategy, and add a regression that proves read-only tool completion does not rescan the whole worktree.
Summary
codex-goal-extensionlib target attest = false; LOC tests use the existing integration-test surface.P1 repair design
codewith-line-change-accounting.lockin the linked worktree private Git directory is opened without truncation and held withFile::try_lock()for the goal-turn baseline lifetime. The process-local registry adds owner/refcount semantics; the filesystem lock is the cross-process authority.NoWorktreeMutation/MaybeMutatesWorktreesignal. Proven read-only first-party tools and MCP tools withreadOnlyHint=trueskip worktree capture while preserving token/time accounting; unknown, dynamic, shell, patch, and other credible mutation paths conservatively capture.Verification
30316026777on exact SHAe04b0e2bcfc31017b87592c6d7270cb1787de7d5: pass. Scoped test compilation, Git snapshot tests, all goal accounting integration tests, the subprocess shared-CWD lease regression, read-only no-rescan regression, parallel completion regression, and core lifecycle dispatch/classification regressions passed.30316709172on the same exact SHA: pass. Scopedjust fixcompleted with no diff; Just format check andcargo fmt --all -- --config imports_granularity=Item --checkfromcodex-rspassed.30316046857was superseded after scopedjust fixcompleted cleanly; its only failure was the gate command targeting the virtual workspace manifest without--all. Replacement30316709172corrected the command and passed.just fmt,git diff --check, staged gitleaks before each commit/push, and full-candidate gitleaks: pass.Residual tradeoffs
MaybeMutatesWorktree, preserving correctness at the cost of a scan until they are proven read-only.Run
run_8be47d216111· backendcodewith· taskinline🏭 Generated by @hasnaxyz/factory
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.