Skip to content

Commit 744c1a9

Browse files
chubes4Chris Huber
andauthored
fix(runs): keep terminal Lab artifact reads local (#11942)
* fix(runs): keep terminal artifact reads local Terminal artifact and evidence readers now use only persisted controller state, while generated public URLs require recorded reachability before emission. AI assistance: OpenAI gpt-5.6-sol via OpenCode was used to trace the artifact-reader and publication paths, implement the bounded-read invariant, and run focused verification. Chris Huber remains responsible for every line. * fix(artifacts): preserve legacy public links Legacy artifact records without public_url_validation retain their persisted public and viewer links. Newly persisted generated URLs are validation-stamped and only emitted when their recorded reachability proof succeeds. AI assistance: OpenAI gpt-5.6-sol via OpenCode was used to trace the persisted artifact compatibility boundary, implement the tri-state validation contract, add migration coverage, and run verification. Chris Huber remains responsible for every line. * fix(tests): restore self-check argument completeness Populate the hidden release readiness input in self-check command arguments so every workspace test target and CI shard planner compile against the current CLI contract. AI assistance: OpenAI gpt-5.6-terra via OpenCode inspected the CI failures, restored the test command argument contract, and ran workspace compilation plus focused tests. Chris Huber remains responsible for every line. --------- Co-authored-by: Chris Huber <chris@chubes.net>
1 parent 032ce9c commit 744c1a9

8 files changed

Lines changed: 123 additions & 44 deletions

File tree

crates/homeboy-cli/src/commands/bench/observation/tests.rs

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,13 +108,53 @@ fn recorded_bench_artifact_reports_unreachable_public_viewer_url() {
108108
artifact.observation_artifact_id.as_deref(),
109109
Some("artifact-1")
110110
);
111-
assert!(artifact.public_url.is_some());
111+
assert_eq!(artifact.public_url, None);
112112
assert!(artifact.viewer_refs.viewer_links.is_empty());
113113
assert_eq!(artifact.viewer_refs.viewer_url, None);
114114
assert_eq!(diagnostic.class, "bench_public_artifact_url_unreachable");
115115
assert_eq!(diagnostic.metadata["status_code"], 404);
116116
}
117117

