fix: key linked-worktree AI storage by stable workdir path, not git slot name#19
Merged
Merged
Conversation
…lot name Per-worktree AI working-log storage lived at `<common>/.git/ai/worktrees/<name>`, where `<name>` was git's internal worktree slot name (basename-derived, e.g. `autter-cli`, `autter-cli2`). Git recycles those slot names when worktrees sharing a basename are pruned and re-added, so checkpoints written under one name could be orphaned: post-commit note generation re-resolved the worktree, got a different slot name, found no working log, and silently wrote an authorship note with zero AI attribution. This hit any tool that spins up many same-basename worktrees (agent sandboxes, CI matrices). Key storage instead by `<sanitized-basename>-<sha256(canonical_workdir)[..16]>`, a stable identity derived from the worktree's absolute working-directory path. Checkpoint-time and commit-time now always resolve to the same storage dir for a given physical worktree, regardless of slot-name churn. `worktree_storage_ai_dir` is the single source of truth for this path (3 call sites, all with `canonical_workdir` already in scope); the daemon worktree map is already path-keyed, so no other callers need changes. Tests: - unit: storage is stable across slot-name churn, distinguishes distinct workdirs, and main worktree still maps to `<common>/ai`. - integration: a worktree forced onto a slot name != its basename still routes checkpoints into its workdir-keyed storage. Also clears 3 pre-existing clippy lints in the file_changes area that blocked `task lint`. Generated-By: PostHog Code Task-Id: 974337c1-2d2a-43d6-b357-cd6a54b5b39d
![autter-dev[bot]](https://avatars.githubusercontent.com/in/2920597?s=60&v=4){kind=small}
| @@ -14,6 +14,7 @@ use unicode_normalization::UnicodeNormalization; | |||
|
|
|||
There was a problem hiding this comment.
🟠 PR mixes refactor and behavior change — Risk: 55/100
PR combines a core behavior fix (worktree storage keying by path hash instead of slot name) with unrelated cleanup (clippy lint fixes in file_changes module). The file_changes changes are described as 'pre-existing clippy lints that blocked task lint' and are orthogonal to the worktree storage bug. src/git/repository.rs is one of the files contributing to the mix. Split into two PRs: (1) the worktree storage fix with its unit and integration tests (src/git/repository.rs, tests/integration/worktrees.rs, tests/integration/repository_unit.rs), and (2) a separate lint-cleanup PR for src/file_changes/mod.rs and src/commands/file_changes.rs. This allows the critical worktree fix to be reviewed and deployed independently from unrelated housekeeping.
References:
- https://docs.rs/crate/tbdflow/latest — tbdflow 0.30.1 - Docs.rs # tbdflow 0.30.1 A CLI to streamline your Git workflow for Trunk-Based Development. ## The problem Many teams say they practise Trunk-Based Development but
- https://go.dev/talks/2016/refactor.article — Codebase Refactoring (with help from Go) - The Go Programming Language # Codebase Refactoring (with help from Go) ## 1. Abstract Go should add the ability to create alternate equiv
🛠 AI fix prompt (copy & paste into your coding agent)
Split this PR so refactors land separately from behavior changes.
Pure-refactor PRs should preserve behavior (no test changes beyond renames). Behavior-change PRs should focus on a single new capability.
Start by extracting `src/git/repository.rs`'s refactor portion (or its behavior portion, whichever is smaller) into its own PR.
References:
https://docs.rs/crate/tbdflow/latest
https://go.dev/talks/2016/refactor.article
Flagged by Autter security & observability checks.
| @@ -146,10 +146,8 @@ pub fn flush_pending_to_cloud() { | |||
|
|
|||
There was a problem hiding this comment.
🟠 PR mixes refactor and behavior change — Risk: 55/100
PR combines a core behavior fix (worktree storage keying by path hash instead of slot name) with unrelated cleanup (clippy lint fixes in file_changes module). The file_changes changes are described as 'pre-existing clippy lints that blocked task lint' and are orthogonal to the worktree storage bug. src/file_changes/mod.rs is one of the files contributing to the mix. Split into two PRs: (1) the worktree storage fix with its unit and integration tests (src/git/repository.rs, tests/integration/worktrees.rs, tests/integration/repository_unit.rs), and (2) a separate lint-cleanup PR for src/file_changes/mod.rs and src/commands/file_changes.rs. This allows the critical worktree fix to be reviewed and deployed independently from unrelated housekeeping.
References:
- https://docs.rs/crate/tbdflow/latest — tbdflow 0.30.1 - Docs.rs # tbdflow 0.30.1 A CLI to streamline your Git workflow for Trunk-Based Development. ## The problem Many teams say they practise Trunk-Based Development but
- https://go.dev/talks/2016/refactor.article — Codebase Refactoring (with help from Go) - The Go Programming Language # Codebase Refactoring (with help from Go) ## 1. Abstract Go should add the ability to create alternate equiv
🛠 AI fix prompt (copy & paste into your coding agent)
Split this PR so refactors land separately from behavior changes.
Pure-refactor PRs should preserve behavior (no test changes beyond renames). Behavior-change PRs should focus on a single new capability.
Start by extracting `src/file_changes/mod.rs`'s refactor portion (or its behavior portion, whichever is smaller) into its own PR.
References:
https://docs.rs/crate/tbdflow/latest
https://go.dev/talks/2016/refactor.article
Flagged by Autter security & observability checks.
|
|
||
| #[test] | ||
| fn worktree_storage_ai_dir_keeps_full_relative_worktree_path() { | ||
| fn worktree_storage_ai_dir_is_stable_across_slot_name_churn() { |
There was a problem hiding this comment.
🟡 [heuristic] Idempotency key not detected — Risk: 48/100
This payment/order operation appears to create or mutate financial state without accepting and enforcing an idempotency key for safe retries.
🛠 AI fix prompt (copy & paste into your coding agent)
In `tests/integration/repository_unit.rs` around line 202, require and enforce an idempotency key for this payment/order operation. Store the key with request fingerprint and final response/result, reject conflicting replays, and return the saved result for safe retries.
Flagged by Autter security & observability checks.
| fs::create_dir_all(&linked_git_dir).expect("create linked git dir"); | ||
| let git_dir_a = common_dir.join("worktrees").join("autter-cli"); | ||
| let git_dir_b = common_dir.join("worktrees").join("autter-cli4"); | ||
| fs::create_dir_all(&git_dir_a).expect("create slot a"); |
There was a problem hiding this comment.
🟡 [heuristic] Multi-write without detected transaction — Risk: 48/100
Could not detect a transaction / rollback / compensating action around these multiple persistence writes. If they must all succeed-or-fail together, wrap them in a transaction (or add compensation / an outbox).
🛠 AI fix prompt (copy & paste into your coding agent)
In `tests/integration/repository_unit.rs` around line 214, multiple write side-effects can partially succeed without a transaction or compensating cleanup. Use a database transaction where possible, or add an explicit rollback/compensation/outbox flow so later failures do not leave inconsistent state.
Flagged by Autter security & observability checks.
| })?; | ||
|
|
||
| let worktree_ai_dir = worktree_storage_ai_dir(&git_dir, &git_common_dir); | ||
| let worktree_ai_dir = worktree_storage_ai_dir(&git_dir, &git_common_dir, &canonical_workdir); |
There was a problem hiding this comment.
🟠 Missing linked tracker issue — Risk: 50/100
PR description does not reference a linked tracker issue (GitHub #, Jira/Linear KEY-, or Fixes/Closes/Resolves keyword). The PR fixes a critical bug affecting worktree_storage_ai_dir (called 3 times across resolve_command_base_dir and from_bare_repository) where AI authorship data was silently orphaned in linked worktrees due to slot-name recycling — downstream impact affects all AI checkpoint round-tripping and post-commit note generation for multi-worktree repositories.
References:
- https://www.conventionalcommits.org/en/v1.0.0/ — Conventional Commits # Conventional Commits A specification for adding human and machine readable meaning to commit messages Quick Summary Full Specification Contribute # Conventio
- https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/helping-others-review-your-changes — # Helping others review your changes You can use pull requests to provide clear context for your changes and keep your team informed, improving collaboration and the quality of rev
🛠 AI fix prompt (copy & paste into your coding agent)
Add a tracker issue reference to the PR description (e.g., 'Fixes #123' or 'Resolves KEY-456') or prepend the issue number to the PR title.
Flagged by Autter security & observability checks.
| let _ = lock.mark_failed( | ||
| &row.repo_url, | ||
| &[row.file_path.clone()], | ||
| std::slice::from_ref(&row.file_path), |
There was a problem hiding this comment.
🔴 New required field added to request — Risk: 80/100
New required field appears here (TS/JS field). Existing callers that omit it will fail validation. Downstream consumers cannot be enumerated yet — re-run the indexer for full impact analysis.
References:
- https://jsonapi.org/format/ — JSON:API — Latest Specification (v1.1) # Latest Specification (v1.1) ## Status This page presents the latest published version of JSON:API, which is currently version 1.1. New vers
- https://jsonapi.org/format/1.2/ — JSON:API — Specification v1.2 (Still in Development) # Specification v1.2 (Still in Development) ## Status This page will always present the most recent text for JSON:API v1.2. Ver
🛠 AI fix prompt (copy & paste into your coding agent)
In `src/file_changes/mod.rs` around line 200, this PR adds a new REQUIRED field to a request body / params. Every existing caller that omits it will start failing validation. Either give the field a sensible default, mark it optional, or bump the API version so old clients keep working.
Flagged by Autter security & observability checks.
There was a problem hiding this comment.
Autter found 3 security/observability issue(s) in this PR:
- 🟠 Overbroad try/catch swallowing all exceptions (risk 55/100) —
tests/integration/repository_unit.rs:226 - 🟠 Overbroad try/catch swallowing all exceptions (risk 55/100) —
tests/integration/worktrees.rs:853 - 🟡 Generic placeholder identifier in production logic (risk 35/100) —
tests/integration/repository_unit.rs:253
| ); | ||
| assert!(ai_a.starts_with(common_dir.join("ai").join("worktrees"))); | ||
| // The human-recognizable basename is preserved as a prefix of the key. | ||
| let key = ai_a.file_name().unwrap().to_string_lossy(); |
There was a problem hiding this comment.
🟠 Overbroad try/catch swallowing all exceptions — Risk: 55/100
Silent error swallow detected: Rust .unwrap() without context. Errors disappear with no log, retry, or rethrow.
References:
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch — try...catch - JavaScript | MDN - Skip to main content - Skip to search # try...catch Baseline Widely available This feature is well established and works across many devices and br
- https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/Scripting/Debugging_JavaScript — JavaScript debugging and error handling - Learn web development | MDN - Skip to main content - Skip to search # JavaScript debugging and error handling - Previous - Overview: Dynam
🛠 AI fix prompt (copy & paste into your coding agent)
In `tests/integration/repository_unit.rs` around line 226, this `try` block wraps a large region and catches all exceptions, often with only a generic log. Narrow the try to the line that can actually throw, name the exception types you handle, and let the rest propagate. Overbroad catches hide real bugs in production.
Flagged by Autter security & observability checks.
| let repo = crate::repos::test_repo::TestRepo::new(); | ||
| let mut seed = repo.filename("seed.txt"); | ||
| seed.set_contents(crate::lines!["seed".human()]); | ||
| repo.stage_all_and_commit("initial").unwrap(); |
There was a problem hiding this comment.
🟠 Overbroad try/catch swallowing all exceptions — Risk: 55/100
Silent error swallow detected: Rust .unwrap() without context. Errors disappear with no log, retry, or rethrow.
References:
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch — try...catch - JavaScript | MDN - Skip to main content - Skip to search # try...catch Baseline Widely available This feature is well established and works across many devices and br
- https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/Scripting/Debugging_JavaScript — JavaScript debugging and error handling - Learn web development | MDN - Skip to main content - Skip to search # JavaScript debugging and error handling - Previous - Overview: Dynam
🛠 AI fix prompt (copy & paste into your coding agent)
In `tests/integration/worktrees.rs` around line 853, this `try` block wraps a large region and catches all exceptions, often with only a generic log. Narrow the try to the line that can actually throw, name the exception types you handle, and let the rest propagate. Overbroad catches hide real bugs in production.
Flagged by Autter security & observability checks.
|
|
||
| #[test] | ||
| fn worktree_storage_ai_dir_main_worktree_uses_common_ai() { | ||
| let temp = tempfile::tempdir().expect("tempdir"); |
There was a problem hiding this comment.
🟡 Generic placeholder identifier in production logic — Risk: 35/100
Identifier temp is a generic placeholder; production logic should name the actual concept.
🛠 AI fix prompt (copy & paste into your coding agent)
In `tests/integration/repository_unit.rs` around line 253, this identifier uses a generic placeholder name (`data`/`result`/`handler`/`temp`/`item`) in production logic. Rename it to describe what it actually contains. Placeholder names are a hallmark of AI-generated scaffolding that was never edited for context.
Flagged by Autter security & observability checks.
There was a problem hiding this comment.
Autter found 3 security/observability issue(s) in this PR:
- 🟠 [ai] Runtime error risk (risk 65/100) —
src/git/repository.rs:2187 - 🟠 [ai] Data integrity risk (risk 65/100) —
src/git/repository.rs:2197 - 🟡 [ai] Code correctness issue (risk 48/100) —
src/git/repository.rs:2204
| /// while remaining unique and stable across git worktree slot-name reuse. | ||
| fn stable_worktree_key(canonical_workdir: &Path) -> String { | ||
| let path_str = canonical_workdir.to_string_lossy(); | ||
| let mut hasher = Sha256::new(); |
There was a problem hiding this comment.
🟠 [ai] Runtime error risk — Risk: 65/100
Sha256 hasher is created but never finalized before use. The hash computation on +L2189 calls hasher.finalize immediately after hasher.update, but if the hash slice [..16] on +L2197 is accessed before finalization completes or if the string format fails, the hash value is undefined. This could produce unstable or empty storage keys, defeating the purpose of stable hashing.
🛠 AI fix prompt (copy & paste into your coding agent)
Verify that `hasher.finalize` on +L2189 is properly called and its result is fully evaluated before the slice `[..16]` is accessed on +L2197. Consider adding a debug assertion or test to confirm the hash string is non-empty and has at least 16 characters before slicing.
Flagged by Autter security & observability checks.
| .filter(|name| !name.is_empty()) | ||
| .unwrap_or_else(|| "wt".to_string()); | ||
|
|
||
| format!("{}-{}", basename, &hash[..16]) |
There was a problem hiding this comment.
🟠 [ai] Data integrity risk — Risk: 65/100
Storage key generation uses a fixed 16-character prefix of the SHA256 hash without bounds checking. If the hash string is shorter than 16 characters (unlikely but possible under error conditions), the slice &hash[..16] will panic at runtime. Additionally, the sanitization of the basename is limited to 48 characters, but there is no validation that the combined key length stays within filesystem limits.
🛠 AI fix prompt (copy & paste into your coding agent)
Add explicit bounds checking: ensure `hash.len >= 16` before slicing, or use `&hash[..hash.len.min(16)]`. Also validate that the final key length (basename + '-' + hash prefix) does not exceed a safe filesystem path component limit (typically 255 bytes on most filesystems).
Flagged by Autter security & observability checks.
| /// (ASCII alphanumerics plus `-`, `_`, `.`), bounded in length so the basename | ||
| /// portion can't blow up the resulting path. | ||
| fn sanitize_storage_component(raw: &str) -> String { | ||
| raw.chars() |
There was a problem hiding this comment.
🟡 [ai] Code correctness issue — Risk: 48/100
The sanitize_storage_component function maps invalid characters to underscore but does not handle Unicode normalization or case-folding, which could cause two different paths with different Unicode representations (e.g., NFD vs NFC) or different cases to produce the same key on case-insensitive filesystems, breaking uniqueness.
🛠 AI fix prompt (copy & paste into your coding agent)
Consider normalizing the input string to a canonical Unicode form (NFC) before sanitization, and document the filesystem case-sensitivity assumption. Alternatively, include a fallback hash collision detector in tests.
Flagged by Autter security & observability checks.
There was a problem hiding this comment.
Autter found 3 security/observability issue(s) in this PR:
- 🟡 [ai] Dead export (no callers) (risk 35/100) —
src/git/repository.rs:2203 - 🟡 [ai] Simplifiable code (risk 22/100) —
src/file_changes/mod.rs:149 - 🟡 [ai] Simplifiable code (risk 20/100) —
src/file_changes/mod.rs:200
| /// Reduce an arbitrary path component to a conservative, filesystem-safe slug | ||
| /// (ASCII alphanumerics plus `-`, `_`, `.`), bounded in length so the basename | ||
| /// portion can't blow up the resulting path. | ||
| fn sanitize_storage_component(raw: &str) -> String { |
There was a problem hiding this comment.
🟡 [ai] Dead export (no callers) — Risk: 35/100
New private function sanitize_storage_component (+L2203) is only called once internally (+L2193) within the same module. Consider inlining the logic to reduce complexity without losing clarity.
🛠 AI fix prompt (copy & paste into your coding agent)
Evaluate whether `sanitize_storage_component` adds sufficient clarity to justify its own function. If it's a one-off utility with a clear purpose, keep it. If it's just character filtering, inline the logic directly into `stable_worktree_key` at the call site to reduce cognitive overhead.
Flagged by Autter security & observability checks.
| .into_iter() | ||
| .map(|(repo_url, file_path)| (repo_url, file_path)) | ||
| .collect(); | ||
| let failed_keys: std::collections::HashSet<(String, String)> = |
There was a problem hiding this comment.
🟡 [ai] Simplifiable code — Risk: 22/100
Map operation on +L149 that just destructures and re-collects (repo_url, file_path) is redundant. The tuple is already (String, String), so .map(|(repo_url, file_path)| (repo_url, file_path)) is an identity function.
🛠 AI fix prompt (copy & paste into your coding agent)
Remove the redundant `.map` identity operation entirely. Change `failed.into_iter.map(|(repo_url, file_path)| (repo_url, file_path)).collect` to simply `failed.into_iter.collect` since the tuple types already match the target `HashSet<(String, String)>`.
Flagged by Autter security & observability checks.
| let _ = lock.mark_failed( | ||
| &row.repo_url, | ||
| &[row.file_path.clone()], | ||
| std::slice::from_ref(&row.file_path), |
There was a problem hiding this comment.
🟡 [ai] Simplifiable code — Risk: 20/100
Using std::slice::from_ref(&row.file_path) on +L200 is correct for the single-element slice, but verify the function signature accepts &[&str] or similar. The previous code cloned into a vec and passed &[String], whereas from_ref passes &[&String]. Ensure the type mismatch is intentional.
🛠 AI fix prompt (copy & paste into your coding agent)
Confirm that the `lock.mark_failed` function signature accepts `&[&String]` or that `&row.file_path` coerces correctly. If the original intent was to pass owned strings, revert to `&[row.file_path.clone]` or use `.iter.map(|s| s.as_str).collect::<Vec<_>>` to build `&[&str]`.
Flagged by Autter security & observability checks.
🚦 Pre-merge checks ·
|
| Check | Status | Explanation |
|---|---|---|
| Mixed concerns (refactor + behavior change) | PR combines a core behavior fix (worktree storage keying by path hash instead of slot name) with unrelated cleanup (clippy lint fixes in file_changes module). The file_changes changes are described as 'pre-existing clippy lints that blocked task lint' and are orthogonal to the worktree storage bug. Split into two PRs ... [truncated 367 chars] |
|
| Multi-write without detected transaction | 1 potential issue(s) detected (max risk 48/100): tests/integration/repository_unit.rs:214. | |
| Idempotency key not detected | 1 potential issue(s) detected (max risk 48/100): tests/integration/repository_unit.rs:202. | |
| Missing linked tracker issue | 1 potential issue(s) detected (max risk 50/100): src/git/repository.rs:2083. | |
| Generic placeholder identifier in production logic | 1 potential issue(s) detected (max risk 35/100): tests/integration/repository_unit.rs:253. | |
| Overbroad try/catch swallowing all exceptions | 2 potential issue(s) detected (max risk 55/100): tests/integration/repository_unit.rs:226, tests/integration/worktrees.rs:853. | |
| New required field added to request | 1 potential issue(s) detected (max risk 80/100): src/file_changes/mod.rs:200. | |
| Code correctness issue | 1 finding(s) on changed lines. | |
| Runtime error risk | 1 finding(s) on changed lines. | |
| Data integrity risk | 1 finding(s) on changed lines. | |
| Simplifiable code | 2 finding(s) on changed lines. | |
| Dead export (no callers) | 1 finding(s) on changed lines. |
✅ Passed checks (127)
| Check | Status | Explanation |
|---|---|---|
| Too many files changed | ✅ Passed | Changed 5 file(s), within the limit of 50. |
| Too many lines changed | ✅ Passed | Changed 287 line(s), within the limit of 1000. |
| Too many unrelated chapters | ✅ Passed | 4 chapter(s) detected, within the limit of 6. |
| Generated files hiding real changes | ✅ Passed | Generated-file volume (0 lines) does not obscure the 287 hand-written line(s). |
| Missing PR context | ✅ Passed | PR context looks sufficient. |
| Migration + app logic + UI combined in one PR | ✅ Passed | No database migrations or UI changes are present. The PR modifies only core repository logic and test files; there are no schema migrations or user-facing interface updates. |
| Sensitive data in logs | ✅ Passed | No sensitive data in logs issues detected. |
| Log injection | ✅ Passed | No log injection issues detected. |
| Missing audit logging | ✅ Passed | No missing audit logging issues detected. |
| Removed observability | ✅ Passed | No removed observability issues detected. |
| Silent exception swallowing | ✅ Passed | No silent exception swallowing issues detected. |
| Unhandled promise rejection | ✅ Passed | No unhandled promise rejection issues detected. |
| Circuit breaker not detected | ✅ Passed | No circuit breaker not detected issues detected. |
| Stack trace leakage | ✅ Passed | No stack trace leakage issues detected. |
| Possible TOCTOU in critical path | ✅ Passed | No possible toctou in critical path issues detected. |
| Possible non-atomic read-modify-write | ✅ Passed | No possible non-atomic read-modify-write issues detected. |
| Optimistic locking not detected | ✅ Passed | No optimistic locking not detected issues detected. |
| Rate limiting not detected | ✅ Passed | No rate limiting not detected issues detected. |
| Rate limiting removed | ✅ Passed | No rate limiting removed issues detected. |
| Batch size limit not detected | ✅ Passed | No batch size limit not detected issues detected. |
| Pagination not detected | ✅ Passed | No pagination not detected issues detected. |
| Publicly exposed storage | ✅ Passed | No publicly exposed storage issues detected. |
| Over-permissive IAM policy | ✅ Passed | No over-permissive iam policy issues detected. |
| Security group open to the internet | ✅ Passed | No security group open to the internet issues detected. |
| Unencrypted storage at rest | ✅ Passed | No unencrypted storage at rest issues detected. |
| Infrastructure missing access logging | ✅ Passed | No infrastructure missing access logging issues detected. |
| Hardcoded secret in IaC | ✅ Passed | No hardcoded secret in iac issues detected. |
| Infrastructure misconfiguration | ✅ Passed | No infrastructure misconfiguration issues detected. |
| Deprecated Kubernetes API version | ✅ Passed | No deprecated kubernetes api version issues detected. |
| Compound IaC attack chain | ✅ Passed | No compound iac attack chain issues detected. |
| PII in logs | ✅ Passed | No pii in logs issues detected. |
| PII or internals leaked in error response | ✅ Passed | No pii or internals leaked in error response issues detected. |
| PII stored without application-level encryption | ✅ Passed | No pii stored without application-level encryption issues detected. |
| User data stored without retention controls | ✅ Passed | No user data stored without retention controls issues detected. |
| PII sent to external / cross-border destination | ✅ Passed | No pii sent to external / cross-border destination issues detected. |
| Lockfile resolution / integrity tampered | ✅ Passed | No lockfile resolution / integrity tampered issues detected. |
| Dependency runs install-time lifecycle script | ✅ Passed | No dependency runs install-time lifecycle script issues detected. |
| Possible dependency-confusion attack | ✅ Passed | No possible dependency-confusion attack issues detected. |
| Lockfile resolves a dependency the manifest does not declare | ✅ Passed | No lockfile resolves a dependency the manifest does not declare issues detected. |
| Checked-in build artefact modified without source change | ✅ Passed | No checked-in build artefact modified without source change issues detected. |
| Dockerfile build-step is insecure | ✅ Passed | No dockerfile build-step is insecure issues detected. |
| External artefact pulled in without integrity pinning | ✅ Passed | No external artefact pulled in without integrity pinning issues detected. |
| Changed export, importer not updated | ✅ Passed | No changed export with an un-updated importer detected. |
| Missing CODEOWNERS reviewer approval | ✅ Passed | No missing codeowners reviewer approval issues detected. |
| Missing security-team review on sensitive path | ✅ Passed | No missing security-team review on sensitive path issues detected. |
| Source changes without matching tests | ✅ Passed | No source changes without matching tests issues detected. |
| Migration missing rollback / down step | ✅ Passed | No migration missing rollback / down step issues detected. |
| Frontend importing database client directly | ✅ Passed | No frontend importing database client directly issues detected. |
| Route handler bypassing service layer | ✅ Passed | No route handler bypassing service layer issues detected. |
| Backend service importing UI module | ✅ Passed | No backend service importing ui module issues detected. |
| Cross-context internals import | ✅ Passed | No cross-context internals import issues detected. |
| Workspace package rule violation | ✅ Passed | No workspace package rule violation issues detected. |
| Inconsistent logging pattern | ✅ Passed | No inconsistent logging pattern issues detected. |
| Inconsistent error handling | ✅ Passed | No inconsistent error handling issues detected. |
| Endpoint missing input validation | ✅ Passed | No endpoint missing input validation issues detected. |
| Multi-write without transaction wrapper | ✅ Passed | No multi-write without transaction wrapper issues detected. |
| New feature shipped without feature flag | ✅ Passed | No new feature shipped without feature flag issues detected. |
| Module placed in the wrong workspace package | ✅ Passed | No module placed in the wrong workspace package issues detected. |
| Direct env-var access bypasses config module | ✅ Passed | No direct env-var access bypasses config module issues detected. |
| Hallucinated import (package not installed) | ✅ Passed | No hallucinated import (package not installed) issues detected. |
| Nonexistent package (not found in registry) | ✅ Passed | No nonexistent package (not found in registry) issues detected. |
| Call to function that does not exist | ✅ Passed | No call to function that does not exist issues detected. |
| Repetitive boilerplate (duplicated block) | ✅ Passed | No repetitive boilerplate (duplicated block) issues detected. |
| TODO / FIXME on critical path | ✅ Passed | No todo / fixme on critical path issues detected. |
| Comment contradicts or fabricates code behaviour | ✅ Passed | No comment contradicts or fabricates code behaviour issues detected. |
| Abstraction defined but never used | ✅ Passed | No abstraction defined but never used issues detected. |
| Code style differs from rest of codebase | ✅ Passed | No code style differs from rest of codebase issues detected. |
| Established pattern ignored | ✅ Passed | No established pattern ignored issues detected. |
| Unhandled edge case (null / empty / zero / boundary) | ✅ Passed | No unhandled edge case (null / empty / zero / boundary) issues detected. |
| Doc-copy code with insecure defaults | ✅ Passed | No doc-copy code with insecure defaults issues detected. |
| Dead code (defined but never referenced) | ✅ Passed | No dead code (defined but never referenced) issues detected. |
| Deprecated API call | ✅ Passed | No deprecated api call issues detected. |
| API pattern from wrong library version | ✅ Passed | No api pattern from wrong library version issues detected. |
| API endpoint removed | ✅ Passed | No api endpoint removed issues detected. |
| HTTP method changed (GET ↔ POST etc.) | ✅ Passed | No http method changed (get ↔ post etc.) issues detected. |
| Field removed from response schema | ✅ Passed | No field removed from response schema issues detected. |
| Response field type changed | ✅ Passed | No response field type changed issues detected. |
| HTTP status code changed | ✅ Passed | No http status code changed issues detected. |
| Auth requirement added / removed / changed | ✅ Passed | No auth requirement added / removed / changed issues detected. |
| Error response shape changed | ✅ Passed | No error response shape changed issues detected. |
| Pagination behaviour changed | ✅ Passed | No pagination behaviour changed issues detected. |
| Outbound webhook payload schema changed | ✅ Passed | No outbound webhook payload schema changed issues detected. |
| GraphQL field removed without deprecation | ✅ Passed | No graphql field removed without deprecation issues detected. |
| GraphQL enum value removed | ✅ Passed | No graphql enum value removed issues detected. |
| SQL injection | ✅ Passed | No sql injection issues detected. |
| Cross-site scripting (XSS) | ✅ Passed | No cross-site scripting (xss) issues detected. |
| Path traversal | ✅ Passed | No path traversal issues detected. |
| Command injection | ✅ Passed | No command injection issues detected. |
| Insecure deserialization | ✅ Passed | No insecure deserialization issues detected. |
| Weak cryptography | ✅ Passed | No weak cryptography issues detected. |
| Hardcoded secret | ✅ Passed | No hardcoded secret issues detected. |
| Insecure randomness for security material | ✅ Passed | No insecure randomness for security material issues detected. |
| Unsafe file upload | ✅ Passed | No unsafe file upload issues detected. |
| Missing input validation | ✅ Passed | No missing input validation issues detected. |
| Unsafe CORS configuration | ✅ Passed | No unsafe cors configuration issues detected. |
| Unsafe / open redirect | ✅ Passed | No unsafe / open redirect issues detected. |
| Missing CSRF protection | ✅ Passed | No missing csrf protection issues detected. |
| Unsafe cookie / session settings | ✅ Passed | No unsafe cookie / session settings issues detected. |
| Sensitive data exposure | ✅ Passed | No sensitive data exposure issues detected. |
| API key in source | ✅ Passed | No api key in source detected. |
| Access token in source | ✅ Passed | No access token in source detected. |
| Private key in source | ✅ Passed | No private key in source detected. |
| Database connection URL with embedded credentials | ✅ Passed | No database connection url with embedded credentials detected. |
| Cloud credential in source | ✅ Passed | No cloud credential in source detected. |
| Webhook signing secret in source | ✅ Passed | No webhook signing secret in source detected. |
| OAuth client secret in source | ✅ Passed | No oauth client secret in source detected. |
| JWT signing secret in source | ✅ Passed | No jwt signing secret in source detected. |
| Auth middleware removed from route | ✅ Passed | No auth middleware removed from route issues detected. |
| Route protection changed (protected → public) | ✅ Passed | No route protection changed (protected → public) issues detected. |
| Permission / RBAC check removed | ✅ Passed | No permission / rbac check removed issues detected. |
| Required role weakened | ✅ Passed | No required role weakened issues detected. |
| Admin-only route exposed to lower privilege | ✅ Passed | No admin-only route exposed to lower privilege issues detected. |
| Token validation skipped in middleware chain | ✅ Passed | No token validation skipped in middleware chain issues detected. |
| JWT verification weakened or changed | ✅ Passed | No jwt verification weakened or changed issues detected. |
| Session expiration / TTL changed | ✅ Passed | No session expiration / ttl changed issues detected. |
| Password reset flow changed | ✅ Passed | No password reset flow changed issues detected. |
| OAuth callback / redirect handling changed | ✅ Passed | No oauth callback / redirect handling changed issues detected. |
| Webhook endpoint missing signature verification | ✅ Passed | No webhook endpoint missing signature verification issues detected. |
| Public route touches private/PII data | ✅ Passed | No public route touches private/pii data issues detected. |
| Resource leak risk | ✅ Passed | No additional explanation was reported. |
| Maintainability issue | ✅ Passed | No additional explanation was reported. |
| Redundant alias / duplicate import | ✅ Passed | No additional explanation was reported. |
| Redundant type construct | ✅ Passed | No additional explanation was reported. |
| Unnecessary type assertion | ✅ Passed | No additional explanation was reported. |
| Module smell | ✅ Passed | No additional explanation was reported. |
| Excessive complexity | ✅ Passed | No additional explanation was reported. |
| Code duplication / DRY violation | ✅ Passed | No additional explanation was reported. |
This comment is updated automatically whenever Autter reviews a new PR revision.
Autter task list
Generated from PR diff, blast radius, and context. Issues found
🛠 Fix options (coming soon)Choose how you would like Autter to package unresolved fixes once automated fix PRs are available.
These checkboxes are currently informational and do not trigger a fix run yet. |
There was a problem hiding this comment.
Autter completed PR review for #19. Generated a PR summary, 8 task(s), and 15 finding(s).
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
1 participant
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.
Problem
AI authorship was silently lost for commits made inside linked git worktrees — even though the proxy was installed, the daemon was running, and checkpoints fired. The PR-authorship UI showed "No AI authorship data" for branches written entirely with AI.
Root cause
Per-worktree AI working-log storage lived at
<common>/.git/ai/worktrees/<name>, where<name>was git's internal worktree slot name (basename-derived, e.g.autter-cli,autter-cli2,autter-cli4). Git recycles those slot names when worktrees sharing a basename are pruned and re-added.When many worktrees share a basename (the norm for agent sandboxes that always check out into
.../<id>/autter-cli, CI matrices, etc.), the slot name a worktree gets is unstable. Checkpoints written under one name got orphaned: at commit time the worktree re-resolved to a different slot name,working_log_for_base_commitfound no live working log, and post-commit note generation silently wrote an authorship note with zero AI attribution.Observed in the wild: four storage dirs (
autter-cli1..4) all holding archived working logs for the same base commit, none consumed.Fix
Key linked-worktree storage by a stable identity derived from the worktree's absolute working-directory path:
Checkpoint-time and commit-time now always resolve to the same storage dir for a given physical worktree, regardless of slot-name churn.
worktree_storage_ai_diris the single source of truth (3 call sites, all withcanonical_workdiralready in scope); the daemon's worktree map is already path-keyed, so no other callers change.Tests
repository_unit.rs): storage is stable across slot-name churn, distinguishes distinct workdirs, main worktree still maps to<common>/ai.worktrees.rs): a worktree forced (via a decoy) onto a slot name ≠ its basename still routes checkpoints into its workdir-keyed storage and resolves identically on re-lookup._in_worktreefailures for amend/cherry_pick/rebase/stats pre-exist onmain).Note on existing data
Not backfilled: in-flight checkpoints written under the old slot-name keying before upgrade won't carry to their next commit (one-time transition; orphaned dirs expire via the existing 7-day TTL). All new work post-upgrade is captured correctly. Commits already made without notes (e.g. PR #17's) remain linkless — the checkpoint signal for them can't be reconstructed.
Also clears 3 pre-existing clippy lints in the
file_changesarea that blockedtask lint.Created with PostHog Code
Summary
Summary generated by Autter.
Fixes a bug where AI storage directories for linked worktrees were keyed by git's internal slot name (e.g.
worktrees/<slot>/) instead of a stable, content-addressed identifier derived from the worktree's actual working directory path. This meant that removing and re-adding a worktree at the same path could produce a different slot name, causing Autter to create a new, disconnected storage directory and orphan any previously accumulated working logs and checkpoints for that path.Changes
src/git/repository.rs— Rewritesworktree_storage_ai_dirto derive the storage key from a SHA256 hash of the canonicalized worktree working directory path rather than the git slot name. Adds aunicode_normalizationimport to ensure path hashing is NFC-stable across platforms. Updatesresolve_command_base_dirandfrom_bare_repositorycall sites to pass the required workdir path.tests/integration/worktrees.rs— Adds 124 lines of new integration test coverage exercising the stable-path keying contract: verifies that storage directories use the<common-dir>/ai/worktrees/<hash>/structure, that re-creating a worktree at the same path reuses the same hash bucket, and that distinct paths produce distinct buckets.tests/integration/repository_unit.rs— Expands unit tests forworktree_storage_ai_dirto cover the new path-based hashing logic, replacing slot-name assertions with workdir hash assertions.src/file_changes/mod.rs— Minor cleanup: removes a redundant flush path and corrects the error string inmark_batch_failed.src/commands/file_changes.rs— Adds a missing#[allow(...)]annotation in the test module.Acceptance Criteria
worktree_storage_ai_dirreturns a path of the form<git-common-dir>/ai/worktrees/<sha256-of-workdir>/for a linked worktree.<git-dir>/ai/directly.worktrees.rsandrepository_unit.rspass undertask test.Test Plan
task test TEST_FILTER=worktreeto execute the full linked-worktree integration suite, including the new stable-keying tests.task test TEST_FILTER=worktree_storage_ai_dirto target the unit-level assertions for the renamed storage function.git worktree add ../my-worktree), record the storage path viaautterdebug output or by inspecting$(git rev-parse --git-common-dir)/ai/worktrees/, then remove the worktree (git worktree remove my-worktree) and re-add it at the same path; confirm the hash bucket directory is identical.ai/worktrees/.task lintandtask fmtto confirm no regressions in code quality.Rollback Plan
worktree_storage_ai_dirchanges insrc/git/repository.rsto restore the original slot-name–based keying viagit revert bb0faa47.<common-dir>/ai/worktrees/<hash>/back to<common-dir>/ai/worktrees/<slot-name>/to recover prior working logs.task devand validate withtask test TEST_FILTER=worktree.Related Issues
No linked issue was identified in the branch name, commit messages, or diff context.
Written for commit bb0faa4. Summary will update on new commits.