feat: resolve diff feedback via commit attribution

[eloylp]

Summary

• capture signed Agents-Attribution trailers from push commit messages as commit attribution artifacts
• resolve PR diff-line /agents improve feedback through the commented commit SHA before ancestry fallbacks
• persist PR review comment ancestry fields on feedback rows and document the new resolver mode

Verification

• go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow
• go test ./...
• go test ./internal/store -run "TestRunAttributionArtifactUpsertAndLookups|TestSelfImprovementFeedbackUpsertAndList" -race
• go test ./internal/store ./internal/observe ./internal/webhook -race timed out in existing store auth migration/bcrypt tests after observe and webhook passed under race; no race report was emitted.

Closes #696

[eloylp]

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

• [medium] internal/webhook/handler.go:595 — Issue Resolve diff-comment self-improvement attribution through signed commit artifacts #696 explicitly requires that new code not overload github_comment_id for PR review comments, but the PR review comment path still writes payload.Comment.ID into CommentID/GitHubCommentID while also adding github_review_comment_id. The store upsert also still keys feedback redelivery on github_comment_id via ON CONFLICT(workspace_id, source_type, github_comment_id, github_review_id, tag) in internal/store/self_improvement.go:219, and migration 057 only adds the new ancestry columns without moving identity to github_review_comment_id. That means REST/MCP/UI consumers still see PR review comment IDs in github_comment_id, and the new field is duplicate context rather than the authoritative identity the issue requested. Please make PR review comment feedback/artifact identity use github_review_comment_id (with a compatible uniqueness/upsert path) and reserve github_comment_id for issue comments, while preserving any needed legacy reads.

Summary

The commit-artifact resolver path and unsigned-commit fail-closed behavior look covered, but the PR review comment persistence contract from #696 is not satisfied yet.

Verification: go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow passed locally.

[eloylp]

Addressed the review finding in 4fb07bb. PR review-comment feedback now keeps github_comment_id empty, carries the authoritative identity in github_review_comment_id, and redelivery/update paths look up by that review-comment identity. Added coverage for multiple review comments on the same owning review and redelivery behavior.

Verification: go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow and go test ./... pass locally.

[eloylp]

PR Review — pr-reviewer

Verdict: APPROVE

Findings

• None.

Summary

The prior request-changes finding is addressed: PR review-comment feedback now keeps github_comment_id out of the exposed identity path, uses github_review_comment_id for redelivery/update lookup, and preserves separate parent/review ancestry fields.

Verification: go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow passed locally.

[eloylp]

Please tighten the data model before we merge. The feature direction is right, but there is still implementation drift in the persistence layer that keeps fields non-orthogonal.

Blocking cleanup requested:

• internal/store/self_improvement.go:209 / :359: remove feedbackLegacyIdentity. A pull_request_review_comment should not be translated into github_review_id = github_review_comment_id internally. That keeps two meanings in one column and makes the table depend on scan-time normalization to hide the shortcut.
• internal/store/self_improvement.go:219-284: the generic insert/upsert path still uses ON CONFLICT(workspace_id, source_type, github_comment_id, github_review_id, tag) and then rereads by the same legacy pair. For PR review comments, use github_review_comment_id as the actual write identity, not just an exposed field. The migration already adds idx_self_improvement_feedback_review_comment; the store should use that as the authoritative path for insert/update/read.
• internal/store/self_improvement.go:288-340: keep the review-comment update path, but it should not write github_review_id with the review comment id. github_review_id must mean the owning pull_request_review id only; github_pull_request_review_id is the ancestry field for review-comment feedback.
• internal/store/self_improvement.go:838-844: remove normalizeFeedbackIdentity once writes are correct. We should not need to hide inconsistent DB rows from REST/MCP/UI surfaces. With no retrocompatibility requirement, the DB should store the right shape directly.
• internal/store/run_attribution_artifacts.go:19: update the stale comment. GitHubCommentID should be issue-comment identity only in new code; PR review-comment identity belongs in GitHubReviewCommentID.

Expected invariant after cleanup:

• issue_comment: github_comment_id populated; review-comment fields zero.
• pull_request_review: github_review_id populated; review-comment fields zero.
• pull_request_review_comment: github_review_comment_id populated, github_parent_comment_id optional, github_pull_request_review_id populated when GitHub provides it; github_comment_id == 0; github_review_id == 0 unless the row is truly a review artifact, which it is not.
• commit artifacts: commit identity lives in commit_sha; no comment/review identity columns are overloaded.