118+
#[test]
119+
fn recorded_bench_artifact_preserves_legacy_public_viewer_url() {
120+
let public_artifact_base = "https://artifacts.example.test/homeboy";
121+
let _public_artifact_base = EnvGuard::set(
122+
homeboy::core::artifacts::PUBLIC_ARTIFACT_BASE_URL_ENV,
123+
public_artifact_base,
124+
);
125+
let mut artifact = BenchArtifact::default();
126+
let record = ArtifactRecord {
127+
id: "legacy-artifact".to_string(),
128+
run_id: "run-1".to_string(),
129+
kind: "bench_artifact".to_string(),
130+
artifact_type: "file".to_string(),
131+
path: "/tmp/blueprint.after.json".to_string(),
132+
url: None,
133+
public_url: None,
134+
viewer_url: None,
135+
viewer_links: Vec::new(),
136+
sha256: None,
137+
size_bytes: None,
138+
mime: Some("application/json".to_string()),
139+
metadata_json: serde_json::json!({
140+
"viewer": crate::commands::runs::HOSTED_BLUEPRINT_VIEWER.to_metadata(None)
141+
}),
142+
created_at: "2026-06-12T00:00:00Z".to_string(),
143+
};
144+
145+
let diagnostic = apply_recorded_bench_artifact_links(
146+
"cold",
147+
Some(0),
148+
"blueprint.after",
149+
&mut artifact,
150+
&record,
151+
);
152+
153+
assert_eq!(diagnostic, None);
154+
assert!(artifact.public_url.is_some());
155+
assert_eq!(artifact.viewer_refs.viewer_links.len(), 1);
156+
}
157+
118158
pub(super) fn bench_results(component_id: &str, scenario_id: &str, p95: f64) -> BenchResults {
119159
serde_json::from_value(serde_json::json!({
120160
"component_id": component_id,

crates/homeboy-cli/src/commands/runs/tests/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1524,7 +1524,7 @@ fn artifacts_command_suppresses_viewer_links_when_public_url_is_unreachable() {
15241524
.find(|artifact| artifact.kind == "bench_artifact")
15251525
.expect("bench artifact");
15261526

1527-
assert!(artifact.public_url.is_some());
1527+
assert_eq!(artifact.public_url, None);
15281528
assert!(artifact.viewer_links.is_empty());
15291529
assert_eq!(artifact.viewer_url, None);
15301530
assert_eq!(

crates/homeboy-core/src/artifact_links.rs

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -250,26 +250,29 @@ pub fn cached_validated_viewer_links(
250250
if links.is_empty() {
251251
return links;
252252
}
253-
if artifact
254-
.metadata_json
255-
.get("public_url_validation")
256-
.and_then(|validation| validation.get("reachable"))
257-
.and_then(Value::as_bool)
258-
== Some(true)
259-
{
253+
if public_artifact_url_is_reachable_or_legacy(artifact) {
260254
links
261255
} else {
262256
Vec::new()
263257
}
264258
}
265259

260+
/// Return whether a persisted public URL may be presented to a reviewer.
261+
///
262+
/// Records written before validation was introduced have no validation metadata
263+
/// and retain their persisted links. New records carry validation metadata, so
264+
/// malformed or failed validation suppresses their generated links.
265+
pub fn public_artifact_url_is_reachable_or_legacy(artifact: &ArtifactRecord) -> bool {
266+
match artifact.metadata_json.get("public_url_validation") {
267+
None => true,
268+
Some(validation) => validation.get("reachable").and_then(Value::as_bool) == Some(true),
269+
}
270+
}
271+
266272
pub fn annotate_public_artifact_url_validation(
267273
artifact: &mut ArtifactRecord,
268274
) -> Option<PublicArtifactUrlValidation> {
269275
let public_url = public_artifact_url(artifact)?;
270-
if viewer_links(artifact, Some(&public_url)).is_empty() {
271-
return None;
272-
}
273276
let validation = validate_public_artifact_url(&public_url);
274277
artifact.metadata_json["public_url_validation"] =
275278
public_artifact_url_validation_json(&validation);
@@ -544,6 +547,27 @@ mod tests {
544547
}
545548
}
546549

550+
#[test]
551+
fn public_url_validation_preserves_legacy_records_and_gates_new_records() {
552+
let legacy = viewer_artifact();
553+
let reachable = ArtifactRecord {
554+
metadata_json: serde_json::json!({
555+
"public_url_validation": { "reachable": true }
556+
}),
557+
..viewer_artifact()
558+
};
559+
let unreachable = ArtifactRecord {
560+
metadata_json: serde_json::json!({
561+
"public_url_validation": { "reachable": false }
562+
}),
563+
..viewer_artifact()
564+
};
565+
566+
assert!(public_artifact_url_is_reachable_or_legacy(&legacy));
567+
assert!(public_artifact_url_is_reachable_or_legacy(&reachable));
568+
assert!(!public_artifact_url_is_reachable_or_legacy(&unreachable));
569+
}
570+
547571
struct EnvGuard {
548572
key: &'static str,
549573
prior: Option<String>,

crates/homeboy-core/src/artifacts.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ pub use super::artifact_dom_boxes::{
1212
};
1313
pub use super::artifact_links::{
1414
cached_validated_viewer_links, public_artifact_path_url, public_artifact_url,
15-
PUBLIC_ARTIFACT_BASE_URL_ENV,
15+
public_artifact_url_is_reachable_or_legacy, PUBLIC_ARTIFACT_BASE_URL_ENV,
1616
};
1717
pub use super::artifact_manifest::{
1818
ArtifactManifest, ARTIFACT_MANIFEST_SCHEMA, RUNTIME_AGENT_ARTIFACT_PATHS_SCHEMA,

crates/homeboy-core/src/observation/runs_service/artifact_links.rs

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,19 @@ use super::*;
22

33
/// Enrich a single artifact record with public/viewer link metadata.
44
///
5-
/// Mirrors the original CLI helper exactly: derive a public URL (from
6-
/// stored artifact metadata or by treating the artifact path as the URL
7-
/// for `url`-typed artifacts), then resolve any cached viewer links.
5+
/// Derive a public URL from stored artifact metadata or, for explicitly
6+
/// URL-typed artifacts, from the recorded URL. Generated public URLs are only
7+
/// emitted after the persistence-time probe verified them, so a successful
8+
/// terminal handoff never advertises an already-broken tunnel URL.
89
pub(crate) fn enrich_artifact_link(mut artifact: ArtifactRecord) -> ArtifactRecord {
910
let public_url =
1011
public_artifact_url(&artifact).or_else(|| public_url_for_url_artifact(&artifact));
1112
if let Some(url) = public_url.clone() {
13+
if artifact.artifact_type != "url"
14+
&& !crate::artifact_links::public_artifact_url_is_reachable_or_legacy(&artifact)
15+
{
16+
return artifact;
17+
}
1218
artifact.public_url = Some(url.clone());
1319
artifact.viewer_links = cached_validated_viewer_links(&artifact, &url);
1420
artifact.viewer_url = artifact.viewer_links.first().map(|link| link.url.clone());
@@ -62,13 +68,10 @@ pub fn related_lab_run_ids(store: &ObservationStore, run: &RunRecord) -> Result<
6268
if run.kind != "runner-exec" {
6369
return Ok(Vec::new());
6470
}
65-
// Resolve the runner-exec run's Lab job id. Prefer authoritative runner
66-
// evidence; fall back to the persisted metadata in either shape.
67-
let Some(job_id) =
68-
runner_evidence::with_runner_evidence(|p| p.mirrored_runner_job_identity(run))
69-
.map(|(_runner_id, job_id)| job_id)
70-
.or_else(|| lab_remote_job_id(run).map(str::to_string))
71-
else {
71+
// The terminal envelope persists the Lab identity. Do not ask a live
72+
// runner from a durable reader: the runner can be gone precisely when its
73+
// already-recorded evidence is needed for review.
74+
let Some(job_id) = lab_remote_job_id(run).map(str::to_string) else {
7275
return Ok(Vec::new());
7376
};
7477
let mut run_ids = Vec::new();
@@ -94,15 +97,15 @@ pub fn related_lab_run_ids(store: &ObservationStore, run: &RunRecord) -> Result<
9497

9598
/// List the enriched artifact records attached to a run.
9699
///
97-
/// Side-effect ordering matches the CLI: refresh mirrored daemon evidence,
98-
/// then index nested publication artifact refs, then list and enrich.
100+
/// This reader only consults the durable local store. Runner reconciliation and
101+
/// remote manifest indexing are explicit operations, so a disconnected or
102+
/// stuck runner cannot block an artifact receipt, evidence report, or terminal
103+
/// handoff.
99104
pub fn list_artifacts_for_run(
100105
store: &ObservationStore,
101106
run_id: &str,
102107
) -> Result<Vec<ArtifactRecord>> {
103108
let run = require_run(store, run_id)?;
104-
refresh_mirrored_daemon_evidence_best_effort(&run.id);
105-
crate::artifacts::index_remote_published_artifact_refs_for_run(store, &run.id)?;
106109
let artifacts = store.list_artifacts(&run.id)?;
107110
Ok(enrich_artifact_links(artifacts))
108111
}

crates/homeboy-core/src/observation/runs_service/tests.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -690,6 +690,11 @@ fn require_run_reads_terminal_lab_review_alias_while_runner_probe_is_stalled() {
690690
store
691691
.import_run(&run)
692692
.expect("persist terminal Lab review run");
693+
let artifact_path = _home.path().join("terminal-evidence.json");
694+
std::fs::write(&artifact_path, br#"{"ok":true}"#).expect("artifact bytes");
695+
store
696+
.record_artifact(&run.id, "terminal_evidence", &artifact_path)
697+
.expect("persist terminal evidence");
693698

694699
let (entered_tx, entered_rx) = mpsc::channel();
695700
let (release_tx, release_rx) = mpsc::channel();
@@ -707,6 +712,10 @@ fn require_run_reads_terminal_lab_review_alias_while_runner_probe_is_stalled() {
707712
let resolved = require_run(&store, label).expect("durable local terminal record");
708713
assert_eq!(resolved.id, run.id);
709714
assert_eq!(resolved.status, RunStatus::Fail.as_str());
715+
let artifacts = list_artifacts_for_run(&store, &run.id)
716+
.expect("durable artifact reader must not wait for the runner");
717+
assert_eq!(artifacts.len(), 1);
718+
assert_eq!(artifacts[0].kind, "terminal_evidence");
710719
assert!(
711720
!stalled.is_finished(),
712721
"lookup completed before probe release"

crates/homeboy-extension/src/bench/artifact_persistence.rs

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -353,25 +353,26 @@ pub fn apply_recorded_bench_artifact_links(
353353
) -> Option<BenchDiagnostic> {
354354
artifact.observation_artifact_id = Some(record.id.clone());
355355
let public_url = artifact_links::public_artifact_url(record)?;
356-
artifact.public_url = Some(public_url.clone());
357-
artifact.viewer_refs.viewer_links =
358-
artifact_links::cached_validated_viewer_links(record, &public_url);
359-
artifact.viewer_refs.viewer_url = artifact
360-
.viewer_refs
361-
.viewer_links
362-
.first()
363-
.map(|link| link.url.clone());
364-
let validation = record.metadata_json.get("public_url_validation")?;
365-
let reachable = validation
366-
.get("reachable")
367-
.and_then(serde_json::Value::as_bool)
368-
.unwrap_or(false);
369-
(!reachable).then(|| {
356+
if artifact_links::public_artifact_url_is_reachable_or_legacy(record) {
357+
artifact.public_url = Some(public_url.clone());
358+
artifact.viewer_refs.viewer_links =
359+
artifact_links::cached_validated_viewer_links(record, &public_url);
360+
artifact.viewer_refs.viewer_url = artifact
361+
.viewer_refs
362+
.viewer_links
363+
.first()
364+
.map(|link| link.url.clone());
365+
None
366+
} else {
367+
let validation = record
368+
.metadata_json
369+
.get("public_url_validation")
370+
.expect("unreachable new artifact has validation metadata");
370371
let error = validation
371372
.get("error")
372373
.and_then(serde_json::Value::as_str)
373374
.unwrap_or("public artifact URL was not reachable");
374-
bench_artifact_diagnostic(
375+
Some(bench_artifact_diagnostic(
375376
scenario_id,
376377
run_index,
377378
name,
@@ -384,8 +385,8 @@ pub fn apply_recorded_bench_artifact_links(
384385
"status_code": validation.get("status_code").cloned().unwrap_or(serde_json::Value::Null),
385386
"error": validation.get("error").cloned().unwrap_or(serde_json::Value::Null),
386387
}),
387-
)
388-
})
388+
))
389+
}
389390
}
390391

391392
fn bench_artifact_metadata(

tests/self_checks_test.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ fn lint_args(root: &Path) -> LintArgs {
5252
force: false,
5353
setting_args: SettingArgs::default(),
5454
baseline_args: BaselineArgs::default(),
55+
release_readiness_source: None,
5556
json_summary: false,
5657
}
5758
}
@@ -73,6 +74,7 @@ fn test_args(root: &Path) -> TestArgs {
7374
changed: LabChangedScopeArgs::default(),
7475
ci_job: None,
7576
setting_args: SettingArgs::default(),
77+
release_readiness_source: None,
7678
args: Vec::new(),
7779
json_summary: false,
7880
restore_checkout: false,

0 commit comments

Comments
 (0)