Skip to content

Add review cache MVP experiment#33

Merged
ralphbean merged 1 commit into
fullsend-ai:mainfrom
raks-tt:review-cache-experiment
Jul 2, 2026
Merged

Add review cache MVP experiment#33
ralphbean merged 1 commit into
fullsend-ai:mainfrom
raks-tt:review-cache-experiment

Conversation

@raks-tt

@raks-tt raks-tt commented Jul 1, 2026

Copy link
Copy Markdown
Contributor
  • SQLite-based persistent memory for review findings
  • Function-based identity (stable across refactoring)
  • Deduplication with lifecycle tracking (new → still_present → resolved)
  • hard filtering that drops dismissed finding
  • publication policy engine that will retain still_present in summary
  • All tests passing

@raks-tt raks-tt requested a review from a team as a code owner July 1, 2026 03:23
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add SQLite-backed review cache MVP with publication policy engine

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add SQLite persistence for PR-scoped review findings with lifecycle tracking and deduplication.
• Introduce deterministic classifier + publication policy to reduce duplicate inline comments.
• Add end-to-end fixtures and shell workflows to validate multi-round review behavior.
Diagram

graph TD
  A["Review agent JSON"] --> B["filter.py"] --> C["publish.py"] --> H{{"GitHub PR comment"}}
  A --> D["save.py"] --> E["store.py"] --> F[("SQLite review-memory.db")]
  F --> G["load.py"] --> A
  B --> E
  C --> E

  subgraph Legend
    direction LR
    _io["Input/Output"] ~~~ _mod["Script/Module"] ~~~ _db[("Database")] ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. JSON-on-disk cache per PR (no DB)
  • ➕ Simpler operationally (no SQLite dependency, no schema migration)
  • ➕ Easy to inspect/merge in git artifacts
  • ➖ Harder to query/update incrementally (resolved/still_present transitions)
  • ➖ More brittle under concurrent writers and partial writes
2. Use code-hash/AST-span identity instead of line buckets
  • ➕ More resilient to line-number churn and rephrasings
  • ➕ Potentially reduces 'new' classification due to bucket drift
  • ➖ Requires reliable language parsing or stable snippet capture
  • ➖ More CPU/complexity; may be inconsistent across languages
3. Embedding/similarity matching for dedup (Phase 2)
  • ➕ Can catch paraphrased duplicates and shifted locations
  • ➕ Improves recall when agent output wording changes
  • ➖ Adds model dependency/cost and non-determinism unless carefully bounded
  • ➖ More complex evaluation and security considerations

Recommendation: For an MVP, the PR’s deterministic SQLite-backed approach is the right tradeoff: it delivers persistent lifecycle tracking and enables a publication policy that immediately reduces inline noise. Keep the current semantic key scheme for now, but consider upgrading identity (code-hash/AST or embeddings) once real-world false-new rates from line-bucket drift are measured.

Files changed (16) +3088 / -0

Enhancement (6) +1136 / -0
extract_functions.shAdd bash helper to enrich findings with function name and snippet +94/-0

Add bash helper to enrich findings with function name and snippet

• Introduces git-based extraction of function/class identifiers and a small surrounding snippet for multiple languages. Provides a jq-based enrichment pipeline for findings JSON, and also supports sourcing as a library.

review-cache-experiment/scripts/extract_functions.sh

filter.pyAdd deterministic lifecycle classifier for findings +151/-0

Add deterministic lifecycle classifier for findings

• Implements a classification step that uses the SQLite store to mark findings as new, still_present, or dismissed (attaching dismissal reason and first_seen_sha where applicable). Handles missing DB by marking everything as new and emitting a classified JSON output for publish.py.

review-cache-experiment/scripts/filter.py

load.pyAdd CLI to load prior PR findings from the cache +73/-0

Add CLI to load prior PR findings from the cache

• Adds a pre-review helper that reads prior findings from the SQLite store for a given PR and writes them to JSON. Gracefully degrades to an empty findings list when no database exists.

review-cache-experiment/scripts/load.py

