fix: route pull-request mapping through ToHostValue

CLAUDE.md

@@ -463,6 +463,27 @@ throw `ArgumentException` out of a public hosting method, whose documented failu
 `GitHostingException` hierarchy. An omitted optional field stays null; only a field the host did
 report and this library cannot represent is raised.
 
+Both providers' pull-request mappers violated this for every field when the pull-request path was
+first added, and the
+`?? string.Empty` fallbacks they used made it worse rather than softer: `GitPullRequestTitle` and
+`GitBranchName` both carry `HasNonWhitespaceContent`, so the fallback turned "the host omitted this"
+into a *guaranteed* `ArgumentException` on data `AzureDevOpsPullRequest.Title` itself declares
+optional — a null title, or a service-principal `IdentityRef` whose `uniqueName` is empty. Octokit's
+side is the same shape for a different reason: its model types are mutable classes with unannotated
+`string` members deserialized straight from the response, so a field GitHub omits arrives null
+however non-nullable the property looks, and `PullRequest.Head`/`.Base` are dereferenced
+conditionally for that reason.
+
+**`GitPullRequest.Number`, `Title`, `SourceBranch` and `TargetBranch` stay `required`; a host that
+omits one raises `GitHostingRequestException`.** The alternative — relaxing them to nullable for the
+omitted-field case — was considered and rejected. Both hosts document all four as part of what a
+pull request *is*, and neither has been observed to omit one; Azure DevOps's DTO declares them
+`string?` only because it mirrors the wire shape defensively. Making them nullable would push a null
+check onto every caller for a case no host produces, and would turn a host contract violation into a
+half-populated record that reads as valid. `GitProvider.ToRequiredHostValue` is the wrapper that
+keeps the distinction: `ToHostValue` for the genuinely optional fields (`Author`, `WebURI`, and
+every `GitRepository` field), `ToRequiredHostValue` for these four.
+
 **Azure DevOps pull request operations require `Project`; repository enumeration does not.** Azure
 DevOps nests repositories under a project — GitHub has no equivalent — so
 `AzureDevOpsProvider.Project` is optional: unset, `GetRepositoriesAsync` enumerates the whole

GitIntegration.Test/Hosting/AzureDevOpsProviderTests.cs

@@ -56,6 +56,26 @@
 		return $"{{\"value\":[{json}],\"count\":1}}";
 	}
 
+	/// <summary>
+	/// Wraps the captured create-response fixture in the pull-request-list envelope after substituting
+	/// one captured field, for the cases that need a field Azure DevOps declares optional to arrive
+	/// omitted, null, or empty.
+	/// </summary>
+	/// <remarks>
+	/// Kept separate from <see cref="SinglePullRequestListResponse"/>, whose <c>status</c> substitution
+	/// is about state mapping rather than about a field being absent. Substituting rather than editing
+	/// a second fixture keeps every other key exactly as captured, so a test that removes <c>title</c>
+	/// is varying one thing and not comparing two hand-written payloads.
+	/// </remarks>
+	/// <param name="original">The captured text to replace, including its trailing comma when removing a whole key.</param>
+	/// <param name="replacement">The text to put in its place, or an empty string to omit the key entirely.</param>
+	private static string SinglePullRequestListResponseReplacing(string original, string replacement)
+	{
+		string json = Fixture("azure-devops-pullrequest-created.json")
+			.Replace(original, replacement, StringComparison.Ordinal);
+		return $"{{\"value\":[{json}],\"count\":1}}";
+	}
+
 	/// <summary>
 	/// Adds a <c>web</c> entry to the captured create-response fixture's <c>_links</c> object.
 	/// </summary>
@@ -754,6 +774,97 @@
 		StringAssert.Contains(exception.ResponseBody, "\"status\": \"notSet\"");
 	}
 
