From 9ac7a0f18b7d37496e3af9eff519db3b2c3a4151 Mon Sep 17 00:00:00 2001 From: Dinesh Pandiyan Date: Sun, 25 Oct 2026 14:35:35 +1100 Subject: [PATCH] feat: pull request metadata contract and additive schema 12 - optional protocol 4 metadata.pullRequest, server-authored only - public ingestion rejects github source, trigger and pullRequest - current heads project winning submission's pullRequest - schema 11 upgrades in place to 12 (additive-only policy) --- docs-website/src/pages/api.md | 2 +- docs/architecture.md | 22 ++++--- docs/development.md | 4 +- docs/release.md | 7 +-- docs/system.md | 8 +++ internal/ingestion/model.go | 37 +++++++++++ internal/ingestion/validate.go | 46 +++++++++++++- internal/ingestion/validate_test.go | 65 +++++++++++++++++++ internal/reviewstore/catalog.go | 15 ++++- internal/reviewstore/pull_request_test.go | 70 +++++++++++++++++++++ internal/reviewstore/reset_test.go | 34 +++++++++- internal/reviewstore/store.go | 14 +++-- internal/serverapp/handler.go | 6 ++ internal/serverapp/handler_test.go | 22 +++++++ packages/api/openapi.yaml | 76 +++++++++++++++++++++++ packages/api/src/schema.d.ts | 38 ++++++++++++ 16 files changed, 442 insertions(+), 24 deletions(-) create mode 100644 internal/reviewstore/pull_request_test.go diff --git a/docs-website/src/pages/api.md b/docs-website/src/pages/api.md index 18fe4433..b57feaf0 100644 --- a/docs-website/src/pages/api.md +++ b/docs-website/src/pages/api.md @@ -35,7 +35,7 @@ Local data lives in `$XDG_STATE_HOME/diffx/state.db`, or `~/.local/state/diffx/state.db`. An isolated `DIFFX_RUNTIME_DIR` moves the default database, manual sync and hook state, and config into that directory. `--state memory` disables persistence. -Current schema 11 state persists across restarts. When local state comes from +Current schema 12 state persists across restarts; schema 11 state upgrades in place. When local state comes from an older diffx, `diffx` asks before moving it to a backup beside the database and starting fresh; scripts pass `--reset-state`, which backs up and resets current state too. Only the newest backup is kept. Newer or unrecognized diff --git a/docs/architecture.md b/docs/architecture.md index dc7cad5c..055c137a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -188,11 +188,15 @@ uses `X-Diffx-State` to pin submission and polling to the destination identity; older protocols are rejected. Release artifacts use the explicit `diffx` project name, independent of the checkout directory. -Schema 11 preserves current state across starts. It moved review state from -each snapshot scope to its stream and scope, because comments and reviewed marks -must survive new collections and snapshot deletion. Older, future and unrecognized -schemas are refused; opening a database never resets it. No automatic schema -migration is implemented. `reviewstore.Reset` is the only reset: under the +Schema 12 preserves current state across starts. Schema 11 moved review state +from each snapshot scope to its stream and scope, because comments and reviewed +marks must survive new collections and snapshot deletion. Schema 11 and later +upgrade in place: later schemas only add tables and indexes, so opening runs the +idempotent creation statements and records the new version in one transaction, +because durable server volumes must gain GitHub integration state without an +operator reset. A schema change that alters existing tables needs a new +compatibility floor instead. Older-than-11, future and unrecognized schemas are +refused; opening a database never resets it. `reviewstore.Reset` is the only reset: under the ownership lock it moves current or older diffx state and its SQLite sidecars to a timestamped backup beside the database, keeping only the newest backup, and refuses newer or unrecognized files. Only local composition decides to call it, @@ -207,8 +211,12 @@ state also suggest `--reset-state`. Fresh state has no saved reviews or server accounts. Install the server first when adding optional protocol 4 metadata: older servers reject unknown request fields and new diff scopes. Captured comparison kinds, -the `uncommitted` and `committed` scopes, and activity metadata retain protocol 4. -Existing saved submissions remain replayable and schema 11 state needs no reset. +the `uncommitted` and `committed` scopes, activity and pull request metadata +retain protocol 4. Public ingestion rejects the `github` source, the `github` +trigger and `pullRequest` metadata at the transport, because only the server's +GitHub integration may claim a hosted pull request; shared validation accepts +them for that internal admission path. +Existing saved submissions remain replayable and schema 11 state upgrades without reset. Legacy comparison kinds are interpreted on read; stored payloads and their retry identities are not rewritten. Upgrade collectors after the server. Versioned producer directories ignore old manual/hook pending payloads. diff --git a/docs/development.md b/docs/development.md index 11ec08b8..470e99c1 100644 --- a/docs/development.md +++ b/docs/development.md @@ -142,8 +142,8 @@ runtime directories (`DIFFX_RUNTIME_DIR`) and in-memory state for lifecycle test must not register inputs in the personal service. `DIFFX_EXIT_ON_STDIN_CLOSE` is a foreground development-process lifecycle hook. -Stop running processes before upgrading. Current schema 11 databases survive -restart; incompatible schemas are refused without resetting data. Local +Stop running processes before upgrading. Schema 11 databases upgrade in place to +schema 12 and survive restart; incompatible schemas are refused without resetting data. Local `diffx` offers to back up older state and start fresh on a terminal; `--reset-state` does so without asking, keeping only the newest backup beside the database. Server state never resets: back up the state directory while diff --git a/docs/release.md b/docs/release.md index a963c829..da7bc6c3 100644 --- a/docs/release.md +++ b/docs/release.md @@ -41,15 +41,14 @@ arm64. Windows archives are also published. Archives include the native binary, README, and license. The embedded web application needs no installed Node.js runtime. -Stop running binaries before upgrading. Current schema 11 state survives -subsequent starts; older, future or unrecognized schemas are refused without +Stop running binaries before upgrading. Schema 11 state upgrades in place to +schema 12 and survives subsequent starts; older, future or unrecognized schemas are refused without resetting data. Local `diffx` offers to back up older state beside it and start fresh, or does so with `--reset-state`. For servers, back up the state directory and explicitly choose a fresh database path if discarding incompatible state. Upgrade server and collectors together for ingestion protocol 4, deploying the server first: new producers send optional activity metadata that older servers reject. Existing protocol 4 -submissions without that metadata still replay unchanged; schema 11 needs no -reset for activity ordering. In-memory state lasts only for one process. +submissions without that metadata still replay unchanged and need no reset. In-memory state lasts only for one process. The tap branch is deterministic per version, such as `diffx-v0.2.0`. Rerunning a release whose tag's first parent is still current `main` reuses the existing diff --git a/docs/system.md b/docs/system.md index 6e513bed..6c01cc26 100644 --- a/docs/system.md +++ b/docs/system.md @@ -207,6 +207,14 @@ head selection. Unchanged submissions can update ordering without producing an unread-content indicator. Piped history and the full catalog retain their existing collection/submission chronology. +Optional `pullRequest` metadata (title, description, author, state, refs and +URL of a hosted pull request) follows the same rule: a current head shows its +winning submission's value, so a title edit or merge that leaves the diff +unchanged reuses the observation and still updates what the head shows, while +historical snapshots keep their captured value. It does not participate in +content identity. Only the server's GitHub integration authors it; public +ingestion rejects it with the reserved `github` source and trigger. + The review picker, opened by Cmd/Ctrl+K or the header's “Search reviews” action, loads the full catalog only while open. It selects a repository, then an observation identified by worktree, branch, source, run and time. Distinct diff --git a/internal/ingestion/model.go b/internal/ingestion/model.go index f619841c..c2cc414e 100644 --- a/internal/ingestion/model.go +++ b/internal/ingestion/model.go @@ -47,6 +47,43 @@ type Metadata struct { Comparison *Comparison `json:"comparison,omitempty"` AgentSession *AgentSession `json:"agentSession,omitempty"` TriggerRoot string `json:"triggerRoot,omitempty"` + PullRequest *PullRequest `json:"pullRequest,omitempty"` +} + +// PullRequest describes the hosted pull request a capture reviews. Only the +// server's GitHub integration authors it; public ingestion rejects it. +type PullRequest struct { + Provider string `json:"provider"` + RepositoryID int64 `json:"repositoryId"` + FullName string `json:"fullName"` + Number int `json:"number"` + URL string `json:"url"` + Title string `json:"title"` + Body string `json:"body"` + State string `json:"state"` + Draft bool `json:"draft"` + Author PullRequestAuthor `json:"author"` + BaseRef string `json:"baseRef"` + HeadRef string `json:"headRef"` + HeadRepository string `json:"headRepository"` + CreatedAt int64 `json:"createdAt"` + UpdatedAt int64 `json:"updatedAt"` +} + +type PullRequestAuthor struct { + Login string `json:"login"` + URL string `json:"url"` +} + +// Server-authored producer markers; public ingestion rejects both. +const ( + GitHubSourceID = "github" + GitHubTrigger = "github" +) + +// Reserved reports metadata only the server's GitHub integration may submit. +func (metadata Metadata) Reserved() bool { + return metadata.PullRequest != nil || metadata.SourceID == GitHubSourceID || metadata.Trigger == GitHubTrigger } type Scope struct { diff --git a/internal/ingestion/validate.go b/internal/ingestion/validate.go index e7fdd5c2..0b12389a 100644 --- a/internal/ingestion/validate.go +++ b/internal/ingestion/validate.go @@ -29,8 +29,11 @@ func Validate(input Request) error { if input.Metadata.ActivityAt < 0 { return fmt.Errorf("activityAt must be nonnegative") } - if input.Metadata.Trigger != "manual" && input.Metadata.Trigger != "agent-hook" && input.Metadata.Trigger != "pipe" { - return fmt.Errorf("trigger must be manual, agent-hook, or pipe") + if input.Metadata.Trigger != "manual" && input.Metadata.Trigger != "agent-hook" && input.Metadata.Trigger != "pipe" && input.Metadata.Trigger != GitHubTrigger { + return fmt.Errorf("trigger must be manual, agent-hook, pipe, or github") + } + if err := validatePullRequest(input.Metadata); err != nil { + return err } values := []string{input.Metadata.Hostname, input.Metadata.RunID, input.Metadata.Agent, input.Metadata.RepositoryKey, input.Metadata.RepositoryName, input.Metadata.RemoteURL, input.Metadata.CheckoutKey, input.Metadata.Root, input.Metadata.WorktreeName, input.Metadata.Branch, input.Metadata.BranchID, input.Metadata.CollectorVersion, input.Metadata.TriggerRoot} if comparison := input.Metadata.Comparison; comparison != nil { @@ -151,6 +154,45 @@ func validateComparisonKind(snapshot review.RepositoryDiff, metadata Metadata) e return nil } +// A pull request capture is always a GitHub-triggered branch comparison. +func validatePullRequest(metadata Metadata) error { + pull := metadata.PullRequest + if (pull != nil) != (metadata.Trigger == GitHubTrigger) { + return fmt.Errorf("pullRequest metadata and github trigger must be supplied together") + } + if pull == nil { + return nil + } + if metadata.SourceID != GitHubSourceID || metadata.Comparison == nil || metadata.Comparison.Kind != "branch" { + return fmt.Errorf("pullRequest requires the github source and a branch comparison") + } + owner, name, ok := strings.Cut(pull.FullName, "/") + if pull.Provider != "github" || pull.RepositoryID <= 0 || pull.Number <= 0 || !ok || owner == "" || name == "" || strings.Contains(name, "/") { + return fmt.Errorf("pullRequest requires github provider, repositoryId, owner/name and number") + } + if pull.URL != fmt.Sprintf("https://github.com/%s/pull/%d", pull.FullName, pull.Number) { + return fmt.Errorf("pullRequest url must address the pull request on github.com") + } + if pull.State != "open" && pull.State != "closed" && pull.State != "merged" { + return fmt.Errorf("pullRequest state must be open, closed, or merged") + } + if pull.Author.Login == "" || pull.Author.URL != "https://github.com/"+pull.Author.Login { + return fmt.Errorf("pullRequest author requires login and github.com profile url") + } + if pull.CreatedAt < 0 || pull.UpdatedAt < pull.CreatedAt { + return fmt.Errorf("pullRequest times must be nonnegative and ordered") + } + if len(pull.Body) > 256<<10 { + return fmt.Errorf("pullRequest body exceeds 256 KiB") + } + for _, value := range []string{pull.FullName, pull.Title, pull.Author.Login, pull.BaseRef, pull.HeadRef, pull.HeadRepository} { + if len(value) > 32<<10 { + return fmt.Errorf("metadata value exceeds 32 KiB") + } + } + return nil +} + func validCommit(value string) bool { if (len(value) != 40 && len(value) != 64) || strings.ToLower(value) != value { return false diff --git a/internal/ingestion/validate_test.go b/internal/ingestion/validate_test.go index 60dd6510..6c7f8170 100644 --- a/internal/ingestion/validate_test.go +++ b/internal/ingestion/validate_test.go @@ -105,6 +105,71 @@ func TestValidateStructuredMetadataAndLegacyFallback(t *testing.T) { } } +func pullRequestRequest() Request { + r := validRequest() + head := strings.Repeat("c", 40) + r.Metadata.SourceID, r.Metadata.Trigger, r.Metadata.Branch, r.Metadata.Head = GitHubSourceID, GitHubTrigger, "feature", &head + r.Scopes[0].Snapshot.Branch, r.Scopes[0].Snapshot.Head = "feature", &head + r.Metadata.Comparison = &Comparison{Kind: "branch", BaseRef: "refs/heads/main", BaseCommit: strings.Repeat("a", 40), MergeBase: strings.Repeat("b", 40)} + r.Metadata.PullRequest = &PullRequest{Provider: "github", RepositoryID: 7, FullName: "octo/app", Number: 12, URL: "https://github.com/octo/app/pull/12", + Title: "Add feature", Body: "Details", State: "open", Author: PullRequestAuthor{Login: "mona", URL: "https://github.com/mona"}, + BaseRef: "main", HeadRef: "feature", HeadRepository: "mona/app", CreatedAt: 1, UpdatedAt: 2} + return r +} + +func TestValidatePullRequestMetadata(t *testing.T) { + if err := Validate(pullRequestRequest()); err != nil { + t.Fatalf("valid pull request: %v", err) + } + cases := []struct { + name string + change func(*Request) + }{ + {"without github trigger", func(r *Request) { r.Metadata.Trigger = "manual" }}, + {"github trigger without pull request", func(r *Request) { r.Metadata.PullRequest = nil }}, + {"foreign source", func(r *Request) { r.Metadata.SourceID = "laptop" }}, + {"working-tree comparison", func(r *Request) { r.Metadata.Comparison = &Comparison{Kind: "working-tree", BaseRef: "HEAD"} }}, + {"unknown provider", func(r *Request) { r.Metadata.PullRequest.Provider = "gitlab" }}, + {"missing repository ID", func(r *Request) { r.Metadata.PullRequest.RepositoryID = 0 }}, + {"missing number", func(r *Request) { r.Metadata.PullRequest.Number = 0 }}, + {"name without owner", func(r *Request) { r.Metadata.PullRequest.FullName = "app" }}, + {"foreign url", func(r *Request) { r.Metadata.PullRequest.URL = "https://example.com/octo/app/pull/12" }}, + {"url for another number", func(r *Request) { r.Metadata.PullRequest.URL = "https://github.com/octo/app/pull/13" }}, + {"unknown state", func(r *Request) { r.Metadata.PullRequest.State = "draft" }}, + {"missing author", func(r *Request) { r.Metadata.PullRequest.Author = PullRequestAuthor{} }}, + {"foreign author url", func(r *Request) { r.Metadata.PullRequest.Author.URL = "javascript:alert(1)" }}, + {"updated before created", func(r *Request) { r.Metadata.PullRequest.UpdatedAt = 0 }}, + {"oversized body", func(r *Request) { r.Metadata.PullRequest.Body = strings.Repeat("x", (256<<10)+1) }}, + {"oversized title", func(r *Request) { r.Metadata.PullRequest.Title = strings.Repeat("x", (32<<10)+1) }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := pullRequestRequest() + tc.change(&r) + if err := Validate(r); err == nil { + t.Fatal("invalid pull request accepted") + } + }) + } +} + +func TestReservedMetadata(t *testing.T) { + if validRequest().Metadata.Reserved() { + t.Fatal("ordinary producer metadata reserved") + } + for _, change := range []func(*Metadata){ + func(m *Metadata) { m.SourceID = GitHubSourceID }, + func(m *Metadata) { m.Trigger = GitHubTrigger }, + func(m *Metadata) { m.PullRequest = &PullRequest{} }, + } { + metadata := validRequest().Metadata + change(&metadata) + if !metadata.Reserved() { + t.Fatalf("server-authored metadata not reserved: %+v", metadata) + } + } +} + func TestValidateEmptyAndUnavailableSnapshots(t *testing.T) { r := validRequest() r.ContentHash = strings.Repeat("a", 64) diff --git a/internal/reviewstore/catalog.go b/internal/reviewstore/catalog.go index 026bccae..854466e0 100644 --- a/internal/reviewstore/catalog.go +++ b/internal/reviewstore/catalog.go @@ -61,7 +61,8 @@ var contextSelect = `SELECT c.id,c.kind,json_extract(o.metadata,'$.root'),NULL,o FROM observation_sessions a JOIN agent_sessions s ON s.owner_id=a.owner_id AND s.source_id=a.source_id AND s.harness=a.harness AND s.session_id=a.session_id WHERE a.context_id=c.id), head.source_id,head.repository_key,head.checkout_key,head.branch,head.comparison_policy, (SELECT hc.id FROM contexts hc JOIN diffs hd ON hd.id=hc.anchor_diff_id WHERE hc.owner_id=c.owner_id AND hc.id=head.context_id - AND (hd.expires_at IS NULL OR hd.expires_at>CAST(strftime('%s','now') AS INTEGER)*1000)) + AND (hd.expires_at IS NULL OR hd.expires_at>CAST(strftime('%s','now') AS INTEGER)*1000)), + CASE WHEN head.context_id=c.id THEN json_extract(head_submission.metadata,'$.pullRequest') END FROM contexts c LEFT JOIN diffs d ON d.id=c.anchor_diff_id LEFT JOIN observations o ON o.context_id=c.id LEFT JOIN observation_stream_heads head ON head.owner_id=o.owner_id AND head.source_id=o.source_id @@ -85,9 +86,10 @@ func scanContext(row scanner) (ContextInfo, error) { var metadata *string var sessions, summaries string var head [5]sql.NullString + var headPull sql.NullString err := row.Scan(&item.ID, &item.Kind, &item.Root, &item.LocationID, &item.RepositoryID, &item.CommonDir, &item.WorktreeKey, &item.SubmittedFrom, &item.CreatedAt, &item.LastSubmittedAt, &item.ExpiresAt, &item.ActivityAt, &item.ChangedFileCount, &summaries, &item.Source, &metadata, &item.Stale, &sessions, - &head[0], &head[1], &head[2], &head[3], &head[4], &item.LatestContextID) + &head[0], &head[1], &head[2], &head[3], &head[4], &item.LatestContextID, &headPull) if errors.Is(err, sql.ErrNoRows) { return ContextInfo{}, ErrNotFound } @@ -105,6 +107,15 @@ func scanContext(row scanner) (ContextInfo, error) { if err := json.Unmarshal([]byte(*metadata), &decoded); err != nil { return ContextInfo{}, err } + // A current head shows its winning submission's pull request, so edits + // and state changes appear without changing captured metadata. + if headPull.Valid { + var pull ingestion.PullRequest + if err := json.Unmarshal([]byte(headPull.String), &pull); err != nil { + return ContextInfo{}, err + } + decoded.PullRequest = &pull + } item.Metadata = &decoded item.Kind = "observation" } diff --git a/internal/reviewstore/pull_request_test.go b/internal/reviewstore/pull_request_test.go new file mode 100644 index 00000000..989b96c0 --- /dev/null +++ b/internal/reviewstore/pull_request_test.go @@ -0,0 +1,70 @@ +package reviewstore + +import ( + "strings" + "testing" + "time" + + "github.com/flexdinesh/diffx/internal/ingestion" +) + +func TestPullRequestProjectionFollowsWinningSubmission(t *testing.T) { + store := openTestStore(t) + owner := testUser(t, store, "pull-requests") + title := func(id string) (string, string) { + t.Helper() + item, err := store.Context(t.Context(), owner.ID, id, time.Now()) + if err != nil || item.Metadata == nil || item.Metadata.PullRequest == nil { + t.Fatalf("pull request projection: %+v, %v", item, err) + } + return item.Metadata.PullRequest.Title, item.Metadata.PullRequest.State + } + request := queuedRequest("opened") + request.ContentHash = strings.Repeat("a", 64) + request.Metadata.CollectedAt = 100 + request.Metadata.PullRequest = &ingestion.PullRequest{Provider: "github", RepositoryID: 7, FullName: "octo/app", Number: 12, Title: "Draft title", State: "open"} + original, err := store.Ingest(owner.ID, request) + if err != nil { + t.Fatal(err) + } + edited := request + edited.SubmissionID = "edited" + edited.Metadata.CollectedAt = 200 + pull := *request.Metadata.PullRequest + pull.Title, pull.State = "Final title", "merged" + edited.Metadata.PullRequest = &pull + reused, err := store.Ingest(owner.ID, edited) + if err != nil || reused.ContextID != original.ContextID { + t.Fatalf("metadata-only edit must reuse the observation: %+v, %v", reused, err) + } + if got, state := title(original.ContextID); got != "Final title" || state != "merged" { + t.Fatalf("head shows %q %q, want latest submission", got, state) + } + // A delayed submission collected earlier never replaces the head's metadata. + late := request + late.SubmissionID = "late" + late.Metadata.CollectedAt = 150 + if _, err := store.Ingest(owner.ID, late); err != nil { + t.Fatal(err) + } + if got, _ := title(original.ContextID); got != "Final title" { + t.Fatalf("late submission replaced head metadata: %q", got) + } + pushed := edited + pushed.SubmissionID, pushed.ContentHash = "pushed", strings.Repeat("b", 64) + pushed.Metadata.CollectedAt = 300 + pushedPull := pull + pushedPull.Title = "After push" + pushed.Metadata.PullRequest = &pushedPull + latest, err := store.Ingest(owner.ID, pushed) + if err != nil || latest.ContextID == original.ContextID { + t.Fatalf("new content must create a snapshot: %+v, %v", latest, err) + } + // Historical snapshots keep their captured metadata. + if got, _ := title(original.ContextID); got != "Draft title" { + t.Fatalf("stale snapshot shows %q, want captured title", got) + } + if got, _ := title(latest.ContextID); got != "After push" { + t.Fatalf("latest snapshot shows %q", got) + } +} diff --git a/internal/reviewstore/reset_test.go b/internal/reviewstore/reset_test.go index fef31200..5c09b445 100644 --- a/internal/reviewstore/reset_test.go +++ b/internal/reviewstore/reset_test.go @@ -4,6 +4,7 @@ import ( "bytes" "database/sql" "errors" + "fmt" "os" "path/filepath" "strings" @@ -26,7 +27,7 @@ func TestOlderSchemaRefusedWithoutReset(t *testing.T) { store.Close() t.Fatal("accepted older schema") } else { - for _, text := range []string{path, "schema 10 is older than supported schema 11", "supporting schema 10", "automatic reset is disabled", "back up", "SQLite sidecar files", "--state /path/to/new-state.db", "no saved reviews or server accounts"} { + for _, text := range []string{path, "schema 10 is older than supported schema 12", "supporting schema 10", "automatic reset is disabled", "back up", "SQLite sidecar files", "--state /path/to/new-state.db", "no saved reviews or server accounts"} { if !strings.Contains(err.Error(), text) { t.Fatalf("missing recovery guidance %q: %v", text, err) } @@ -48,6 +49,35 @@ func TestOlderSchemaRefusedWithoutReset(t *testing.T) { } } +func TestAdditiveSchemaUpgradesInPlace(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.db") + store, err := Open(path) + if err != nil { + t.Fatal(err) + } + user, err := store.User("owner", "owner") + if err != nil { + t.Fatal(err) + } + if _, err := store.db.Exec("PRAGMA user_version=11"); err != nil { + t.Fatal(err) + } + store.Close() + store, err = Open(path) + if err != nil { + t.Fatalf("schema 11 must upgrade in place: %v", err) + } + defer store.Close() + var version int + if err := store.db.QueryRow("PRAGMA user_version").Scan(&version); err != nil || version != schemaVersion { + t.Fatalf("upgraded version %d %v", version, err) + } + again, err := store.User("owner", "owner") + if err != nil || again.ID != user.ID { + t.Fatalf("upgrade lost state: %+v %v", again, err) + } +} + func TestCurrentSchemaSurvivesRestartAndFutureVersionRefused(t *testing.T) { path := filepath.Join(t.TempDir(), "state.db") store, err := Open(path) @@ -184,7 +214,7 @@ func TestResetBacksUpOlderStateBesideItAndKeepsLatestBackup(t *testing.T) { if err != nil { t.Fatalf("current schema not reset: %v", err) } - if !strings.HasPrefix(filepath.Base(second), "state.db.schema11-") { + if !strings.HasPrefix(filepath.Base(second), fmt.Sprintf("state.db.schema%d-", schemaVersion)) { t.Fatalf("unexpected backup path %q", second) } for _, pruned := range []string{first, first + "-shm"} { diff --git a/internal/reviewstore/store.go b/internal/reviewstore/store.go index 5ba69161..e630c080 100644 --- a/internal/reviewstore/store.go +++ b/internal/reviewstore/store.go @@ -17,7 +17,13 @@ import ( _ "modernc.org/sqlite" ) -const schemaVersion = 11 +const schemaVersion = 12 + +// additiveSchema is the oldest schema that opens by upgrading in place. Each +// later schema only creates tables and indexes, so the idempotent initialization +// statements migrate it in the same transaction that records the new version. +const additiveSchema = 11 + const DefaultRetention = 7 * 24 * time.Hour var ErrNotFound = reviewdata.ErrNotFound @@ -148,9 +154,9 @@ type StateError struct { reason string } -// Older reports diffx state written by an earlier schema. +// Older reports diffx state written by an earlier schema that cannot upgrade in place. func (failure *StateError) Older() bool { - return failure.Version > 0 && failure.Version < schemaVersion + return failure.Version > 0 && failure.Version < additiveSchema } // Supported is the schema this binary opens. @@ -176,7 +182,7 @@ func (store *Store) initialize(path string) error { if version > schemaVersion { return newerStateError(path, version) } - if version > 0 && version < schemaVersion { + if version > 0 && version < additiveSchema { return &StateError{Path: path, Version: version, reason: fmt.Sprintf("diffx state schema %d is older than supported schema %d; to keep existing data, use a diffx binary supporting schema %d", version, schemaVersion, version)} } if _, err := store.db.Exec("PRAGMA foreign_keys=OFF"); err != nil { diff --git a/internal/serverapp/handler.go b/internal/serverapp/handler.go index 48608c6c..c9741083 100644 --- a/internal/serverapp/handler.go +++ b/internal/serverapp/handler.go @@ -145,6 +145,12 @@ func decodeIngestion(service *contextservice.Service, w http.ResponseWriter, r * problem(w, 400, err.Error()) return ingestion.Request{}, false } + // Producers cannot claim a hosted pull request; only the server's GitHub + // integration admits that provenance. + if input.Metadata.Reserved() { + problem(w, 400, "github source, trigger and pullRequest metadata are reserved for the server's GitHub integration") + return ingestion.Request{}, false + } return input, true } func stream(ctx context.Context, service *contextservice.Service, w http.ResponseWriter, r *http.Request) { diff --git a/internal/serverapp/handler_test.go b/internal/serverapp/handler_test.go index 7b3fc8b8..165e3f0d 100644 --- a/internal/serverapp/handler_test.go +++ b/internal/serverapp/handler_test.go @@ -316,6 +316,26 @@ func TestIngestionHTTPRejectsMalformedAndAmbiguousInput(t *testing.T) { if err != nil { t.Fatal(err) } + reserved := emptyRequest() + reserved.Metadata.SourceID = ingestion.GitHubSourceID + reservedRaw, err := json.Marshal(reserved) + if err != nil { + t.Fatal(err) + } + pull := emptyRequest() + head := strings.Repeat("c", 40) + pull.Metadata.SourceID, pull.Metadata.Trigger, pull.Metadata.Branch, pull.Metadata.Head = ingestion.GitHubSourceID, ingestion.GitHubTrigger, "feature", &head + pull.Scopes[0].Snapshot.Branch, pull.Scopes[0].Snapshot.Head = "feature", &head + pull.Metadata.Comparison = &ingestion.Comparison{Kind: "branch", BaseRef: "refs/heads/main", BaseCommit: strings.Repeat("a", 40), MergeBase: strings.Repeat("b", 40)} + pull.Metadata.PullRequest = &ingestion.PullRequest{Provider: "github", RepositoryID: 7, FullName: "octo/app", Number: 12, URL: "https://github.com/octo/app/pull/12", + Title: "Spoofed", State: "open", Author: ingestion.PullRequestAuthor{Login: "mona", URL: "https://github.com/mona"}} + if err := ingestion.Validate(pull); err != nil { + t.Fatalf("fixture must be otherwise valid: %v", err) + } + pullRaw, err := json.Marshal(pull) + if err != nil { + t.Fatal(err) + } cases := []struct { name, method, contentType, body string status int @@ -328,6 +348,8 @@ func TestIngestionHTTPRejectsMalformedAndAmbiguousInput(t *testing.T) { {"trailing JSON", "POST", "application/json", string(raw) + ` {}`, 400}, {"trailing garbage", "POST", "application/json", string(raw) + ` x`, 400}, {"unsupported protocol", "POST", "application/json", string(unsupportedRaw), 400}, + {"reserved github source", "POST", "application/json", string(reservedRaw), 400}, + {"producer-claimed pull request", "POST", "application/json", string(pullRaw), 400}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/packages/api/openapi.yaml b/packages/api/openapi.yaml index dd1c8a20..7a117e52 100644 --- a/packages/api/openapi.yaml +++ b/packages/api/openapi.yaml @@ -1027,6 +1027,80 @@ components: type: string name: type: string + PullRequest: + type: object + additionalProperties: false + description: Hosted pull request a capture reviews. Authored only by the server's GitHub integration; public ingestion rejects it. A current stream head shows its winning submission's value, so edits and state changes appear without new content; older snapshots keep their captured value. + required: + [ + provider, + repositoryId, + fullName, + number, + url, + title, + body, + state, + draft, + author, + baseRef, + headRef, + headRepository, + createdAt, + updatedAt, + ] + properties: + provider: + type: string + enum: [github] + repositoryId: + type: integer + format: int64 + minimum: 1 + fullName: + type: string + description: Base repository as owner/name. + number: + type: integer + minimum: 1 + url: + type: string + description: https://github.com/{fullName}/pull/{number}. + title: + type: string + body: + type: string + description: Untrusted Markdown description; clients must render it without raw HTML. + state: + type: string + enum: [open, closed, merged] + draft: + type: boolean + author: + type: object + additionalProperties: false + required: [login, url] + properties: + login: + type: string + url: + type: string + description: https://github.com/{login}. + baseRef: + type: string + headRef: + type: string + headRepository: + type: string + description: Head repository as owner/name; differs from fullName for forks. + createdAt: + type: integer + format: int64 + description: Unix milliseconds. + updatedAt: + type: integer + format: int64 + description: Unix milliseconds. SessionAssociation: type: object additionalProperties: false @@ -1120,6 +1194,8 @@ components: triggerRoot: type: string description: Directory whose manual command or agent event triggered collection; does not establish authorship. + pullRequest: + $ref: "#/components/schemas/PullRequest" IngestionScope: type: object additionalProperties: false diff --git a/packages/api/src/schema.d.ts b/packages/api/src/schema.d.ts index df1ccd84..473797cc 100644 --- a/packages/api/src/schema.d.ts +++ b/packages/api/src/schema.d.ts @@ -579,6 +579,43 @@ export interface components { id: string; name?: string; }; + /** @description Hosted pull request a capture reviews. Authored only by the server's GitHub integration; public ingestion rejects it. A current stream head shows its winning submission's value, so edits and state changes appear without new content; older snapshots keep their captured value. */ + PullRequest: { + /** @enum {string} */ + provider: "github"; + /** Format: int64 */ + repositoryId: number; + /** @description Base repository as owner/name. */ + fullName: string; + number: number; + /** @description https://github.com/{fullName}/pull/{number}. */ + url: string; + title: string; + /** @description Untrusted Markdown description; clients must render it without raw HTML. */ + body: string; + /** @enum {string} */ + state: "open" | "closed" | "merged"; + draft: boolean; + author: { + login: string; + /** @description https://github.com/{login}. */ + url: string; + }; + baseRef: string; + headRef: string; + /** @description Head repository as owner/name; differs from fullName for forks. */ + headRepository: string; + /** + * Format: int64 + * @description Unix milliseconds. + */ + createdAt: number; + /** + * Format: int64 + * @description Unix milliseconds. + */ + updatedAt: number; + }; SessionAssociation: { sourceId: string; harness: string; @@ -624,6 +661,7 @@ export interface components { agentSession?: components["schemas"]["AgentSession"]; /** @description Directory whose manual command or agent event triggered collection; does not establish authorship. */ triggerRoot?: string; + pullRequest?: components["schemas"]["PullRequest"]; }; IngestionScope: { snapshot: components["schemas"]["RepositoryDiff"];