Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions src/authorship/attribution_recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,11 @@ struct CommitMetadataSessionSelection {
pub(crate) struct AttributionRecoveryContext<'a> {
pub(crate) file_timestamps: Option<&'a FileTimestampsByPath>,
pub(crate) before_external_recovery: Option<&'a dyn Fn(&UnknownLinesByFile)>,
/// Per-file sets of committed-coordinate line numbers that were explicitly displaced
/// to INITIAL by the attribution loop (i.e., human-modified lines that replaced
/// AI-checkpoint content without a known_human checkpoint firing). Edge recovery
/// must not re-claim these lines for AI — they were consciously excluded.
pub(crate) displaced_to_initial_lines: Option<&'a HashMap<String, HashSet<u32>>>,
}

pub(crate) fn recover_attribution(
Expand Down Expand Up @@ -232,6 +237,7 @@ pub(crate) fn recover_attribution(
commit_sha,
authorship_log,
committed_hunks,
context.displaced_to_initial_lines,
);
let unknown_after_edges = unknown_lines_by_file(authorship_log, committed_hunks);
if unknown_after_edges.is_empty() {
Expand Down Expand Up @@ -1122,15 +1128,40 @@ fn recover_adjacent_edges(
commit_sha: &str,
authorship_log: &mut AuthorshipLog,
committed_hunks: &HashMap<String, Vec<LineRange>>,
displaced_to_initial_lines: Option<&HashMap<String, HashSet<u32>>>,
) {
let unknown = unknown_lines_by_file(authorship_log, committed_hunks);
for (file_path, unknown_lines) in unknown {
let line_to_author = line_author_map(authorship_log, &file_path);
let runs = contiguous_runs(&unknown_lines);

// Lines that were explicitly displaced to INITIAL by the attribution loop (e.g.,
// human-modified lines that replaced AI-checkpoint content). Edge recovery must not
// re-claim these — they were consciously excluded from the authorship note.
let displaced: Option<&HashSet<u32>> =
displaced_to_initial_lines.and_then(|map| map.get(&file_path));

for run in runs {
let Some(recovery) = edge_recovery_for_run(&line_to_author, &run) else {
continue;
};

// Skip any lines that were explicitly sent to INITIAL during attribution.
let lines: Vec<u32> = if let Some(displaced_set) = displaced {
recovery
.lines
.iter()
.copied()
.filter(|&line_num| !displaced_set.contains(&line_num))
.collect()
} else {
recovery.lines.clone()
};

if lines.is_empty() {
continue;
}

let trace_id = generate_trace_id();
let source_session = recovery
.source_author
Expand All @@ -1143,19 +1174,14 @@ fn recover_adjacent_edges(
} else {
recovery.source_author.clone()
};
let recovered_line_count = recovery.lines.len() as u32;
add_attestation(
authorship_log,
&file_path,
&recovered_author,
&recovery.lines,
);
let recovered_line_count = lines.len() as u32;
add_attestation(authorship_log, &file_path, &recovered_author, &lines);

let metadata = json!({
"solver": "edge_extension",
"file_path": file_path,
"source_author": &recovery.source_author,
"recovered_lines": &recovery.lines,
"recovered_lines": &lines,
});
record_recovery_metric(RecoveryMetricInput {
repo,
Expand Down
65 changes: 64 additions & 1 deletion src/authorship/post_commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,39 @@ where
&commit_sha,
context.precomputed_parent_diff,
)?;
// Compute the set of committed-coordinate lines that were explicitly displaced to
// INITIAL by the attribution loop (i.e., human-modified lines that replaced AI
// content after the last checkpoint). These are lines present in both the INITIAL
// attributions (carrying their carryover line numbers) and in recovery_hunks. Edge
// recovery must not re-claim these lines.
//
// Coordinate invariant: initial_attributions line numbers are in carryover/workdir
// coordinates, while recovery_hunks line numbers are in committed coordinates. These
// match here because stale-detection in build_carryover_snapshot replaces the carryover
// with committed content whenever pure insertions are present (the only case that would
// shift coordinates), which empties initial_attributions for those files. The only
// scenario that populates initial_attributions is in-place replacement (no coordinate
// shift), so the direct intersection is safe. If stale-detection logic changes, a
// coordinate-aware conversion using pure_insertion_hunks would be needed here.
let displaced_to_initial_lines: HashMap<String, HashSet<u32>> = initial_attributions
.files
.iter()
.filter_map(|(file, line_attrs)| {
let committed_for_file = recovery_hunks.get(file)?;
let committed_set: HashSet<u32> =
committed_for_file.iter().flat_map(|r| r.expand()).collect();
let displaced: HashSet<u32> = line_attrs
.iter()
.flat_map(|la| la.start_line..=la.end_line)
.filter(|l| committed_set.contains(l))
.collect();
if displaced.is_empty() {
None
} else {
Some((file.clone(), displaced))
}
})
.collect();
Comment on lines +409 to +427

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.

🚩 displaced_to_initial_lines assumes carryover and committed coordinates are identical

The displaced_to_initial_lines computation at src/authorship/post_commit.rs:400-418 intersects initial_attributions line numbers (carryover/working-directory coordinates) with recovery_hunks line numbers (committed coordinates). These coordinate spaces differ when there are pure insertions in carryover. However, I traced through the scenarios and found that when pure insertions exist (human appends lines after AI checkpoint), the stale-observed detection at src/authorship/virtual_attribution.rs:2058-2062 fires and sets carryover = committed, eliminating all unstaged hunks and producing empty initial_attributions. For replacements (human modifies a line), there are no pure insertions, so coordinates are identical. The assumption appears safe for the targeted scenarios, but if future changes alter the stale-detection logic, this intersection could silently produce incorrect results.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Confirmed safe for the current scenarios.

But, I added a comment documenting the invariant and why it holds:
stale-detection in build_carryover_snapshot fires for all cases where pure insertions would exist (human appends/inserts), replacing the carryover with committed content and emptying initial_attributions for those files. The only case that populates initial_attributions is in-place replacement, which has no coordinate shift. Added a note about what would need to change (coordinate-aware conversion using pure_insertion_hunks) if the stale-detection path is ever altered.

crate::authorship::attribution_recovery::recover_attribution(
repo,
&parent_sha,
Expand All @@ -402,6 +435,11 @@ where
AttributionRecoveryContext {
file_timestamps: context.recovery_file_timestamps,
before_external_recovery: context.before_external_recovery,
displaced_to_initial_lines: if displaced_to_initial_lines.is_empty() {
None
} else {
Some(&displaced_to_initial_lines)
},
},
)?;
authorship_log.metadata.base_commit_sha = commit_sha.clone();
Expand Down Expand Up @@ -685,7 +723,7 @@ pub(crate) fn post_commit_amend_with_recovery_timestamps_detailed(
let observed_snapshot = working_log.observed_file_snapshot()?;
let mut final_state_snapshot =
commit_tree_snapshot_for_files(repo, amended_commit, &pathspecs)?;
final_state_snapshot.extend(observed_snapshot);
final_state_snapshot.extend(observed_snapshot.clone());

// Check if original commit has existing authorship data
let has_existing_data =
Expand Down Expand Up @@ -769,6 +807,26 @@ pub(crate) fn post_commit_amend_with_recovery_timestamps_detailed(
}

let recovery_hunks = recovery_committed_hunks(repo, &parent_sha, amended_commit, None)?;
// See coordinate invariant comment on the equivalent computation in post_commit_detailed.
let displaced_to_initial_lines_amend: HashMap<String, HashSet<u32>> = initial_attributions
.files
.iter()
.filter_map(|(file, line_attrs)| {
let committed_for_file = recovery_hunks.get(file)?;
let committed_set: HashSet<u32> =
committed_for_file.iter().flat_map(|r| r.expand()).collect();
let displaced: HashSet<u32> = line_attrs
.iter()
.flat_map(|la| la.start_line..=la.end_line)
.filter(|l| committed_set.contains(l))
.collect();
if displaced.is_empty() {
None
} else {
Some((file.clone(), displaced))
}
})
.collect();
crate::authorship::attribution_recovery::recover_attribution(
repo,
&parent_sha,
Expand All @@ -779,6 +837,11 @@ pub(crate) fn post_commit_amend_with_recovery_timestamps_detailed(
AttributionRecoveryContext {
file_timestamps: recovery_file_timestamps,
before_external_recovery,
displaced_to_initial_lines: if displaced_to_initial_lines_amend.is_empty() {
None
} else {
Some(&displaced_to_initial_lines_amend)
},
},
)?;
authorship_log.metadata.base_commit_sha = amended_commit.to_string();
Expand Down
Loading