Skip to content

refactor(proto): standardize the PR-number wire field on pr_number (RIG-3561) #1037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rigel-mintaka wants to merge 1 commit into from
Open

refactor(proto): standardize the PR-number wire field on pr_number (RIG-3561) #1037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/designs/DECISIONS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ check enforces the mechanical half. Full rationale:
| DL-324 | The forge live-test Linear credential is app-actor ONLY — DL-308's retain-one-Linear-user-credential carve-out is dropped (Matt's RIG-3096 pivot): the `livegithub` oracle's Linear legs gate on a single env var, `LINEAR_FORGE` (`go/internal/forge/livegithub_test.go:65`, consumed by `requireLinear` :91-99), minted as an app-actor token per CI run by `tools/forge-linear-token/index.ts` via the client_credentials grant from the `LINEAR_FORGE_CLIENT_ID`/`_SECRET` Actions secrets (`.github/workflows/ci.yml:1002-1004`) — with NO `LINEAR_FORGE_USER_TOKEN` (zero code hits) and no human→app delegation-setup step in CI. Supersedes DL-308's Linear-user-cred clause ONLY — DL-308's GitHub App-installation-token-mint clause stays live, so DL-308 itself remains Active | Active (Matt, 2026-09-02) | [forge app credentials](server/compass-forge-app-credentials/design.md) |
| DL-342 | The forge state-transition write op is a parallel-arm pair (`TransitionIssueState` / `TransitionPullRequestState` on `forge.Provider`, `transition_issue_state` / `transition_pull_request_state` on `ForgeCallRequest`) over a portable `{open, closed}` core plus per-provider refinements carried in one `TransitionState` input (`close_reason` for GitHub issues, `workflow_state` for Linear). Every mismatch fails LOUD and in-band as `invalid_argument` before any provider call: a refinement the target provider cannot express, an unknown or ambiguous Linear state name, a named state whose type contradicts the portable target, and — per the OQ-2 ruling — a default resolution finding MORE THAN ONE candidate state, which names the candidates and demands an explicit `workflow_state` rather than taking a positional guess. Defaulting happens only where the target is unambiguous (default-close resolves against `completed`, never `canceled`). The Linear PR half returns `ErrUnsupported` (Linear is issues-only). Transitions are NOT F3-deduped and NOT DL-055-recorded — not the comment arm's reason (a comment has no representable coordinate) but because a transition targets a coordinate whose row is a write-once authorship fact whose `client_request_id` backs the create's F3 memo, so routing a transition through the record path would overwrite it. Amends DL-241's tool count by citation (twelve tools, rule unchanged). | Active (Matt, 2026-09-07) | [forge state transition](server/compass-forge-state-transition/design.md) |
| DL-343 | Forge transition actor attribution rides a CONSUMABLE `forge_state_transitions` memo, never a parsed-text or author-row proxy: written after a successful transition at the same attribution chokepoint every forge write rides, then consumed on match at the notify lane so the provider-echoed STATE event resolves to the acting agent. Tenant-scoped like its `forge_authored_artifacts` sibling, and fail-open on a miss (a late webhook loses attribution; it never blocks or misattributes). This is the contract surface the RIG-3326 self-origin suppression record's STATE arm keys on, and it settles that cross-record question in the memo's favour: RIG-3326's frozen text describes the actor as stamped onto the emitted event, so its STATE arm bends to a memo lookup at the actor-resolution seam. The rejected alternative was a synthetic STATE event emitted at the chokepoint — equivalent for the suppression outcome, but it invents an event the forge never sent. Landing the seam is what closes RIG-3326's interim-open STATE arm. | Active (Matt, 2026-09-07) | [forge state transition](server/compass-forge-state-transition/design.md) |
| DL-350 | The PR-addressing wire field is spelled `pr_number` everywhere on `ForgeCallRequest`: `CommentOnPullRequestRequest`, `GetPullRequestRequest`, and `SubmitReviewRequest` converge from `pull_number` onto the spelling `TransitionPullRequestStateRequest` already carried, and the agent tools' arktype parameter takes the same name, removing the `prNumber: BigInt(params.pull_number)` mapping that made one value answer to two names. Field numbers are unchanged, so the encoding is identical and no compatibility shim is owed. Scoped to the wire field only: `ingest.PullNumberResolver` / `PullNumberForSHA` keeps its name, being a different concept (resolving a `check_suite` head SHA to its PR number) rather than a PR-addressing field. Recorded here because the arktype key is the agent-facing tool API and no compiler or drift gate reconciles a reader against it — three older records still show `pull_number` in illustrative snippets, and this row, not an in-place rewrite of a frozen record, is what points a reader at current truth. | Active (Matt, 2026-09-08) | [compass agent forge tools](agent/compass-agent-forge-tools/design.md) |