+	[TestMethod]
+	public async Task TranslatesAnOmittedRequiredPullRequestFieldToGitHostingRequestExceptionAsync()
+	{
+		// AzureDevOpsPullRequest declares title, sourceRefName and targetRefName as string? because
+		// Azure DevOps's own schema does, so a response omitting one is ordinary data rather than a
+		// malformed payload. The mapping used to substitute string.Empty, which GitPullRequestTitle
+		// and GitBranchName both reject for whitespace — turning "the host omitted this" into a
+		// guaranteed ArgumentException out of a public hosting method, escaping past every
+		// catch (GitHostingException) a caller wrote. The exception type is the whole point of the
+		// assertion: ThrowsExactly fails on the ArgumentException the old mapping raised.
+		(string original, string replacement, string field)[] cases =
+		[
+			("\"title\": \"A new feature\",", string.Empty, "pull request title"),
+			("\"title\": \"A new feature\",", "\"title\": null,", "pull request title"),
+			("\"sourceRefName\": \"refs/heads/npaulk/my_work\",", string.Empty, "pull request source branch"),
+			("\"targetRefName\": \"refs/heads/new_feature\",", string.Empty, "pull request target branch"),
+		];
+
+		foreach ((string original, string replacement, string field) in cases)
+		{
+			using FakeHttpMessageHandler handler = new FakeHttpMessageHandler()
+				.Respond(HttpStatusCode.OK, SinglePullRequestListResponseReplacing(original, replacement), ("Content-Type", "application/json"));
+			AzureDevOpsProvider provider = new()
+			{
+				Owner = "contoso".As<GitProviderOwner>(),
+				Project = "ExampleProject".As<AzureDevOpsProjectName>(),
+				Handler = handler,
+			};
+
+			GitHostingRequestException exception = await Assert.ThrowsExactlyAsync<GitHostingRequestException>(
+				async () => await provider.GetPullRequestsAsync("example-repo".As<GitRepositoryName>(), TestContext.CancellationTokenSource.Token).ConfigureAwait(false))
+				.ConfigureAwait(false);
+
+			StringAssert.Contains(exception.Message, $"reported no {field}");
+		}
+	}
+
+	[TestMethod]
+	public async Task TranslatesAnUnrepresentablePullRequestAuthorToGitHostingRequestExceptionAsync()
+	{
+		// A service-principal identity commonly carries an empty uniqueName. Unlike an absent
+		// createdBy, that is a value the host did report and this library cannot represent, so it is
+		// raised rather than dropped — as a hosting failure, not the ArgumentException .As<T>() raised
+		// before. The substitution also hits the reviewers entry's uniqueName, which the mapping does
+		// not read; only createdBy's reaches GitPullRequest.Author.
+		using FakeHttpMessageHandler handler = new FakeHttpMessageHandler()
+			.Respond(
+				HttpStatusCode.OK,
+				SinglePullRequestListResponseReplacing("\"uniqueName\": \"example-user@contoso.example\",", "\"uniqueName\": \"\","),
+				("Content-Type", "application/json"));
+		AzureDevOpsProvider provider = new()
+		{
+			Owner = "contoso".As<GitProviderOwner>(),
+			Project = "ExampleProject".As<AzureDevOpsProjectName>(),
+			Handler = handler,
+		};
+
+		GitHostingRequestException exception = await Assert.ThrowsExactlyAsync<GitHostingRequestException>(
+			async () => await provider.GetPullRequestsAsync("example-repo".As<GitRepositoryName>(), TestContext.CancellationTokenSource.Token).ConfigureAwait(false))
+			.ConfigureAwait(false);
+
+		StringAssert.Contains(exception.Message, "pull request author this library cannot represent");
+	}
+
+	[TestMethod]
+	public async Task ReportsNoAuthorWhenAzureDevOpsOmitsCreatedByAsync()
+	{
+		// The other side of the same rule: GitPullRequest.Author is optional, so a host that reports
+		// no identity at all yields null rather than an exception. Without this, routing Author
+		// through ToHostValue could be "fixed" by making every field required and nothing would say
+		// otherwise.
+		using FakeHttpMessageHandler handler = new FakeHttpMessageHandler()
+			.Respond(
+				HttpStatusCode.OK,
+				SinglePullRequestListResponseReplacing("\"uniqueName\": \"example-user@contoso.example\",", string.Empty),
+				("Content-Type", "application/json"));
+		AzureDevOpsProvider provider = new()
+		{
+			Owner = "contoso".As<GitProviderOwner>(),
+			Project = "ExampleProject".As<AzureDevOpsProjectName>(),
+			Handler = handler,
+		};
+
+		IReadOnlyList<GitPullRequest> pullRequests = await provider
+			.GetPullRequestsAsync("example-repo".As<GitRepositoryName>(), TestContext.CancellationTokenSource.Token)
+			.ConfigureAwait(false);
+
+		Assert.IsNull(pullRequests[0].Author);
+		Assert.AreEqual("A new feature".As<GitPullRequestTitle>(), pullRequests[0].Title);
+	}
+
 	[TestMethod]
 	public async Task FetchesEveryPageOfPullRequestsAsync()
 	{

GitIntegration.Test/Hosting/GitHubProviderTests.cs

@@ -58,6 +58,21 @@
 		return $"[{json}]";
 	}
 