publish.pyAdd publication policy engine and GitHub markdown renderer +297/-0

Add publication policy engine and GitHub markdown renderer

• Implements a policy that publishes only new findings inline while moving still_present/resolved/dismissed to a progress summary section. Optionally computes resolved findings by diffing cache keys against current output and writes both markdown and a policy JSON artifact for testing.

review-cache-experiment/scripts/publish.py

save.pyAdd save + enrichment pipeline with deduplication against cache +143/-0

Add save + enrichment pipeline with deduplication against cache

• Adds a post-review helper that enriches findings using extract_functions.sh, constructs Finding objects, and deduplicates against prior cache entries to update lifecycle status. Persists new/still_present/resolved findings back into SQLite and prints a status breakdown.

review-cache-experiment/scripts/save.py

store.pyImplement SQLite store, semantic keys, deduplication, and dismissals +378/-0

Implement SQLite store, semantic keys, deduplication, and dismissals

• Adds the core ReviewMemoryStore with schema initialization, save/update semantics that preserve first_seen metadata, PR-scoped dismissal storage, and a deterministic deduplication routine. Introduces semantic_key generation with function name + category + line bucket and sanitization to reduce prompt-injection risk in stored text.

review-cache-experiment/scripts/store.py

Tests (7) +1130 / -0
review1.jsonAdd review fixture for round 1 (initial findings) +61/-0

Add review fixture for round 1 (initial findings)

• Provides a realistic agent-result JSON fixture with six findings used across end-to-end and workflow tests. Establishes the baseline set of findings for lifecycle tracking.

review-cache-experiment/test-data/review/review1.json

review2.jsonAdd review fixture for round 2 (fixes + rephrases + new finding) +52/-0

Add review fixture for round 2 (fixes + rephrases + new finding)

• Adds a second-round fixture where some findings are resolved, some remain, and some appear as new due to rephrasing/line movement. Used to validate deduplication and lifecycle transitions.

review-cache-experiment/test-data/review/review2.json

review3.jsonAdd review fixture for round 3 (dismissals + new issue) +34/-0

Add review fixture for round 3 (dismissals + new issue)

• Adds a third-round fixture demonstrating dismissed items reappearing and a genuinely new finding. Used to validate dismissal-context persistence and policy-driven presentation.

review-cache-experiment/test-data/review/review3.json

test_e2e.pyAdd 3-round end-to-end test of lifecycle tracking and key stability +418/-0

Add 3-round end-to-end test of lifecycle tracking and key stability

• Implements an end-to-end simulation of a three-round review flow, asserting counts for new/still_present/resolved and verifying first_seen_sha and dismissal reason retention. Includes a dedicated test for semantic-key stability and known limitations around line buckets.

review-cache-experiment/test_e2e.py

test_filter.shAdd shell test for dismissal-based filtering behavior +174/-0

Add shell test for dismissal-based filtering behavior

• Adds a workflow-style test that creates findings, writes a dismissal into SQLite, runs filter.py, and asserts dismissed findings are removed from output. Also validates passthrough behavior when no dismissals exist and attempts to check verdict recalculation semantics.

review-cache-experiment/test_filter.sh

test_full_workflow.shAdd integration test for save.py + load.py round-trip persistence +96/-0

Add integration test for save.py + load.py round-trip persistence

• Adds a script that runs save/load across two review rounds and prints a final status breakdown from the cache. Validates persistence, deduplication grouping, and end-to-end JSON output formatting.

review-cache-experiment/test_full_workflow.sh

test_publication_policy.shAdd integration test for publication policy across three rounds +295/-0

Add integration test for publication policy across three rounds

• Adds a full workflow test that classifies findings, applies publication policy, saves to cache, and repeats across three rounds including dismissals. Asserts the reduction of duplicate inline comments and generates markdown previews and policy JSON for verification.

review-cache-experiment/test_publication_policy.sh

Documentation (2) +754 / -0
README.mdDocument 5-layer review cache and publication policy architecture +269/-0

Document 5-layer review cache and publication policy architecture