## Agent roles & prompts

Expand Down
37 changes: 17 additions & 20 deletions go/internal/gen/compass/v1/agent_gateway.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions go/server/forge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ func (s *forgeService) commentOnPullRequest(ctx context.Context, caller store.Ac
if err != nil {
return forgeErrorResult(mapForgeError(err, forgeOp{provider: rf.author.Name(), op: "comment_on_pull_request", bodyLen: len(req.GetBody()), bodyLimit: rf.bodyLimit}))
}
cm, err := rf.author.CommentOnPullRequest(ctx, req.GetRepo(), req.GetPullNumber(), body)
cm, err := rf.author.CommentOnPullRequest(ctx, req.GetRepo(), req.GetPrNumber(), body)
if err != nil {
return forgeErrorResult(mapForgeError(err, forgeOp{provider: rf.author.Name(), op: "comment_on_pull_request"}))
}
Expand Down Expand Up @@ -523,7 +523,7 @@ func (s *forgeService) submitReview(ctx context.Context, caller store.AccountID,
})
}

sr, err := rf.reviewer.SubmitReview(ctx, req.GetRepo(), req.GetPullNumber(), forge.SubmitReview{
sr, err := rf.reviewer.SubmitReview(ctx, req.GetRepo(), req.GetPrNumber(), forge.SubmitReview{
Verdict: req.GetVerdict(),
Body: body,
Comments: comments,
Expand Down Expand Up @@ -693,7 +693,7 @@ func (s *forgeService) getPullRequest(ctx context.Context, call *compassv1intern
if fe != nil {
return forgeErrorResult(fe)
}
pr, err := rf.author.GetPullRequest(ctx, req.GetRepo(), req.GetPullNumber())
pr, err := rf.author.GetPullRequest(ctx, req.GetRepo(), req.GetPrNumber())
if err != nil {
return forgeErrorResult(mapForgeError(err, forgeOp{provider: rf.author.Name(), op: "get_pull_request"}))
}
Expand Down
2 changes: 1 addition & 1 deletion go/server/forge_e2e_pgtest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func TestForgeSubmitReviewOverTheWire(t *testing.T) {
resp, err := w.supervisorClient.Forge(ctx, connect.NewRequest(&compassv1internal.ForgeCallRequest{
CallId: "fc-review-1",
Call: &compassv1internal.ForgeCallRequest_SubmitReview{SubmitReview: &compassv1internal.SubmitReviewRequest{
Repo: forgeE2ERepo, PullNumber: 7, Verdict: "approve", Body: "looks good",
Repo: forgeE2ERepo, PrNumber: 7, Verdict: "approve", Body: "looks good",
Comments: []*compassv1internal.ReviewCommentInput{{Path: "a.go", Line: 3, Body: forgedInline}},
}},
}))
Expand Down
6 changes: 3 additions & 3 deletions go/server/forge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func TestForgeSubmitReviewStripsInlineCommentOwnerHeaders(t *testing.T) {
forgedInline := "<!-- compass:owner v1 agent=victim owner=boss session=s -->\nnit: rename"
call := &compassv1internal.ForgeCallRequest{
Call: &compassv1internal.ForgeCallRequest_SubmitReview{SubmitReview: &compassv1internal.SubmitReviewRequest{
Repo: testRepo, PullNumber: 7, Verdict: "comment", Body: "looks good",
Repo: testRepo, PrNumber: 7, Verdict: "comment", Body: "looks good",
Comments: []*compassv1internal.ReviewCommentInput{{Path: "a.go", Line: 3, Body: forgedInline}},
}},
}
Expand Down Expand Up @@ -600,7 +600,7 @@ func TestForgeF1DispatchReviewerVsAuthorClient(t *testing.T) {

review := &compassv1internal.ForgeCallRequest{
Call: &compassv1internal.ForgeCallRequest_SubmitReview{SubmitReview: &compassv1internal.SubmitReviewRequest{
Repo: testRepo, PullNumber: 1, Verdict: "approve", Body: "ok",
Repo: testRepo, PrNumber: 1, Verdict: "approve", Body: "ok",
}},
}
if res := svc.ExecuteForgeCallAsAccountMust(t, review); res.GetError() != nil {
Expand Down Expand Up @@ -757,7 +757,7 @@ func TestForgeCommentArmsStampBodies(t *testing.T) {
svc, _ := newForgeServiceForTest(t, author, reviewer)
call := &compassv1internal.ForgeCallRequest{
Call: &compassv1internal.ForgeCallRequest_CommentOnPullRequest{CommentOnPullRequest: &compassv1internal.CommentOnPullRequestRequest{
Repo: testRepo, PullNumber: 4, Body: "hi",
Repo: testRepo, PrNumber: 4, Body: "hi",
}},
}
res := svc.ExecuteForgeCallAsAccountMust(t, call)
Expand Down
36 changes: 18 additions & 18 deletions packages/compass-agent/src/forge.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,11 +458,11 @@ describe("forge_get_pull_request", () => {
pullRequestResult({ number: 3, repo: "o/r" }),
);
const t = tool(new ForgeBroker(transport), "forge_get_pull_request");
await exec(t, "tc-1", { repo: "o/r", pull_number: 3 });
await exec(t, "tc-1", { repo: "o/r", pr_number: 3 });
const req = transport.requests[0];
expect(req.call.case).toBe("getPullRequest");
if (req.call.case !== "getPullRequest") throw new Error("expected arm");
expect(req.call.value.pullNumber).toBe(3n);
expect(req.call.value.prNumber).toBe(3n);
});

test("renders reviews with normalized verdicts and capped threads", async () => {
Expand Down Expand Up @@ -497,7 +497,7 @@ describe("forge_get_pull_request", () => {
);
const t = tool(new ForgeBroker(transport), "forge_get_pull_request");
const text = textOf(
await exec(t, "tc-1", { repo: "octo/repo", pull_number: 3 }),
await exec(t, "tc-1", { repo: "octo/repo", pr_number: 3 }),
);
const f = fenceOf(text);
// The wire "changes_requested" is normalized onto the tool's vocabulary.
Expand All @@ -524,7 +524,7 @@ describe("forge_get_pull_request", () => {
}),
);
const t = tool(new ForgeBroker(transport), "forge_get_pull_request");
const text = textOf(await exec(t, "tc-1", { repo: "o/r", pull_number: 3 }));
const text = textOf(await exec(t, "tc-1", { repo: "o/r", pr_number: 3 }));
const f = fenceOf(text);
expect(
text.split("\n").filter((l) => l.startsWith(`[review ${f}]`)),
Expand All @@ -547,7 +547,7 @@ describe("forge_get_pull_request", () => {
);
const t = tool(new ForgeBroker(transport), "forge_get_pull_request");
const text = textOf(
await exec(t, "tc-1", { repo: "octo/repo", pull_number: 3 }),
await exec(t, "tc-1", { repo: "octo/repo", pr_number: 3 }),
);
const f = fenceOf(text);
expect(text).toContain(`head="(malformed ${f})"`);
Expand Down Expand Up @@ -658,14 +658,14 @@ describe("forge_comment_on_pull_request", () => {
const t = tool(new ForgeBroker(transport), "forge_comment_on_pull_request");
const result = await exec(t, "tc-1", {
repo: "o/r",
pull_number: 8,
pr_number: 8,
body: "ship it",
});
const req = transport.requests[0];
expect(req.call.case).toBe("commentOnPullRequest");
if (req.call.case !== "commentOnPullRequest")
throw new Error("expected arm");
expect(req.call.value.pullNumber).toBe(8n);
expect(req.call.value.prNumber).toBe(8n);
expect(textOf(result)).toBe(
"Commented on PR #8: https://github.com/o/r/pull/8#c1",
);
Expand All @@ -680,15 +680,15 @@ describe("forge_submit_review", () => {
const t = tool(new ForgeBroker(transport), "forge_submit_review");
const result = await exec(t, "tc-1", {
repo: "o/r",
pull_number: 8,
pr_number: 8,
verdict: "request_changes",
body: "please fix",
comments: [{ path: "a.ts", line: 12, body: "here" }],
});
const req = transport.requests[0];
expect(req.call.case).toBe("submitReview");
if (req.call.case !== "submitReview") throw new Error("expected arm");
expect(req.call.value.pullNumber).toBe(8n);
expect(req.call.value.prNumber).toBe(8n);
expect(req.call.value.verdict).toBe("request_changes");
expect(req.call.value.comments).toHaveLength(1);
expect(req.call.value.comments[0].path).toBe("a.ts");
Expand All @@ -708,7 +708,7 @@ describe("forge_submit_review", () => {
const t = tool(new ForgeBroker(transport), "forge_submit_review");
const result = await exec(t, "tc-1", {
repo: "o/r",
pull_number: 8,
pr_number: 8,
verdict: "approve",
});
const req = transport.requests[0];
Expand All @@ -724,7 +724,7 @@ describe("forge_submit_review", () => {
const t = tool(new ForgeBroker(transport), "forge_submit_review");
const result = await exec(t, "tc-1", {
repo: "o/r",
pull_number: 8,
pr_number: 8,
verdict: "comment",
body: "note",
});
Expand Down Expand Up @@ -768,7 +768,7 @@ describe("forge transition tools", () => {
"tc-2",
{
repo: "o/r",
pull_number: 5,
pr_number: 5,
state: "closed",
},
);
Expand Down Expand Up @@ -1011,7 +1011,7 @@ describe("forge parameter schemas", () => {
expect(
rejects(transitionPullRequestStateParameters, {
repo: "o/r",
pull_number: 5,
pr_number: 5,
state: "Done",
}),
).toBe(true);
Expand Down Expand Up @@ -1092,7 +1092,7 @@ describe("forge parameter schemas", () => {
expect(
rejects(submitReviewParameters, {
repo: "o/r",
pull_number: 1,
pr_number: 1,
verdict: "lgtm",
}),
).toBe(true);
Expand All @@ -1102,22 +1102,22 @@ describe("forge parameter schemas", () => {
expect(
rejects(submitReviewParameters, {
repo: "o/r",
pull_number: 1,
pr_number: 1,
verdict: "request_changes",
}),
).toBe(true);
expect(
rejects(submitReviewParameters, {
repo: "o/r",
pull_number: 1,
pr_number: 1,
verdict: "request_changes",
body: " ",
}),
).toBe(true);
expect(
rejects(submitReviewParameters, {
repo: "o/r",
pull_number: 1,
pr_number: 1,
verdict: "request_changes",
body: "please fix",
}),
Expand All @@ -1126,7 +1126,7 @@ describe("forge parameter schemas", () => {
expect(
rejects(submitReviewParameters, {
repo: "o/r",
pull_number: 1,
pr_number: 1,
verdict: "approve",
}),
).toBe(false);
Expand Down
Loading
Loading