+	/// <summary>
+	/// Wraps the captured pull request fixture in a one-element array after substituting one captured
+	/// field, for the cases that need a field to arrive omitted or null.
+	/// </summary>
+	/// <remarks>
+	/// Kept separate from <see cref="SinglePullRequestArray"/>, whose substitutions are about state
+	/// and draft mapping rather than about a field being absent. Substituting rather than writing a
+	/// second fixture keeps every other key exactly as captured, so a test that removes <c>title</c>
+	/// is varying one thing.
+	/// </remarks>
+	/// <param name="original">The captured text to replace, including its trailing comma when removing a whole key.</param>
+	/// <param name="replacement">The text to put in its place, or an empty string to omit the key entirely.</param>
+	private static string SinglePullRequestArrayReplacing(string original, string replacement) =>
+		$"[{Fixture("github-pullrequest-created.json").Replace(original, replacement, StringComparison.Ordinal)}]";
+
 	/// <summary>
 	/// Builds a single-repository response carrying only the fields <see cref="GitHubProvider"/>'s
 	/// mapping reads, with a caller-supplied <c>name</c> — used to drive
@@ -333,6 +348,60 @@
 		Assert.AreEqual(new DateTimeOffset(2026, 8, 21, 0, 43, 5, TimeSpan.Zero), pullRequest.CreatedAt);
 	}
 