• Adds a full architecture write-up explaining the problem (duplicate findings), the five layers (SQLite → dedup → classifier → publication policy → GitHub posting), and integration snippets for pre/post review hooks. Includes example outputs and a test coverage/issue-claims matrix.

review-cache-experiment/README.md

results.mdAdd captured run logs demonstrating expected behavior +485/-0

Add captured run logs demonstrating expected behavior

• Checks in example outputs from the e2e and shell workflow runs, showing multi-round lifecycle transitions and publication policy effects. Acts as a human-readable artifact for evaluating the experiment.

review-cache-experiment/test-data/results/results.md

Other (1) +68 / -0
schema.sqlAdd reference SQLite schema for findings and dismissals +68/-0

Add reference SQLite schema for findings and dismissals

• Defines the MVP schema for findings plus an intentional_exceptions table holding dismissal reasons. Serves as a readable reference for the runtime schema initialized in store.py.

review-cache-experiment/scripts/schema.sql

@qodo-code-review

qodo-code-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📜 Skill insights (2)

Context used
✅ Compliance rules (platform): 9 rules
✅ Skills: writing-how-to

Grey Divider


Action required

1. set -euo pipefail not second 📜 Skill insight ☼ Reliability
Description
New shell scripts do not place set -euo pipefail immediately after the shebang, weakening
fail-fast behavior. This violates the requirement that the second line must be set -euo pipefail.
Code

review-cache-experiment/test_filter.sh[R1-8]

+#!/bin/bash
+#
+# Test the hard filter (Layer 2 enforcement)
+#
+# Verifies that dismissed findings are dropped before posting to GitHub
+
+set -euo pipefail
+
Evidence
PR Compliance ID 1062125 requires set -euo pipefail to be the second line immediately after the
shebang, but these scripts include comments/blank lines between the shebang and the set line.

review-cache-experiment/test_filter.sh[1-8]
review-cache-experiment/scripts/extract_functions.sh[1-8]
review-cache-experiment/test_full_workflow.sh[1-8]
review-cache-experiment/test_publication_policy.sh[1-13]
Skill: writing-how-to

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shell scripts added/modified in this PR do not have `set -euo pipefail` as the second line directly after the shebang.

## Issue Context
Compliance requires:
1) Line 1: `#!/usr/bin/env bash`
2) Line 2: `set -euo pipefail`
Comments and other content must come after line 2.

## Fix Focus Areas
- review-cache-experiment/scripts/extract_functions.sh[1-8]
- review-cache-experiment/test_filter.sh[1-8]
- review-cache-experiment/test_full_workflow.sh[1-8]
- review-cache-experiment/test_publication_policy.sh[1-13]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Semantic key mismatch 🐞 Bug ☼ Reliability
Description
filter.py/publish.py compute semantic keys from the input function field (defaulting to
package), but save.py stores semantic keys using an extracted function_name; when the agent
output lacks accurate function, prior matching and dismissals won’t work.
Code

review-cache-experiment/scripts/filter.py[R67-76]

