From 15a1188bb90797f86ea4fd15e2db678ed70cccf5 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 28 Aug 2026 15:32:01 -0400 Subject: [PATCH] Keep engine fix in separate pull request --- .../workflows/code-review/code-review.fabro | 2 +- .../code-review/scripts/code_review.py | 6 +- .../code-review/scripts/publish_pr.py | 3 +- lib/components/fabro-workflow/src/artifact.rs | 84 +++---------------- 4 files changed, 19 insertions(+), 76 deletions(-) diff --git a/.fabro/workflows/code-review/code-review.fabro b/.fabro/workflows/code-review/code-review.fabro index c215eda81..8e58a3629 100644 --- a/.fabro/workflows/code-review/code-review.fabro +++ b/.fabro/workflows/code-review/code-review.fabro @@ -33,7 +33,7 @@ digraph CodeReview { timeout="300s", output_schema="routing", stdin_source="context.internal.run_id", - script="python3 -c \"import hashlib,sys; pairs=list(zip(sys.argv[1::2],sys.argv[2::2])); sys.exit(0 if pairs and all(hashlib.sha256(open(path,'rb').read()).hexdigest()==expected for path,expected in pairs) else 91)\" .fabro/workflows/code-review/scripts/code_review.py 7800fce696daddd39dd508e5f8b6de8a0bfb4c44b944e1100164d8dff13ae749 .fabro/workflows/code-review/scripts/git_readonly.py 29cee508724f7bee8d73317d82fe94d0d830361476b367e4a012b70071a0e841 .fabro/workflows/code-review/scripts/publish_pr.py b56bed0d7eab7ca1573aab2b1880167620c9457fe5d05c7f366ec82a94801c66 .fabro/workflows/code-review/scripts/render_report.py fe432d8a53e1294b8030a54ca17b412312338e761ee751ab770e221450b53b8c .fabro/workflows/code-review/scripts/review_contract.py 8917fe7ae046cfda547f4f1240570fa295e110fd5e8d0c84c2a98719137db6fa .fabro/workflows/code-review/scripts/rule_loader.py eaa7258e5cf7b231a7a1192c9738eb2a0486480cd04ebe2059a79295040fc66e .fabro/workflows/code-review/specs/report-spec.md 7a54f72ee46f09218d18854d184a1f36875f9011877e94779c6b1f0d5dd118a9 .fabro/workflows/code-review/templates/report.html 5def570da34ca186da31781378367d70fb9c58e82f7aeec4aaf420fd348a8e61 .fabro/workflows/code-review/schemas/findings.schema.json 2f4d0a9052d5af0dad92db12a1e9d49cc91a282c4dddda495791352bf1559ed8 .fabro/workflows/code-review/schemas/verdict.schema.json de13ce02c5fd0c088640542831cc732e35dee3ddb38f89d4412f6a46fea75567 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 cb3866240077d1bc8993b2f012a8d66a6ea61d4a9f2e88a1fefc6ef375f630e2 .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 a81ee5b0ac134eb121dbf503025387c64126d3276e4673ebc836cfb62a3689fb .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json ecd1d77ad8c77cae153280cb775e5e5f7fa9b68925900473e7d2331af377bb49 && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }}" + script="python3 -c \"import hashlib,sys; pairs=list(zip(sys.argv[1::2],sys.argv[2::2])); sys.exit(0 if pairs and all(hashlib.sha256(open(path,'rb').read()).hexdigest()==expected for path,expected in pairs) else 91)\" .fabro/workflows/code-review/scripts/code_review.py 78d239edb68be8e3db983445a786b9dca7eed6044ff4430e5581ae4c4c8466d3 .fabro/workflows/code-review/scripts/git_readonly.py 29cee508724f7bee8d73317d82fe94d0d830361476b367e4a012b70071a0e841 .fabro/workflows/code-review/scripts/publish_pr.py 35cde9006c9d079f468228498704c7c6079bff52fc790d7e2cee9505d3de9a06 .fabro/workflows/code-review/scripts/render_report.py fe432d8a53e1294b8030a54ca17b412312338e761ee751ab770e221450b53b8c .fabro/workflows/code-review/scripts/review_contract.py 8917fe7ae046cfda547f4f1240570fa295e110fd5e8d0c84c2a98719137db6fa .fabro/workflows/code-review/scripts/rule_loader.py eaa7258e5cf7b231a7a1192c9738eb2a0486480cd04ebe2059a79295040fc66e .fabro/workflows/code-review/specs/report-spec.md 7a54f72ee46f09218d18854d184a1f36875f9011877e94779c6b1f0d5dd118a9 .fabro/workflows/code-review/templates/report.html 5def570da34ca186da31781378367d70fb9c58e82f7aeec4aaf420fd348a8e61 .fabro/workflows/code-review/schemas/findings.schema.json 2f4d0a9052d5af0dad92db12a1e9d49cc91a282c4dddda495791352bf1559ed8 .fabro/workflows/code-review/schemas/verdict.schema.json de13ce02c5fd0c088640542831cc732e35dee3ddb38f89d4412f6a46fea75567 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 cb3866240077d1bc8993b2f012a8d66a6ea61d4a9f2e88a1fefc6ef375f630e2 .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 a81ee5b0ac134eb121dbf503025387c64126d3276e4673ebc836cfb62a3689fb .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json ecd1d77ad8c77cae153280cb775e5e5f7fa9b68925900473e7d2331af377bb49 && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }}" ] grouping [ diff --git a/.fabro/workflows/code-review/scripts/code_review.py b/.fabro/workflows/code-review/scripts/code_review.py index 0f7c758ef..e9cdc29ce 100644 --- a/.fabro/workflows/code-review/scripts/code_review.py +++ b/.fabro/workflows/code-review/scripts/code_review.py @@ -911,9 +911,9 @@ def assert_workspace_unchanged(state: Mapping[str, Any]) -> None: """Refuse to publish results derived from a tampered source tree. Tamper evidence behind the read-only tool guard: an agent that finds a way - to write could shape what the verifiers and the report see. Checked only at - the publication gates: final-tally, render-report, and publish-pr (the - last immediately before anything leaves for GitHub). + to write could shape what the verifiers and the report see. Checked at + final-tally after the last agent and again at publish-pr immediately before + anything leaves for GitHub. """ expected = state.get("workspace_digest") actual = workspace_digest() diff --git a/.fabro/workflows/code-review/scripts/publish_pr.py b/.fabro/workflows/code-review/scripts/publish_pr.py index 0f0782cb5..56fa54b30 100644 --- a/.fabro/workflows/code-review/scripts/publish_pr.py +++ b/.fabro/workflows/code-review/scripts/publish_pr.py @@ -39,6 +39,7 @@ from typing import Any, Dict, List, Mapping, NoReturn, Optional, Sequence, Set, sys.path.insert(0, str(Path(__file__).resolve().parent)) import render_report as renderer # noqa: E402 (the bundle validators) +from review_contract import FINDING_ID_RE # noqa: E402 PLAN_VERSION = 1 @@ -872,7 +873,7 @@ def validate_plan_document( if not isinstance(entry, dict): fail(f"plan {field} must be an object") finding_id = entry.get("finding_id") - if not isinstance(finding_id, str) or not renderer.FINDING_ID_RE.fullmatch( + if not isinstance(finding_id, str) or not FINDING_ID_RE.fullmatch( finding_id ): fail(f"plan {field}.finding_id is invalid") diff --git a/lib/components/fabro-workflow/src/artifact.rs b/lib/components/fabro-workflow/src/artifact.rs index a61f0ba0e..6d2eb1e79 100644 --- a/lib/components/fabro-workflow/src/artifact.rs +++ b/lib/components/fabro-workflow/src/artifact.rs @@ -521,47 +521,23 @@ pub async fn resolve_text_or_blob_ref(value: &Value, run_store: &RunStoreHandle) } } -/// Resolve a structured JSON value from inline context or Fabro-managed blob -/// references at any depth. +/// Resolve a structured JSON value from inline context or a Fabro-managed +/// blob reference. /// /// Managed `file://` references are normalized through their content-addressed /// blob hash instead of reading an execution-local path. Ordinary strings and /// ordinary file references remain unchanged for the caller to validate. -pub(crate) fn resolve_json_value<'a>( - value: Value, - run_store: &'a RunStoreHandle, -) -> BoxFuture<'a, Result> { - Box::pin(async move { - match value { - Value::String(reference) => { - let blob_hash = - parse_blob_ref(&reference).or_else(|| parse_managed_blob_file_ref(&reference)); - let Some(blob_hash) = blob_hash else { - return Ok(Value::String(reference)); - }; - let bytes = read_required_blob(&blob_hash, run_store).await?; - let resolved = serde_json::from_slice(&bytes).map_err(|err| { - Error::engine_with_source("artifact blob was not valid JSON", err) - })?; - resolve_json_value(resolved, run_store).await - } - Value::Array(items) => { - let mut resolved = Vec::with_capacity(items.len()); - for item in items { - resolved.push(resolve_json_value(item, run_store).await?); - } - Ok(Value::Array(resolved)) - } - Value::Object(items) => { - let mut resolved = serde_json::Map::with_capacity(items.len()); - for (key, item) in items { - resolved.insert(key, resolve_json_value(item, run_store).await?); - } - Ok(Value::Object(resolved)) - } - primitive => Ok(primitive), - } - }) +pub(crate) async fn resolve_json_value(value: Value, run_store: &RunStoreHandle) -> Result { + let blob_hash = value.as_str().and_then(|reference| { + parse_blob_ref(reference).or_else(|| parse_managed_blob_file_ref(reference)) + }); + let Some(blob_hash) = blob_hash else { + return Ok(value); + }; + + let bytes = read_required_blob(&blob_hash, run_store).await?; + serde_json::from_slice(&bytes) + .map_err(|err| Error::engine_with_source("artifact blob was not valid JSON", err)) } /// Resolve a flat workflow context key (`context.NAME` or `NAME`) to a @@ -990,40 +966,6 @@ mod tests { ); } - #[tokio::test] - async fn resolve_json_value_hydrates_nested_parallel_branch_values() { - let run_store = make_run_store("nested-structured-json-resolution").await; - let finder_output = serde_json::json!({ - "findings": [{"file": "src/lib.rs", "line": 7}] - }); - let finder_blob = run_store - .write_blob(&serde_json::to_vec(&finder_output).unwrap()) - .await - .unwrap(); - let parallel_results = serde_json::json!([{ - "id": "finder", - "index": 0, - "status": "succeeded", - "context_updates": { - "output.finder": format_blob_ref(&finder_blob), - "small": "kept inline" - } - }]); - - let resolved = resolve_json_value(parallel_results, &run_store.into()) - .await - .unwrap(); - - assert_eq!( - resolved[0]["context_updates"]["output.finder"], - finder_output - ); - assert_eq!( - resolved[0]["context_updates"]["small"], - serde_json::json!("kept inline") - ); - } - #[tokio::test] async fn offload_preserves_parallel_results_and_replaces_large_context_updates() { let run_store = make_run_store("parallel-result-artifact-offload").await;