Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 28 additions & 1 deletion crates/pet/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,42 @@
fn main() {
println!("cargo:rerun-if-env-changed=PET_BUILD_ID");
println!("cargo:rerun-if-env-changed=BUILD_BUILDID");
println!("cargo:rerun-if-env-changed=PET_COMMIT_SHA");
println!("cargo:rerun-if-env-changed=BUILD_SOURCEVERSION");
println!("cargo:rerun-if-env-changed=GITHUB_SHA");

// Filter empties per-candidate so an explicitly-set-but-empty primary var
// doesn't short-circuit the fallback chain (e.g., `PET_BUILD_ID=""`).
if let Some(build_id) = std::env::var("PET_BUILD_ID")
.ok()
.or_else(|| std::env::var("BUILD_BUILDID").ok())
.filter(|value| !value.is_empty())
.or_else(|| {
std::env::var("BUILD_BUILDID")
.ok()
.filter(|value| !value.is_empty())
})
{
println!("cargo:rustc-env=PET_BUILD_ID={build_id}");
}

// BUILD_SOURCEVERSION is set by Azure Pipelines; GITHUB_SHA by GitHub Actions.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
The Skeptic identified (structurally confirmed) that an explicitly-set-but-empty PET_COMMIT_SHA silently suppresses the BUILD_SOURCEVERSION/GITHUB_SHA fallbacks: or_else only fires on None, so Some("") short-circuits the chain and the trailing .filter() then drops it to None, losing the SHA entirely. This is concretely triggerable (a GitHub Actions YAML referencing an unset context yields ""). While the Advocate notes this mirrors the existing build_id logic and may be unreachable in practice, the fix is cheap — filter each candidate before falling back: .ok().filter(|v| !v.is_empty()).or_else(|| std::env::var("BUILD_SOURCEVERSION").ok().filter(|v| !v.is_empty())).... Apply the same hardening to the build_id chain above.

[verified]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 356f4af. Moved the empty-string .filter() inside each candidate so a Some("") falls through to the next env var instead of being swallowed by the trailing filter. Applied the same hardening to the �uild_id chain.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 356f4af. Moved the empty-string .filter() inside each candidate so a Some(\"\") falls through to the next env var instead of being swallowed by the trailing filter. Applied the same hardening to the build_id chain.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
Operational caveat from the Skeptic (Low): Azure Pipelines' BUILD_SOURCEVERSION is the ephemeral merge commit for PR-triggered builds, not the PR head commit. Since the stated goal is correlating regressions to a specific PET commit, verify the CI trigger config resolves to the intended (head) commit — this is a config check, not a code defect.

[verified]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — treating this as a config check rather than a code change. Shipped PET binaries are built from main / release pipeline runs (not PR-triggered validation), where BUILD_SOURCEVERSION resolves to the real source commit. If we ever change the release pipeline to build from a PR context, I'll set PET_COMMIT_SHA explicitly to the head SHA so this stays correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — treating this as a config check rather than a code change. Shipped PET binaries are built from main / release pipeline runs (not PR-triggered validation), where BUILD_SOURCEVERSION resolves to the real source commit. If we ever change the release pipeline to build from a PR context, I'll set PET_COMMIT_SHA explicitly to the head SHA so this stays correct.

if let Some(commit_sha) = std::env::var("PET_COMMIT_SHA")
.ok()
.filter(|value| !value.is_empty())
.or_else(|| {
std::env::var("BUILD_SOURCEVERSION")
.ok()
.filter(|value| !value.is_empty())
})
.or_else(|| {
std::env::var("GITHUB_SHA")
.ok()
.filter(|value| !value.is_empty())
})
{
println!("cargo:rustc-env=PET_COMMIT_SHA={commit_sha}");
}

#[cfg(target_os = "windows")]
{
if std::env::var("CARGO_BIN_NAME").is_err() {
Expand Down
38 changes: 37 additions & 1 deletion crates/pet/src/jsonrpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,8 @@ pub struct InfoResponse {
pub pet_version: String,
#[serde(skip_serializing_if = "Option::is_none")]
pub build_id: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub commit_sha: Option<String>,
}

impl InfoResponse {
Expand All @@ -532,6 +534,9 @@ impl InfoResponse {
build_id: option_env!("PET_BUILD_ID")
.filter(|value| !value.is_empty())
.map(ToString::to_string),
commit_sha: option_env!("PET_COMMIT_SHA")
.filter(|value| !value.is_empty())
.map(ToString::to_string),
}
}
}
Expand Down Expand Up @@ -1537,14 +1542,45 @@ mod tests {
}

#[test]
fn test_info_response_uses_package_version_and_optional_build_id() {
fn test_info_response_uses_package_version_and_optional_build_metadata() {
let info = InfoResponse::current();

assert_eq!(info.pet_version, env!("CARGO_PKG_VERSION"));
// build_id / commit_sha are populated from env vars set at compile time by CI.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
The Skeptic, Advocate, and Architect all noted these assertions are vacuously true: commit_sha.as_deref().is_none_or(|s| !s.is_empty()) passes whenever commit_sha is None, which is every local dev run — and build.rs's own .filter() already guarantees it can never be Some(""). As a result no test positively exercises the build.rs → binary → JSON path the PR claims, so a regression (the empty-string short-circuit, a serde-rename typo, a dropped option_env!) would ship green. Consider a test that, when option_env!("PET_COMMIT_SHA") is Some, asserts the serialized JSON contains a non-empty commitSha key.

[verified]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Added test_info_response_serializes_camel_case_and_omits_none in 356f4af that constructs InfoResponse directly and asserts the JSON wire format — both the camelCase rename and the skip_serializing_if behavior. That catches serde-rename typos, dropped option_env!, or accidentally serializing None without requiring CI env vars at compile time. Kept the existing assertion (now renamed to ..._optional_build_metadata) as a light sanity check on the option_env! path; the new test provides the real wire-format protection.

// For local dev builds they will be None; assert non-empty only when present.
assert!(info
.build_id
.as_deref()
.is_none_or(|build_id| !build_id.is_empty()));
assert!(info
.commit_sha
.as_deref()
.is_none_or(|commit_sha| !commit_sha.is_empty()));
}

#[test]
fn test_info_response_serializes_camel_case_and_omits_none() {
// Guards the JSON wire format (camelCase rename + skip_serializing_if)
// independently of whether CI env vars were set at compile time.
let json = serde_json::to_value(InfoResponse {
pet_version: "1.2.3".to_string(),
build_id: Some("42".to_string()),
commit_sha: Some("abc123".to_string()),
})
.unwrap();
assert_eq!(json["petVersion"], "1.2.3");
assert_eq!(json["buildId"], "42");
assert_eq!(json["commitSha"], "abc123");

let json = serde_json::to_value(InfoResponse {
pet_version: "1.2.3".to_string(),
build_id: None,
commit_sha: None,
})
.unwrap();
assert_eq!(json["petVersion"], "1.2.3");
assert!(json.get("buildId").is_none());
assert!(json.get("commitSha").is_none());
}

#[test]
Expand Down
1 change: 1 addition & 0 deletions crates/pet/tests/jsonrpc_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub struct RefreshResult {
pub struct PetInfoResponse {
pub pet_version: String,
pub build_id: Option<String>,
pub commit_sha: Option<String>,
}

#[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
Expand Down
8 changes: 7 additions & 1 deletion crates/pet/tests/jsonrpc_server_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,16 +111,22 @@ fn assert_single_environment(
}

#[test]
fn info_reports_pet_version_and_optional_build_id() {
fn info_reports_pet_version_and_optional_build_metadata() {
let client = PetJsonRpcClient::spawn().expect("failed to spawn PET server");

let info = client.info().expect("info request failed");

assert_eq!(info.pet_version, env!("CARGO_PKG_VERSION"));
// build_id / commit_sha are populated from env vars set at compile time by CI.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
Same vacuous-assertion concern as the unit test: commit_sha/build_id are None in local and most CI runs, so this assertion can essentially never fail and provides little real coverage of the end-to-end wire format. A round-trip test that sets the env var and asserts the commitSha JSON key would give genuine protection.

[verified]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix path as the unit-test thread: the new test_info_response_serializes_camel_case_and_omits_none in 356f4af covers the wire format at the serialization layer, which is where a serde-rename / skip_serializing_if regression would actually break. Setting compile-time env vars from an integration test would require rebuilding the binary, so I kept this assertion (renamed to info_reports_pet_version_and_optional_build_metadata) as a smoke check that deserialization still works when a SHA is present in a CI build.

// For local dev builds they will be None; assert non-empty only when present.
assert!(info
.build_id
.as_deref()
.is_none_or(|build_id| !build_id.is_empty()));
assert!(info
.commit_sha
.as_deref()
.is_none_or(|commit_sha| !commit_sha.is_empty()));
}

#[test]
Expand Down
7 changes: 7 additions & 0 deletions docs/JSONRPC.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,15 @@ interface InfoResponse {
petVersion: string;
/**
* Build identifier baked into the binary when built by CI.
* Sourced from `PET_BUILD_ID` or Azure Pipelines `BUILD_BUILDID`.
*/
buildId?: string;
/**
* Source git commit SHA baked into the binary when built by CI.
* Sourced from `PET_COMMIT_SHA`, Azure Pipelines `BUILD_SOURCEVERSION`,
* or GitHub Actions `GITHUB_SHA`. Absent for local dev builds.
*/
commitSha?: string;
}
```

Expand Down
Loading