Please add or adjust tests so they inspect the stored DB row semantics, not only the normalized API struct. The goal is that tables are orthogonal and every field has exactly one meaning, without compatibility shims or hidden normalization.

[eloylp]

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

• [medium] internal/store/self_improvement.go:209 — The maintainer's requested data-model cleanup has not landed at this SHA. feedbackLegacyIdentity is still called before feedback upsert, so pull_request_review_comment rows are still normalized into legacy github_review_id identity internally instead of storing github_review_comment_id as the sole review-comment identity.
• [medium] internal/store/self_improvement.go:229 — The generic feedback upsert still conflicts on (workspace_id, source_type, github_comment_id, github_review_id, tag), then rereads by that same legacy identity. This does not satisfy the requested invariant that PR review-comment feedback use github_review_comment_id as the authoritative insert/update/read key.
• [medium] internal/store/self_improvement.go:838 — normalizeFeedbackIdentity is still present and invoked on read paths, so REST/MCP/UI surfaces can still depend on scan-time cleanup rather than the table storing orthogonal field semantics directly.

Summary

The feature remains directionally correct, but the maintainer's blocking cleanup request is still outstanding at 4fb07bb; this should stay out of human review ready until the DB write/read identity paths and row-level tests are tightened.

[eloylp]

Addressed the blocking data-model cleanup in 209dcfa.

Changes made:

• removed the review-comment compatibility shim and read-side normalization
• made PR review-comment feedback insert/update/read by github_review_comment_id
• added migration 058 to replace the legacy table-level feedback uniqueness with partial identity indexes, allowing review-comment rows to keep github_comment_id = 0 and github_review_id = 0
• added row-level store assertions for the expected persisted review-comment identity shape
• updated the stale run-attribution comment for GitHubCommentID

Verification:

• go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow
• go test ./...

[eloylp]

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

• [medium] internal/store/migrations/058_self_improvement_feedback_identity_indexes.sql:78 — Migration 058 rebuilds self_improvement_feedback by renaming the existing table, creating a replacement, copying rows, then dropping the renamed table, but this migration is executed inside a transaction by internal/store/migrations.go:56. In SQLite, PRAGMA foreign_keys = OFF has no effect once the transaction has started, so an existing install with rows in self_improvement_recommendations still has a live FK to the renamed parent table during the drop. That can make the upgrade fail for any database that already has analyzed feedback. Please either make this rebuild work with the migration runner’s transaction model, or add a migration test that seeds a pre-058 feedback row plus recommendation row and proves store.Open can apply 058 successfully.

Summary

The review-comment write path now stores github_review_comment_id as the direct feedback identity with github_comment_id and github_review_id at zero, and the new row-level tests cover that persisted shape. The remaining blocker is migration safety for existing self-improvement recommendation rows.

Verification: go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow and go test ./... passed locally.

[eloylp]

Addressed the migration-safety review finding in d6a342c.

Changes made:

• removed the writable_schema rewrite from migration 058
• added an explicit migration-runner opt-out for migration files that must execute outside the standard transaction so SQLite applies PRAGMA foreign_keys=OFF before table rebuilds
• added a regression test that reruns migration 058 with an existing self_improvement_recommendations row and verifies the feedback FK survives with PRAGMA foreign_key_check

Verification:

• go test ./internal/store -run 'TestSelfImprovementFeedbackIdentityMigrationPreservesRecommendations|TestOpenAndMigrate' -count=1
• go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow
• go test ./...

[eloylp]

PR Review — pr-reviewer

Verdict: APPROVE

Findings

• None.

Summary

The migration 058 blocker is addressed: the non-transactional marker is narrowly applied by the migration runner, the SQLite rebuild now runs with foreign_keys=OFF before the table rename/drop sequence, and the regression covers an existing feedback recommendation row plus PRAGMA foreign_key_check. Focused store/observe/webhook/workflow tests pass locally.

Verification: go test ./internal/store -run 'TestSelfImprovementFeedbackIdentityMigrationPreservesRecommendations|TestOpenAndMigrate' -count=1; go test ./internal/store ./internal/observe ./internal/webhook ./internal/workflow.