feat(p2): Context + Contract extractors with tests - #2
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Free Run ID: 📒 Files selected for processing (9)
Note 🎁 Summarized by CodeRabbit FreeThe PR author is not assigned a seat. To perform a comprehensive line-by-line review, please assign a seat to the pull request author through the subscription management page by visiting https://app.coderabbit.ai/login. Comment |
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces heuristic context and contract extraction adapters along with their corresponding domain models, traits, and integration into the compilation bundle. The feedback highlights several areas for improvement in the heuristic parsing logic: ensuring that decisions and contract criteria are not overly deduplicated by generic keyword matches (which discards actual message content), refining token cleaning to handle trailing punctuation for file paths and symbols, and preventing URLs from being falsely classified as file paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // --- Decisions --- | ||
| for pat in DECISION_PATTERNS { | ||
| if lower.contains(pat) { | ||
| let summary = format!("Session contains '{pat}' language"); | ||
| let decision = Decision { summary, rationale: Some(content.to_string()) }; | ||
| if !key_decisions.iter().any(|d| d.summary == decision.summary) { | ||
| key_decisions.push(decision); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Deduplicating decisions solely by the generic summary (e.g., Session contains 'decided' language) means that only the first decision matching a given keyword is ever recorded. Any subsequent decisions in different messages using the same keyword are silently discarded. Deduplicating by the message content (rationale) instead allows capturing multiple distinct decisions across the session.
| // --- Decisions --- | |
| for pat in DECISION_PATTERNS { | |
| if lower.contains(pat) { | |
| let summary = format!("Session contains '{pat}' language"); | |
| let decision = Decision { summary, rationale: Some(content.to_string()) }; | |
| if !key_decisions.iter().any(|d| d.summary == decision.summary) { | |
| key_decisions.push(decision); | |
| } | |
| } | |
| } | |
| // --- Decisions --- | |
| for pat in DECISION_PATTERNS { | |
| if lower.contains(pat) { | |
| let summary = format!("Session contains '{pat}' language"); | |
| let decision = Decision { summary, rationale: Some(content.to_string()) }; | |
| if !key_decisions.iter().any(|d| d.rationale.as_deref() == Some(content)) { | |
| key_decisions.push(decision); | |
| break; | |
| } | |
| } | |
| } |
| // --- Success criteria --- | ||
| for pat in CRITERIA_PATTERNS { | ||
| if lower.contains(pat) { | ||
| let crit = format!("Goal/requirement: '{pat}'"); | ||
| if !success_criteria.contains(&crit) { | ||
| success_criteria.push(crit); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // --- Tests / verifications --- | ||
| for pat in TEST_PATTERNS { | ||
| if lower.contains(pat) { | ||
| let test = format!("Verification: '{pat}'"); | ||
| if !tests_or_verifications.contains(&test) { | ||
| tests_or_verifications.push(test); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // --- Constraints --- | ||
| for pat in CONSTRAINT_PATTERNS { | ||
| if lower.contains(pat) { | ||
| let constraint = format!("Constraint: '{pat}'"); | ||
| if !constraints.contains(&constraint) { | ||
| constraints.push(constraint); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // --- Do-not-touch --- | ||
| for pat in DO_NOT_TOUCH_PATTERNS { | ||
| if lower.contains(pat) { | ||
| let dnt = format!("Do-not-touch: '{pat}'"); | ||
| if !do_not_touch.contains(&dnt) { | ||
| do_not_touch.push(dnt); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The contract extractor currently formats extracted criteria using only the matched pattern (e.g., Goal/requirement: 'goal') and deduplicates based on this generic string. This completely discards the actual message content containing the specific requirement or constraint, and prevents extracting multiple distinct criteria that happen to match the same keyword. Including the message content in the formatted string preserves the actual criteria and allows proper deduplication.
// --- Success criteria ---
for pat in CRITERIA_PATTERNS {
if lower.contains(pat) {
let crit = format!("Goal/requirement: '{pat}' ({})", msg.content.trim());
if !success_criteria.contains(&crit) {
success_criteria.push(crit);
}
}
}
// --- Tests / verifications ---
for pat in TEST_PATTERNS {
if lower.contains(pat) {
let test = format!("Verification: '{pat}' ({})", msg.content.trim());
if !tests_or_verifications.contains(&test) {
tests_or_verifications.push(test);
}
}
}
// --- Constraints ---
for pat in CONSTRAINT_PATTERNS {
if lower.contains(pat) {
let constraint = format!("Constraint: '{pat}' ({})", msg.content.trim());
if !constraints.contains(&constraint) {
constraints.push(constraint);
}
}
}
// --- Do-not-touch ---
for pat in DO_NOT_TOUCH_PATTERNS {
if lower.contains(pat) {
let dnt = format!("Do-not-touch: '{pat}' ({})", msg.content.trim());
if !do_not_touch.contains(&dnt) {
do_not_touch.push(dnt);
}
}
}| // --- Files --- | ||
| for token in &tokens { | ||
| let clean = token.trim_matches(|c: char| { | ||
| !c.is_alphanumeric() && c != '/' && c != '.' && c != '-' && c != '_' | ||
| }); | ||
| if is_file_path(clean) { | ||
| let fp = clean.to_string(); | ||
| if !files_mentioned.contains(&fp) { | ||
| files_mentioned.push(fp); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If a file path is mentioned at the end of a sentence (e.g., src/auth/mod.rs.), the trailing period is not trimmed because . is excluded from the trimmed characters. This causes the extractor to capture the path with the trailing period, or fail to match the extension entirely. Trimming trailing periods from the cleaned token resolves this issue.
| // --- Files --- | |
| for token in &tokens { | |
| let clean = token.trim_matches(|c: char| { | |
| !c.is_alphanumeric() && c != '/' && c != '.' && c != '-' && c != '_' | |
| }); | |
| if is_file_path(clean) { | |
| let fp = clean.to_string(); | |
| if !files_mentioned.contains(&fp) { | |
| files_mentioned.push(fp); | |
| } | |
| } | |
| } | |
| // --- Files --- | |
| for token in &tokens { | |
| let mut clean = token.trim_matches(|c: char| { | |
| !c.is_alphanumeric() && c != '/' && c != '.' && c != '-' && c != '_' | |
| }); | |
| if clean.ends_with('.') { | |
| clean = clean.trim_end_matches('.'); | |
| } | |
| if is_file_path(clean) { | |
| let fp = clean.to_string(); | |
| if !files_mentioned.contains(&fp) { | |
| files_mentioned.push(fp); | |
| } | |
| } | |
| } |
| // --- Symbols (identifiers with :: or () pattern) --- | ||
| // Look for token-like patterns (CamelCase, snake_case with parens or ::) | ||
| for token in &tokens { | ||
| let clean = token.trim_matches(|c: char| { | ||
| c == '(' | ||
| || c == ')' | ||
| || c == ',' | ||
| || c == ';' | ||
| || c == '{' | ||
| || c == '}' | ||
| || c == '[' | ||
| || c == ']' | ||
| }); | ||
| let is_func_call = token.contains("()"); | ||
| if clean.contains("::") || is_func_call { | ||
| let sym = clean.to_string(); | ||
| if !key_symbols.contains(&sym) { | ||
| key_symbols.push(sym); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current symbol extraction logic uses a hardcoded list of characters to trim, which leaves trailing periods, question marks, or other punctuation intact (e.g., verify_token(). becomes verify_token().). We can make this much more robust by trimming all non-alphanumeric characters except _ and : from the boundaries of the token.
// --- Symbols (identifiers with :: or () pattern) ---
// Look for token-like patterns (CamelCase, snake_case with parens or ::)
for token in &tokens {
let clean = token.trim_matches(|c: char| {
!c.is_alphanumeric() && c != '_' && c != ':'
});
let is_func_call = token.contains("()");
if (clean.contains("::") || is_func_call) && !clean.is_empty() {
let sym = clean.to_string();
if !key_symbols.contains(&sym) {
key_symbols.push(sym);
}
}
}| fn is_file_path(token: &str) -> bool { | ||
| if token.is_empty() || token.len() < 3 { | ||
| return false; | ||
| } | ||
| // Must contain a `/` or start like a relative path (e.g. `src/`). | ||
| if token.contains('/') { | ||
| return true; | ||
| } | ||
| // Match common file extensions. | ||
| let lower = token.to_lowercase(); | ||
| FILE_EXTENSIONS.iter().any(|ext| lower.ends_with(ext) && lower.len() > ext.len()) | ||
| } |
There was a problem hiding this comment.
The is_file_path function checks if a token contains /. This causes it to incorrectly classify HTTP/HTTPS URLs (e.g., https://github.com) as file paths. Adding a simple prefix check to ignore URLs prevents this false positive.
fn is_file_path(token: &str) -> bool {
if token.is_empty() || token.len() < 3 {
return false;
}
if token.starts_with("http://") || token.starts_with("https://") {
return false;
}
// Must contain a `/` or start like a relative path (e.g. `src/`).
if token.contains('/') {
return true;
}
// Match common file extensions.
let lower = token.to_lowercase();
FILE_EXTENSIONS.iter().any(|ext| lower.ends_with(ext) && lower.len() > ext.len())
}
1 participant
P2 SessionLedger: Context bundle + Acceptance Contract extraction, hexagonal, following P1 IntentExtractor pattern. Gate green.