+	[TestMethod]
+	public async Task TranslatesAnOmittedRequiredPullRequestFieldToGitHostingRequestExceptionAsync()
+	{
+		// Octokit's model types are mutable classes with unannotated string members deserialized
+		// straight from the response, so a field GitHub omits arrives as null however non-nullable the
+		// property looks. The mapping used to call .As<T>() on those directly, which raises the
+		// ArgumentException a semantic type owes a caller who passed a bad argument — out of a public
+		// hosting method whose documented failure surface is the GitHostingException hierarchy, past
+		// every catch (GitHostingException) a caller wrote. ThrowsExactly is what pins that down: it
+		// fails on the ArgumentException the old mapping raised.
+		(string original, string replacement, string field)[] cases =
+		[
+			("\"title\": \"Don't build all JIT flavors for clr.aot\",", string.Empty, "pull request title"),
+			("\"title\": \"Don't build all JIT flavors for clr.aot\",", "\"title\": null,", "pull request title"),
+			("\"ref\": \"example-branch-1\",", string.Empty, "pull request source branch"),
+			("\"ref\": \"main\",", string.Empty, "pull request target branch"),
+		];
+
+		foreach ((string original, string replacement, string field) in cases)
+		{
+			using FakeHttpMessageHandler handler = new FakeHttpMessageHandler()
+				.Respond(HttpStatusCode.OK, SinglePullRequestArrayReplacing(original, replacement), ("Content-Type", "application/json"));
+			GitHubProvider provider = new() { Owner = "contoso".As<GitProviderOwner>(), Handler = handler };
+
+			GitHostingRequestException exception = await Assert.ThrowsExactlyAsync<GitHostingRequestException>(
+				async () => await provider.GetPullRequestsAsync("example-repo".As<GitRepositoryName>(), TestContext.CancellationTokenSource.Token).ConfigureAwait(false))
+				.ConfigureAwait(false);
+
+			StringAssert.Contains(exception.Message, $"reported no {field}");
+		}
+	}
+
+	[TestMethod]
+	public async Task ReportsNoAuthorWhenGitHubOmitsTheUserAsync()
+	{
+		// The other side of the same rule: GitPullRequest.Author is optional, so a pull request whose
+		// user carries no login yields null rather than an exception. Without this, routing Author
+		// through ToHostValue could be "fixed" by making every field required and nothing would say
+		// otherwise.
+		using FakeHttpMessageHandler handler = new FakeHttpMessageHandler()
+			.Respond(
+				HttpStatusCode.OK,
+				SinglePullRequestArrayReplacing("\"login\": \"example-user-1\",", string.Empty),
+				("Content-Type", "application/json"));
+		GitHubProvider provider = new() { Owner = "contoso".As<GitProviderOwner>(), Handler = handler };
+
+		IReadOnlyList<GitPullRequest> pullRequests = await provider
+			.GetPullRequestsAsync("example-repo".As<GitRepositoryName>(), TestContext.CancellationTokenSource.Token)
+			.ConfigureAwait(false);
+
+		Assert.IsNull(pullRequests[0].Author);
+		Assert.AreEqual("Don't build all JIT flavors for clr.aot".As<GitPullRequestTitle>(), pullRequests[0].Title);
+	}
+
 	[TestMethod]
 	public async Task RequestsOnlyOpenPullRequestsAsync()
 	{

GitIntegration/GitHubProvider.cs

@@ -321,19 +321,31 @@ protected override void Dispose(bool disposing)
 	/// <summary>
 	/// Maps an Octokit pull request onto this library's model.
 	/// </summary>
+	/// <remarks>
+	/// Every field goes through <c>GitProvider.ToHostValue</c> or
+	/// <c>GitProvider.ToRequiredHostValue</c>, never through <c>As&lt;T&gt;()</c>: this is a response
+	/// GitHub sent, so a value this library cannot represent is a hosting failure and not a caller's
+	/// argument failure. Octokit's model types are all mutable classes with unannotated
+	/// <see langword="string"/> members deserialized straight from the response, so nothing about
+	/// them guarantees a field GitHub omitted arrives as anything but <see langword="null"/> —
+	/// <c>As&lt;T&gt;()</c> on one would throw <see cref="ArgumentException"/> out of a public hosting
+	/// method, escaping past every <c>catch (GitHostingException)</c> a caller wrote.
+	/// <see cref="PullRequest.Head"/> and <see cref="PullRequest.Base"/> are dereferenced
+	/// conditionally for the same reason.
+	/// </remarks>
 	/// <param name="pullRequest">The pull request Octokit returned.</param>
 	/// <returns>The equivalent <see cref="GitPullRequest"/>.</returns>
-	private static GitPullRequest ToGitPullRequest(PullRequest pullRequest) => new()
+	private GitPullRequest ToGitPullRequest(PullRequest pullRequest) => new()
 	{
-		Number = pullRequest.Number.ToString(CultureInfo.InvariantCulture).As<GitPullRequestNumber>(),
-		Title = pullRequest.Title.As<GitPullRequestTitle>(),
+		Number = ToRequiredHostValue<GitPullRequestNumber>(pullRequest.Number.ToString(CultureInfo.InvariantCulture), Name, "pull request number"),
+		Title = ToRequiredHostValue<GitPullRequestTitle>(pullRequest.Title, Name, "pull request title"),
 		Description = pullRequest.Body,
-		SourceBranch = pullRequest.Head.Ref.As<GitBranchName>(),
-		TargetBranch = pullRequest.Base.Ref.As<GitBranchName>(),
-		Author = pullRequest.User?.Login?.As<GitPullRequestAuthor>(),
+		SourceBranch = ToRequiredHostValue<GitBranchName>(pullRequest.Head?.Ref, Name, "pull request source branch"),
+		TargetBranch = ToRequiredHostValue<GitBranchName>(pullRequest.Base?.Ref, Name, "pull request target branch"),
+		Author = ToHostValue<GitPullRequestAuthor>(pullRequest.User?.Login, Name, "pull request author"),
 		State = ToGitPullRequestState(pullRequest),
 		IsDraft = pullRequest.Draft,
-		WebURI = pullRequest.HtmlUrl?.As<GitPullRequestWebURI>(),
+		WebURI = ToHostValue<GitPullRequestWebURI>(pullRequest.HtmlUrl, Name, "pull request web URI"),
 		CreatedAt = pullRequest.CreatedAt,
 	};
 

GitIntegration/GitProvider.cs

@@ -377,6 +377,45 @@ internal HostingCredential ResolveCredential() =>
 			$"{providerName} reported a {description} this library cannot represent: '{value}'.");
 	}
 