+            # Build semantic key from finding
+            file_path = f_data.get('file', '')
+            line = f_data.get('line', 0)
+            category = f_data.get('category', 'unknown')
+            function_name = f_data.get('function', 'package')
+
+            # Calculate semantic key (same logic as store.py)
+            line_bucket = (line // 10) * 10 if line else 0
+            semantic_key = f"{file_path}:{function_name}:{category}:{line_bucket}"
+
Evidence
filter.py defaults to package when function is missing, while the store’s semantic key is
based on the persisted function_name (typically extracted during save), so keys can diverge and
prevent matches.

review-cache-experiment/scripts/filter.py[66-76]
review-cache-experiment/scripts/save.py[84-105]
review-cache-experiment/scripts/store.py[40-50]
review-cache-experiment/scripts/publish.py[209-218]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Semantic key derivation is inconsistent across pipeline steps:
- `save.py` persists findings keyed by extracted `function_name`.
- `filter.py` and `publish.py` derive keys from whatever `function` field exists in the incoming JSON (or `package`).
If the incoming JSON does not contain the exact same function name used by `save.py`, classification (still_present/new) and dismissal lookups will fail silently.

### Issue Context
The core feature depends on stable semantic keys across runs; inconsistent key derivation breaks deduplication and dismissal.

### Fix Focus Areas
- review-cache-experiment/scripts/filter.py[66-76]
- review-cache-experiment/scripts/publish.py[209-218]
- review-cache-experiment/scripts/save.py[84-105]
- review-cache-experiment/scripts/store.py[40-50]

### Suggested fix
Choose one consistent approach and apply it everywhere:
- Preferred: run the same enrichment step (extract function name / bucket) before *both* filtering and saving, and store the enriched `function` back into the JSON used by later steps.
 - Option A: call the same extraction routine from `filter.py` (shared helper imported from `save.py` or a new module).
 - Option B: add a dedicated `enrich.py` step that writes `function` and `code_snippet` into the findings JSON, then run `filter.py` and `save.py` over the enriched JSON.
- Ensure `publish.py.compute_resolved_findings()` uses the same derived key function as the store (single shared utility).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Non-env bash shebangs 📜 Skill insight ☼ Reliability
Description
New shell scripts use #!/bin/bash instead of the required #!/usr/bin/env bash, reducing
portability across environments. This violates the mandated shebang format for experiment scripts.
Code

review-cache-experiment/scripts/extract_functions.sh[1]

+#!/bin/bash
Evidence
PR Compliance ID 1062126 requires the first line of shell scripts to be exactly `#!/usr/bin/env
bash, but the added scripts start with #!/bin/bash`.

review-cache-experiment/scripts/extract_functions.sh[1-1]
review-cache-experiment/test_filter.sh[1-1]
review-cache-experiment/test_full_workflow.sh[1-1]
review-cache-experiment/test_publication_policy.sh[1-1]
Skill: writing-how-to

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shell scripts added in this PR start with `#!/bin/bash` but compliance requires the first line to be exactly `#!/usr/bin/env bash`.

## Issue Context
This rule applies to shell scripts used for experiment setup/execution.

## Fix Focus Areas
- review-cache-experiment/scripts/extract_functions.sh[1-1]
- review-cache-experiment/test_filter.sh[1-1]
- review-cache-experiment/test_full_workflow.sh[1-1]
- review-cache-experiment/test_publication_policy.sh[1-1]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (3)
4. Cross-PR cache collisions 🐞 Bug ≡ Correctness
Description
The findings table uses semantic_key as the only PRIMARY KEY and save_finding() updates rows
by semantic_key alone, so identical findings across different PRs can overwrite each other and
corrupt lifecycle tracking.
Code

review-cache-experiment/scripts/store.py[R138-212]

+        self.conn.executescript("""
+            CREATE TABLE IF NOT EXISTS findings (
+                semantic_key TEXT PRIMARY KEY,
+                pr_number INTEGER NOT NULL,
+                file_path TEXT NOT NULL,
+                function_name TEXT NOT NULL,
+                line_number INTEGER,
+                category TEXT NOT NULL,
+                severity TEXT NOT NULL,
+                description TEXT NOT NULL,
+                remediation TEXT,
+                code_snippet TEXT,
+                code_hash TEXT,
+                status TEXT NOT NULL,
+                first_seen_sha TEXT NOT NULL,
+                last_seen_sha TEXT NOT NULL,
+                created_at TEXT NOT NULL,
+                updated_at TEXT NOT NULL
+            );
+
+            CREATE INDEX IF NOT EXISTS idx_pr ON findings(pr_number);
+            CREATE INDEX IF NOT EXISTS idx_status ON findings(status);
+            CREATE INDEX IF NOT EXISTS idx_file_func ON findings(file_path, function_name);
+
+            -- PR-scoped dismissals for security
+            CREATE TABLE IF NOT EXISTS intentional_exceptions (
+                semantic_key TEXT,
+                pr_number INTEGER NOT NULL,
+                dismissal_reason TEXT NOT NULL,
+                approved_by TEXT,
+                created_at TEXT NOT NULL,
+                PRIMARY KEY (semantic_key, pr_number),
+                FOREIGN KEY (semantic_key) REFERENCES findings(semantic_key)
+            );
+        """)
+        self.conn.commit()
+
+    def save_finding(self, finding: Finding):
+        """
+        Save or update finding, preserving first_seen metadata.
+
+        Uses explicit UPDATE vs INSERT to avoid overwriting history.
+        """
+        # Check if exists
+        existing = self.conn.execute(
+            "SELECT first_seen_sha, created_at FROM findings WHERE semantic_key = ?",
+            (finding.semantic_key,)
+        ).fetchone()
+
+        now = datetime.now().isoformat()
+
+        if existing:
+            # Update: preserve first_seen and created_at
+            self.conn.execute("""
+                UPDATE findings
+                SET last_seen_sha = ?,
+                    status = ?,
+                    line_number = ?,
+                    description = ?,
+                    remediation = ?,
+                    code_snippet = ?,
+                    code_hash = ?,
+                    updated_at = ?
+                WHERE semantic_key = ?
+            """, (
+                finding.last_seen_sha,
+                finding.status,
+                finding.line_number,
+                sanitize_text(finding.description, max_length=1000),
+                sanitize_text(finding.remediation, max_length=1000),
+                finding.code_snippet,
+                finding.code_hash,
+                now,
+                finding.semantic_key
+            ))
Evidence
The schema makes semantic_key globally unique while storing pr_number, and the update path
ignores pr_number, enabling cross-PR overwrites.

review-cache-experiment/scripts/store.py[136-173]
review-cache-experiment/scripts/store.py[175-236]
review-cache-experiment/scripts/schema.sql[5-16]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The store claims PR context (`pr_number`) but the DB uniqueness and upsert path are keyed only by `semantic_key`. This causes cross-PR contamination when the same semantic key appears in multiple PRs.

### Issue Context
- Schema: `semantic_key TEXT PRIMARY KEY` while also storing `pr_number`.
- Upsert: existence check and UPDATE use `WHERE semantic_key = ?` only.

### Fix Focus Areas
- review-cache-experiment/scripts/store.py[136-237]
- review-cache-experiment/scripts/store.py[238-334]

### Suggested fix
- Change schema to use a composite primary key: `PRIMARY KEY (semantic_key, pr_number)`.
- Update queries to include `pr_number`:
 - existence check: `WHERE semantic_key = ? AND pr_number = ?`
 - update: `WHERE semantic_key = ? AND pr_number = ?`
- Update the `intentional_exceptions` foreign key to reference the composite key (or drop the FK if SQLite constraints become cumbersome).
- Add a migration strategy (or recreate DB) since the schema change affects existing caches.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Dismissed not hard-filtered 🐞 Bug ≡ Correctness
Description
filter.py classifies dismissed findings but still writes them into the output findings list,
contradicting the repository’s “hard filter drops dismissed before posting” test expectation.
Code

review-cache-experiment/scripts/filter.py[R77-89]

+            # Check 1: Is this finding dismissed?
+            dismissal_reason = store.get_dismissal_reason(semantic_key, pr_number)
+
+            if dismissal_reason:
+                # CLASSIFY as dismissed
+                f_data['status'] = 'dismissed'
+                f_data['dismissal_reason'] = dismissal_reason
+                print(f"ℹ️  CLASSIFIED (dismissed): {category} in {file_path}:{function_name}")
+                print(f"   Reason: {dismissal_reason}")
+                dismissed_count += 1
+                classified.append(f_data)
+                continue
+
Evidence
The code path explicitly appends dismissed findings to the output list, while the test asserts they
must be absent.

review-cache-experiment/scripts/filter.py[26-35]
review-cache-experiment/scripts/filter.py[66-116]
review-cache-experiment/test_filter.sh[102-110]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`filter.py` currently preserves dismissed findings (adds status/reason and appends them to output). However, `test_filter.sh` explicitly asserts that dismissed findings are removed from the output before downstream steps.

### Issue Context
There is a mismatch between:
- `filter.py` docstring: “PASS ALL findings to publish.py”
- `test_filter.sh`: expects dismissed findings to be absent in filtered output

### Fix Focus Areas
- review-cache-experiment/scripts/filter.py[66-116]
- review-cache-experiment/test_filter.sh[102-136]

### Suggested fix
- Implement true hard filtering in `filter.py`:
 - When a dismissal reason exists, do **not** append the finding to `classified`.
 - Optionally track dropped counts for logging.
- Implement the verdict/action recomputation expected by the test (currently `filter.py` never mutates `data['action']`):
 - Example policy: if any remaining finding is `actionable` and severity is `high/critical` (or matches your project rules), keep `request-changes`; otherwise set `comment`.
- Update the README/docstrings to match the chosen behavior (hard-drop vs summary-only transparency).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Bash command injection 🐞 Bug ⛨ Security
Description
save.py interpolates the untrusted finding file path into a bash -c script; bash will still
evaluate command substitutions like $(...) inside double quotes, enabling arbitrary command
execution during enrichment.
Code

review-cache-experiment/scripts/save.py[R30-51]

+        bash_cmd = f"""
+        source {extract_script}
+        extract_function_name "{file_path}" {line}
+        """
+        result = subprocess.run(
+            ["bash", "-c", bash_cmd],
+            capture_output=True,
+            text=True,
+            check=True
+        )
+        function_name = result.stdout.strip() or "package"
+
+        bash_cmd = f"""
+        source {extract_script}
+        extract_code_snippet "{file_path}" {line}
+        """
+        result = subprocess.run(
+            ["bash", "-c", bash_cmd],
+            capture_output=True,
+            text=True,
+            check=True
+        )
Evidence
save.py constructs a bash program text containing the untrusted file_path and runs it with `bash
-c`, which will interpret command substitutions in that string.

review-cache-experiment/scripts/save.py[19-55]
review-cache-experiment/scripts/save.py[84-105]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`review-cache-experiment/scripts/save.py` builds a bash script string with f-strings and runs it via `bash -c`. Because the finding `file` is attacker-controlled (comes from JSON), bash command substitution inside the quoted string can execute.

### Issue Context
The risky pattern is interpolating `file_path` into a shell program text. Even though it is wrapped in double quotes, bash still executes `$(...)` and backticks.

### Fix Focus Areas
- review-cache-experiment/scripts/save.py[19-59]

### Suggested fix
- Avoid `bash -c` with interpolated user input.
- Pass parameters safely:
 - `subprocess.run(["bash","-c","source ...; extract_function_name \"$1\" \"$2\"","--", file_path, str(line)], ...)`
 - Same for `extract_code_snippet`.
- Alternatively, invoke `extract_functions.sh` as an executable that accepts `--file`/`--line` arguments (no `source`, no shell evaluation), or re-implement extraction in Python.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread review-cache-experiment/scripts/extract_functions.sh
Comment thread review-cache-experiment/test_filter.sh
Comment thread review-cache-experiment/scripts/save.py
Comment thread review-cache-experiment/scripts/store.py
Comment thread review-cache-experiment/scripts/filter.py
Comment thread review-cache-experiment/scripts/filter.py
Comment thread review-cache-experiment/README.md Outdated
@ralphbean

Copy link
Copy Markdown
Member

Nice work on the prototype — the architecture is clean and the pipeline hangs together well. The 5-layer separation of state from presentation is a solid design.

One thought on the experiment framing. The strongest experiments in this repo tend to follow a structure like:

Hypothesis — a falsifiable claim stated up front. Something like: "Function-based semantic keys with line bucketing will correctly deduplicate ≥N% of repeated findings across review rounds." This tells the reader what you're trying to prove before they dive into code.

Research — evidence gathered to test the hypothesis. The synthetic fixtures here prove the plumbing works, which is valuable. To go further, you could pull real agent output from the GH Actions artifacts on the issues you reference (#1500, #1013, #2794, #1552) and feed them through the pipeline. That would show whether semantic keys hold up on real review data — where function extraction is messier, line numbers shift unpredictably, and the agent rephrases findings between rounds. Not saying this is required, but it would strengthen confidence.

Result — did the hypothesis hold? Where did it break? What surprised you? Even "the keys matched 80% of duplicates but missed cases where code moved across functions" is a useful result that informs integration decisions.

Right now the experiment proves that the plumbing is possible and the architecture is sound — that's a real result worth stating explicitly as the hypothesis. The impact table with "100% dup elimination" reads as measuring performance against synthetic data designed to pass, which undersells what you actually demonstrated.

Does that framing resonate? I think the core work here is solid and close to ready for integration — this is more about how to frame the experiment so the results are easy to evaluate.

@raks-tt raks-tt force-pushed the review-cache-experiment branch 2 times, most recently from 8893b9a to 8964e82 Compare July 2, 2026 16:32
@raks-tt

raks-tt commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Nice work on the prototype — the architecture is clean and the pipeline hangs together well. The 5-layer separation of state from presentation is a solid design.

One thought on the experiment framing. The strongest experiments in this repo tend to follow a structure like:

Hypothesis — a falsifiable claim stated up front. Something like: "Function-based semantic keys with line bucketing will correctly deduplicate ≥N% of repeated findings across review rounds." This tells the reader what you're trying to prove before they dive into code.

Research — evidence gathered to test the hypothesis. The synthetic fixtures here prove the plumbing works, which is valuable. To go further, you could pull real agent output from the GH Actions artifacts on the issues you reference (#1500, #1013, #2794, #1552) and feed them through the pipeline. That would show whether semantic keys hold up on real review data — where function extraction is messier, line numbers shift unpredictably, and the agent rephrases findings between rounds. Not saying this is required, but it would strengthen confidence.

Result — did the hypothesis hold? Where did it break? What surprised you? Even "the keys matched 80% of duplicates but missed cases where code moved across functions" is a useful result that informs integration decisions.

Right now the experiment proves that the plumbing is possible and the architecture is sound — that's a real result worth stating explicitly as the hypothesis. The impact table with "100% dup elimination" reads as measuring performance against synthetic data designed to pass, which undersells what you actually demonstrated.

Does that framing resonate? I think the core work here is solid and close to ready for integration — this is more about how to frame the experiment so the results are easy to evaluate.

Yes that is about right. Updated my findings and removed some other deviating experiments in the repo

  - SQLite-based persistent memory for review findings
  - Function-based identity (stable across refactoring)
  - Deduplication with lifecycle tracking (new → still_present → resolved)
  - hard filtering that drops dismissed finding
  - publication policy engine that will retain still_present in summary
  - All tests passing

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: rrajashe <rrajashe@redhat.com>
@raks-tt raks-tt force-pushed the review-cache-experiment branch from 8964e82 to ed1f5e2 Compare July 2, 2026 16:38

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thank you!

@ralphbean ralphbean added this pull request to the merge queue Jul 2, 2026
Merged via the queue into fullsend-ai:main with commit fb11077 Jul 2, 2026
4 checks passed
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 2, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 7:46 PM UTC · Completed 7:57 PM UTC
Commit: ed1f5e2 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

PR #33 on fullsend-ai/experiments added a review-cache MVP experiment (3,266 lines of Python/shell). No fullsend review agent ran because the author (raks-tt) opened the PR from a fork and lacks write permission on the target repo — the dispatch routing's is_event_actor_authorized check rejected all synchronize events. Only qodo-code-review[bot] provided automated review, finding real bugs (command injection, cross-PR cache collisions). Human reviewer ralphbean gave substantive methodology feedback. The retro agent was dispatched on merge despite zero fullsend agent participation, wasting tokens.

One proposal is already covered by existing issue #2942 (skip retro when no fullsend agents participated). No new proposals filed for that.

One new proposal filed for the fork-PR review coverage gap — existing issue #2552 covers deferring review for untrusted external PRs, but does not address enabling review for trusted fork contributors who are org members or have been explicitly approved.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants