feat(observe): resolve self-improvement attribution via GitHub artifact ownership

[eloylp]

Summary

• Adds run_attribution_artifacts table (migration 056) as a narrow reverse-index from GitHub artifact identity (comment_id, review_id, review_comment_id) to span_id, keeping run_attributions as the canonical run-fact source
• Extended AttributionQuery with CommentID, ReviewCommentID, ReviewID, InReplyToID, PullRequestReviewID, FilePath to carry artifact ancestry context into the resolver
• ResolveRunAttribution now walks the artifact ownership chain (comment → parent reply → owning review → conservative PR context) before falling back to time-window inference; added mode constants (direct, artifact_comment, artifact_parent_comment, artifact_review, artifact_pr_context, commit_trailer, inferred, unresolved)
• inferRunAttributions now excludes internal-catalog-analyst runs from loose inference — only exact signed metadata or artifact ancestry can resolve to those agents
• webhookComment extended with InReplyToID and PullRequestReviewID; CaptureArtifact is called for every issue_comment, pull_request_review, and pull_request_review_comment delivery (not just /agents improve), storing valid signed artifacts in the reverse-index
• 9 new focused tests covering all required paths: direct metadata, artifact chain (comment, parent reply, review), wrong-repo rejection, invalid signature rejection, ambiguous PR context, internal analyst exclusion from inference, and unresolved fallback

Test plan

• go test ./internal/observe/... — all 9 new artifact tests pass
• go test ./internal/webhook/... — existing webhook tests still pass
• go test ./internal/store/... — migration applied, schema validates
• go test ./... — full suite green

Closes #692

[eloylp]

PR Review — pr-reviewer

Verdict: APPROVE

Findings

• [low] internal/webhook/handler.go:792-807 — captureArtifact unconditionally overwrites in.WorkspaceID, in.RepoOwner, and in.RepoName from the repo argument, but all three call sites already populate those same fields in the RunAttributionArtifactInput literal they pass in. One of the two should do the work; right now both do, silently. The override in the helper is a safe guard but the call-site initializations are dead writes. Could be simplified by either having the helper set them (and callers omit them) or vice versa.

• [low] internal/observe/attribution.go:131-170 — The inline step comments (// Step 2, // Step 3, // Step 4, // Step 6) skip numbering the ReviewID direct-check block that sits between "Step 4" and "Step 5" (q.ReviewID > 0). The code is functionally correct, but the numbering doesn't line up with the design doc or the resolveArtifactAttribution doc comment's "2–6" range. A // Step 5 (ReviewID direct) comment or adjusting the labels would make the walk easier to audit later.

• [low] internal/observe/attribution_artifacts_test.go:342 — The tamper expression validComment[len(validComment)-5+5:] evaluates to validComment[len(validComment):] which is always "". The intent (replace last 5 chars) is correct and the test does invalidate the signature, but the trailing slice expression is dead code. tampered := validComment[:len(validComment)-5] + "XXXXX" is equivalent and clearer.

What's correct

• SQL injection: Every query uses ? placeholders throughout attribution_artifacts.go. No string interpolation of external input anywhere.
• Concurrency safety: selfImprovementInternalAgentNames is a package-level read-only map (safe). All DB writes go through s.db.Exec/s.db.QueryRow which are safe for concurrent use (SQLite WAL). No unprotected shared mutable state introduced.
• Unique index design: The three partial unique indexes cleanly distinguish issue comments (source_type, github_comment_id > 0), PR reviews (github_review_id > 0 AND github_review_comment_id = 0), and PR review comments (github_review_comment_id > 0). ON CONFLICT DO NOTHING idempotency is correct because all three indexes enforce the right keys and the filters prevent cross-type collisions.
• runAttributionArtifactByReviewID filter: AND github_review_comment_id=0 correctly isolates PR review rows from review comment rows that happen to share the same github_review_id — crucial for the step 4/ReviewID path.
• Ambiguous PR context short-circuit: resolveArtifactAttribution returns (resolution, true) when PR context yields >1 candidate, which correctly prevents the caller from falling through to time-window inference for an ambiguous result.
• artifactResolution unknown span: Returns (AttributionUnresolved, true) rather than (_, false) when the artifact exists but the span isn't in run_attributions. This correctly short-circuits inference (the artifact reference is definitive evidence that inference would be wrong).
• Internal analyst exclusion: The filter in inferRunAttributions plus the map guard in attribution.go ensures internal-catalog-analyst is only reachable via exact paths; the TestInferenceSkipsInternalAnalystRuns two-part test (one mixed, one analyst-only) covers both branches of the switch.
• Issue Resolve self-improvement attribution through GitHub artifact ownership #692 acceptance criteria: All items satisfied — artifact ancestry resolution, run_attributions stays canonical, no GitHub writes, wrong-repo rejection, invalid-sig rejection, ambiguity → unresolved, and all nine test scenarios from the issue checklist are present.

Summary

Solid, well-scoped implementation of artifact-based attribution resolution. The three minor findings are cosmetic; the logic, SQL posture, concurrency model, and test coverage are all correct.

[eloylp]

Addressed the three low review cleanup findings in 8576706:

• removed duplicate WorkspaceID / repo fields from the artifact capture call sites, leaving captureArtifact as the single owner for those fields
• clarified the artifact resolution step comments around direct review lookup
• simplified the invalid-signature tamper expression

Verified locally:

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

[eloylp]

PR Review — pr-reviewer

Verdict: APPROVE

Findings

• None.

Summary

Cleanup commit 8576706 addresses the prior low findings; the artifact reverse-index implementation still satisfies issue #692 and CI is green. Local verification: go test -race ./internal/observe/... ./internal/webhook/... passed as part of the focused run; go test ./internal/store/... passed. The combined go test -race ./internal/observe/... ./internal/webhook/... ./internal/store/... run timed out in internal/store at Go's 10-minute package timeout after observe/webhook had passed.

[eloylp]

Requested change before merge: please move the new raw SQL/persistence operations for run_attribution_artifacts into internal/store.

The feature shape is correct, but the current implementation puts new SQL in internal/observe/attribution_artifacts.go. We have intentionally been keeping SQL centralized in internal/store for consistency of schema ownership, migrations, scans, transaction behavior, and future refactors. I agree that packages owning their own persistence can be a valid design in general, but that is not the architecture we want to extend right now. Existing SQL outside internal/store is historical convenience debt, not precedent.

Please adjust this PR as follows:

• Keep the resolver/domain behavior in internal/observe if that is still the clearest owner.
• Move the artifact table row types and SQL primitives into internal/store:
  • upsert artifact
  • lookup by issue comment id
  • lookup by PR review comment id
  • lookup by PR review id
  • lookup by PR/file/commit context
  • row scanning
• internal/observe should call typed store APIs instead of using s.db.Exec, s.db.Query, or s.db.QueryRow for the new artifact table.
• Keep run_attributions canonical; the artifact table remains only a GitHub artifact -> span_id reverse index.
• Preserve the existing tests and add/adjust tests at the store boundary if needed.
• Update AGENTS.md and CLAUDE.md in this PR to record the rule: all new SQL belongs in internal/store; non-store SQL is legacy debt, not a pattern to extend.

This is release-blocking because the self-improvement feature is about to become a flagship surface, and we do not want to introduce more persistence-boundary drift right before launch.

[eloylp]

PR Review — pr-reviewer

Verdict: REQUEST_CHANGES

Findings

• [blocking] internal/observe/attribution_artifacts.go:1 — Maintainer guidance in the latest conversation comment requires the new raw SQL and persistence primitives for run_attribution_artifacts to move into internal/store, with internal/observe calling typed store APIs instead of owning s.db.Exec, s.db.Query, or s.db.QueryRow for the artifact table. The current PR still adds the artifact row types, upsert, lookup, and scanning SQL in internal/observe/attribution_artifacts.go, and the docs update currently reinforces observe as the owner. That misses the release-blocking architecture requirement.

Summary

The feature behavior remains on target, but the latest maintainer request narrows the acceptable implementation boundary; this PR needs the artifact persistence moved into internal/store and the code map docs updated before it is merge-ready.

[eloylp]

Addressed the release-blocking persistence-boundary request in ef8d55a:

• moved run_attribution_artifacts row types, upsert, lookups, and scanning into internal/store
• kept internal/observe responsible for signature/context validation and artifact-chain resolution while calling typed store APIs for the new table
• added store-boundary coverage for idempotent upsert, comment/review/review-comment lookups, PR context lookup, and review-vs-review-comment isolation
• updated AGENTS.md and CLAUDE.md to record that new SQL belongs in internal/store and non-store SQL is legacy debt, not precedent

Verified locally:

• go test ./internal/store/... ./internal/observe/... ./internal/webhook/...
• go test ./...
• go test -race ./internal/observe/... ./internal/store/... passed internal/observe, then timed out in the existing internal/store suite at Go's 10-minute package timeout
• go test -race ./internal/store -run TestRunAttributionArtifactUpsertAndLookups

[eloylp]

PR Review — pr-reviewer

Verdict: APPROVE

Findings

• None.

Summary

The requested persistence-boundary refactor is resolved: run_attribution_artifacts row types, scans, upsert, and lookup SQL now live in internal/store, while internal/observe validates attribution metadata and calls typed store APIs. The store-boundary docs and tests are in place, and issue #692's artifact-ancestry goals remain covered.

Verified locally:

• go test ./internal/store/... ./internal/observe/... ./internal/webhook/...
• go test -race ./internal/store -run TestRunAttributionArtifactUpsertAndLookups