+	/// <summary>
+	/// Converts a field a host reported into a semantic value this library's model requires, reporting
+	/// an omitted field as a hosting failure rather than letting it reach a <see langword="required"/>
+	/// property as <see langword="null"/>.
+	/// </summary>
+	/// <remarks>
+	/// <para>
+	/// <see cref="ToHostValue{TSemantic}"/> treats an absent field as ordinary, which is right for
+	/// every optional property on <see cref="GitRepository"/> and for
+	/// <see cref="GitPullRequest.Author"/> and <see cref="GitPullRequest.WebURI"/>. It is not right
+	/// for <see cref="GitPullRequest.Number"/>, <see cref="GitPullRequest.Title"/>,
+	/// <see cref="GitPullRequest.SourceBranch"/> and <see cref="GitPullRequest.TargetBranch"/>:
+	/// those are <see langword="required"/>, so "not reported" is not a state the model can hold.
+	/// </para>
+	/// <para>
+	/// The alternative — making those four nullable — was considered and rejected. Both hosts
+	/// document all four as part of what a pull request *is*, and neither has ever been observed to
+	/// omit one; Azure DevOps's DTO declares them <see langword="string?"/> only because it mirrors
+	/// the wire shape defensively. Relaxing the model would push a null check onto every caller for a
+	/// case no host produces, and would silently turn a host contract violation into a
+	/// half-populated record. Raising it keeps the violation visible, and keeps it inside the
+	/// <see cref="GitHostingException"/> hierarchy a public hosting method documents.
+	/// </para>
+	/// </remarks>
+	/// <typeparam name="TSemantic">The semantic string type to produce.</typeparam>
+	/// <param name="value">The raw field as the host reported it, which may be <see langword="null"/>.</param>
+	/// <param name="providerName">The provider that reported it, named in the exception.</param>
+	/// <param name="description">What the field is, used in the failure message.</param>
+	/// <returns>The converted value.</returns>
+	/// <exception cref="GitHostingRequestException">
+	/// <paramref name="value"/> is <see langword="null"/>, or is non-null and fails
+	/// <typeparamref name="TSemantic"/>'s validation.
+	/// </exception>
+	private protected static TSemantic ToRequiredHostValue<TSemantic>(string? value, GitProviderName providerName, string description)
+		where TSemantic : SemanticString<TSemantic>, new() =>
+		ToHostValue<TSemantic>(value, providerName, description)
+			?? throw new GitHostingRequestException(
+				$"{providerName} reported no {description}, which this library requires.");
+
 	/// <summary>
 	/// Creates a transport carrying the settings every provider in this library wants of a shared,
 	/// long-lived handler.