scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-10 03:30:59 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Fix large parallel review inputs

Browse source
This commit is contained in:
Bryan Helmkamp 2026-08-28 15:07:39 -04:00
parent 7416745cd7
commit c2ac1a8500
No known key found for this signature in database
14 changed files with 245 additions and 203 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
.fabro/workflows/code-review/code-review.fabro
View file

@ -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 6770c5ea6673063ea63a9bbef178c37b5427f24476122a4333eff7c4d390bbc2 .fabro/workflows/code-review/scripts/git_readonly.py bcd4364ba3aca2ee1e12d5909204f645c16bdf22e3753a39d74c79d8d37cf73e .fabro/workflows/code-review/scripts/publish_pr.py b69d8ed821293f05cb56e1719c460ac69d5fa08f9ea3123c3fca6811309087da .fabro/workflows/code-review/scripts/render_report.py 79def74b4415f1a6d1d58cab6480450fed2ee9de11105db95e0996ed08057085 .fabro/workflows/code-review/scripts/rule_loader.py 6cb9c665db54d4dd799acd0867c9a68ad0a26e19de804d63b7781abbd43d60e6 .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 68f853c1624e6a14596d0d5b79501b8b4ba182ce65a7670af413c667fbd84fc8 && 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 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 }}"
]
grouping [

10
.fabro/workflows/code-review/rules/builtin-manifest.json
View file

@ -86,7 +86,7 @@
},
{
"path": "rules/builtin/language/arkts.yaml",
"sha256": "af33d250eb3aa46c4f37b14eafa0f4329aa2800a8cd4515047aa52f33aa116aa"
"sha256": "234846f961952b85d4979960a09db2a380b027e754dea22877032ccd22c039c4"
},
{
"path": "rules/builtin/language/astro.yaml",
@ -98,11 +98,11 @@
},
{
"path": "rules/builtin/language/cpp.yaml",
"sha256": "3c9b9b4951f406eb3ccf185a9c18e6fc17c99693ac565ea506489c4925f1efe6"
"sha256": "5e99bbddbbf913328271e09528928b3c97b61d1dc3dcf91c7c5880c5ddb0f382"
},
{
"path": "rules/builtin/language/elm.yaml",
"sha256": "70a8037e3c537162504dcd9b28e859d87b8a8e042d0f5f713082b3383d515769"
"sha256": "31993c24e5d472bdf9b65ec2436f4e49c2bfac877e5eb7e4cbb28fb66db0d2c5"
},
{
"path": "rules/builtin/language/freemarker.yaml",
@ -138,7 +138,7 @@
},
{
"path": "rules/builtin/language/matlab.yaml",
"sha256": "0773945d54c9b818a7d52552176d21980f679f400f80a6e652e3cad9475f4c3d"
"sha256": "7482032d11b4196df25f4764ba4d1720f1330e9c5b2de77b568d4e1df80ffaa5"
},
{
"path": "rules/builtin/language/nim.yaml",
@ -158,7 +158,7 @@
},
{
"path": "rules/builtin/language/python.yaml",
"sha256": "ee63e8cba2877d4ce50ec6a085cd449d2a5b7793a905d6a98155e1eac86ca04b"
"sha256": "c02fe9717e29f36eb605479531f62876ec78065b3eaefc38a94203c9b67d35b7"
},
{
"path": "rules/builtin/language/r.yaml",

4
.fabro/workflows/code-review/rules/builtin/language/arkts.yaml
View file

@ -26,7 +26,7 @@ rules:
- id: state-decorator-usage
category: correctness
guidance: |
- Arrays/objects decorated with `@State` will not trigger UI refresh when modified via push/property changes; references must be replaced
- `@State` observes array additions, removals, and item replacement, but not nested object property mutations; use `@Observed` + `@ObjectLink` when the UI depends on nested changes
- Verify correct usage of `@Prop` (one-way) vs `@Link` (two-way) for the given scenario
- Nested object state updates must use `@Observed` + `@ObjectLink`
- Props drilling beyond 3 levels should use `@Provide/@Consume` instead
@ -49,7 +49,7 @@ rules:
guidance: |
- Large lists (>20 items) must use `LazyForEach` instead of `ForEach`
- Creating new objects, closures, or calling functions that return styles in the `build` method is prohibited, as it causes unnecessary child component rebuilds
- Complex computation results should be cached via `@Watch` to avoid redundant calculations on each render
- Complex computations repeated in `build` when their inputs have not changed; precompute on input changes or use an appropriate computed-state mechanism (`@Watch` is a change callback, not a cache)
- Image resources should have proper caching strategies to avoid repeated loading
- id: resource-access-standards
category: correctness

2
.fabro/workflows/code-review/rules/builtin/language/cpp.yaml
View file

@ -10,7 +10,7 @@ rules:
- id: language.cpp
match:
paths:
- "**/*.{cpp,cc,hpp}"
- "**/*.{cpp,cc,cxx,h,hpp,hh,hxx}"
checks:
- id: obvious-typos-or-spelling-errors
category: conventions

2
.fabro/workflows/code-review/rules/builtin/language/elm.yaml
View file

@ -52,7 +52,7 @@ rules:
guidance: |
- `Debug.log` or `Debug.toString` left in code paths that ship to production; `elm make --optimize` fails to compile with `Debug.log`/`Debug.todo` present, so leftover calls block optimized builds
- `Debug.todo` used as a placeholder for unimplemented branches that are reachable in normal application flow rather than genuinely unreachable states
- Comparing custom types or records with `==` where the comparison actually needs `Debug.toString` or a dedicated field-by-field comparison, given Elm's structural equality semantics
- Comparing values that can contain functions with `==`, which can fail at runtime; compare dedicated fields instead. Do not recommend `Debug.toString` for comparisons because `Debug` is unavailable in `--optimize` builds
- id: package-versioning-and-dependencies
category: correctness
guidance: |

2
.fabro/workflows/code-review/rules/builtin/language/matlab.yaml
View file

@ -128,7 +128,7 @@ rules:
- Loop-invariant work inside a loop: repeated `ismember` against the same set, repeated struct field lookups, repeated table indexing, repeated file access
- Small element-wise loops that vectorize cleanly
- A variable that changes class or shape mid-function instead of a new variable being introduced
- `parfor` used before the serial version has been profiled; `parfor` without a `numThreads` argument, which makes debugging inside called functions impossible
- `parfor` used before the serial version has been profiled; `parfor` without an explicit worker bound (the optional second argument, e.g. `parfor (i = 1:n, 4)`) where a deterministic pool size matters
- Loop-carried dependencies, order-dependent output, or shared mutable state inside `parfor`; results that depend on iteration order are a correctness defect, not a performance note
- Random number generation inside `parfor` without an explicit reproducible stream, where results must be repeatable
- Do not raise micro-optimizations; clear code that is slower is explicitly preferred to fast code that is hard to follow

2
.fabro/workflows/code-review/rules/builtin/language/python.yaml
View file

@ -56,7 +56,7 @@ rules:
category: correctness
guidance: |
- Using `is`/`is not` to compare against literals such as strings, numbers, or tuples; this relies on implementation-specific interning rather than value equality — use `==` (a real correctness risk)
- Comparing against `True`/`False` with `==`, where a truthy-but-not-`True` value (e.g. `1`, a non-empty container) would compare unequal; prefer a plain truthiness check
- Comparing against `True`/`False` with `==`, where a truthy-but-not-`True` value (e.g. `2`, a non-empty container) would compare unequal; prefer a plain truthiness check
- Reserve `is` for identity checks against singletons and sentinels
- Comparing against `None` with `==`/`!=` rather than `is`/`is not` is a style preference; report as minor, not blocking
- id: resource-management

185
.fabro/workflows/code-review/scripts/code_review.py
View file

@ -50,6 +50,17 @@ from typing import (
Tuple,
)
sys.path.insert(0, str(Path(__file__).resolve().parent))
from review_contract import ( # noqa: E402
CATEGORIES,
COMPILED_RULE_ID_RE,
EFFORT_TIERS,
ISSUE_TYPES,
MAX_RULE_IDS_PER_FINDING,
REVIEW_MODES,
)
WORKFLOW_ROOT = Path(".fabro/workflows/code-review")
CONTROL_DIR = WORKFLOW_ROOT / "runtime"
@ -81,26 +92,6 @@ SMALL_DIFF_MAX_FILES = 5
SMALL_DIFF_MAX_LINES = 300
SMALL_SCOPE_MAX_FILES = 5
EFFORT_TIERS = ("low", "medium", "high", "xhigh", "max")
REVIEW_MODES = ("changes", "commit", "files")
CATEGORIES = (
"correctness",
"reuse",
"simplification",
"efficiency",
"altitude",
"conventions",
"test-coverage",
)
ISSUE_TYPES = (
"bug",
"security",
"performance",
"maintainability",
"test",
"style",
"documentation",
)
# Correctness bugs always outrank cleanup findings when a cap forces a cut.
CLEANUP_CATEGORIES = frozenset(CATEGORIES) - {"correctness"}
# Policy filters drop well-formed findings the review does not want; unlike a
@ -869,6 +860,20 @@ def diff_file_records(
return records
def diff_record_summary(
records: Mapping[str, Mapping[str, Any]],
) -> Tuple[List[str], Optional[int]]:
"""Return sorted paths and total text churn from one diff scan."""
churn = [
record.get("added", 0) + record.get("deleted", 0)
for record in records.values()
if isinstance(record.get("added"), int)
and isinstance(record.get("deleted"), int)
]
total = sum(churn) if len(churn) == len(records) else None
return sorted(records), total
def read_file_at_revision(revision: str, path: str) -> Optional[bytes]:
result = git("show", f"{revision}:{path}")
if result.returncode != 0:
@ -1068,16 +1073,6 @@ def verify_schema_sources() -> None:
# --- Rule compilation (rule-mapped tiers) -------------------------------------
# Mirrors rule_loader.COMPILED_ID_RE; prepare asserts the two agree so the
# lower tiers never need to import the loader (or PyYAML) to validate.
COMPILED_RULE_ID_RE = re.compile(
r"^(builtin|repo):"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?"
r"/"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?$"
)
def import_rule_loader() -> Any:
try:
import rule_loader
@ -1086,14 +1081,6 @@ def import_rule_loader() -> Any:
"the rule-mapped tiers need the rule loader and its pinned PyYAML "
f"dependency (see README, Developing): {error}"
) from error
if tuple(rule_loader.CATEGORIES) != CATEGORIES:
raise WorkflowDataError(
"rule_loader's category list does not match this engine"
)
if rule_loader.COMPILED_ID_RE.pattern != COMPILED_RULE_ID_RE.pattern:
raise WorkflowDataError(
"rule_loader's compiled-ID pattern does not match this engine"
)
return rule_loader
@ -1598,16 +1585,7 @@ def prepare(args: argparse.Namespace) -> None:
revision_range = f"{merge_base}..HEAD"
if cell["rule_mapped"]:
file_records = diff_file_records(revision_range, scope)
changed_files = sorted(file_records)
churn = [
record.get("added", 0) + record.get("deleted", 0)
for record in file_records.values()
if isinstance(record.get("added"), int)
and isinstance(record.get("deleted"), int)
]
diff_lines = (
sum(churn) if len(churn) == len(file_records) else None
)
changed_files, diff_lines = diff_record_summary(file_records)
else:
changed_files, diff_lines = diff_stats(revision_range, scope)
elif mode == "commit":
@ -1629,16 +1607,7 @@ def prepare(args: argparse.Namespace) -> None:
revision_range = f"{parent or empty_tree_hash()}..{target_commit}"
if cell["rule_mapped"]:
file_records = diff_file_records(revision_range, scope)
changed_files = sorted(file_records)
churn = [
record.get("added", 0) + record.get("deleted", 0)
for record in file_records.values()
if isinstance(record.get("added"), int)
and isinstance(record.get("deleted"), int)
]
diff_lines = (
sum(churn) if len(churn) == len(file_records) else None
)
changed_files, diff_lines = diff_record_summary(file_records)
else:
changed_files, diff_lines = diff_stats(revision_range, scope)
else:
@ -1919,6 +1888,13 @@ def phase_jobs_context(
# --- Finding and verdict normalization ---------------------------------------
def bounded_rule_ids(values: Iterable[Any]) -> List[str]:
"""Return the renderer-safe, deterministic union of compiled rule IDs."""
return sorted({value for value in values if isinstance(value, str)})[
:MAX_RULE_IDS_PER_FINDING
]
def finding_or_rejection(
value: Any,
rule_context: Optional[Mapping[str, Any]] = None,
@ -2027,7 +2003,12 @@ def finding_or_rejection(
return None, "rule_id is not a compiled check ID"
if raw_rule_id not in (effective.get(path) or ()):
return None, "the named rule check does not apply to the file"
rule_ids = sorted(set(raw_rule_ids))
rule_ids = bounded_rule_ids(raw_rule_ids)
if len(set(raw_rule_ids)) > MAX_RULE_IDS_PER_FINDING:
return None, (
"rule_ids names more than "
f"{MAX_RULE_IDS_PER_FINDING} distinct checks"
)
if category == "conventions" and not rule_ids:
return None, CONVENTIONS_FILTER_REASON
@ -2615,15 +2596,15 @@ def plan_verify() -> None:
merged = dict(report)
merged["reports"] = 1
merged["reporters"] = [report["angle"]]
merged["rule_ids"] = sorted(set(report.get("rule_ids") or []))
merged["rule_ids"] = bounded_rule_ids(report.get("rule_ids") or [])
by_key[key] = merged
continue
existing["reports"] += 1
if report["angle"] not in existing["reporters"]:
existing["reporters"].append(report["angle"])
existing["rule_ids"] = sorted(
set(existing.get("rule_ids") or [])
| set(report.get("rule_ids") or [])
existing["rule_ids"] = bounded_rule_ids(
list(existing.get("rule_ids") or [])
+ list(report.get("rule_ids") or [])
)
# A fix is publishable only when reporting passes agree on its exact
# range and replacement. A pass that offers no fix does not veto an
@ -2875,7 +2856,7 @@ def reviewed_revision(state: Optional[Mapping[str, Any]]) -> Optional[str]:
return revision if isinstance(revision, str) and revision else None
@functools.lru_cache(maxsize=256)
@functools.lru_cache(maxsize=16)
def reviewed_source_lines(
file_path: str, revision: Optional[str] = None
) -> Optional[List[str]]:
@ -3275,17 +3256,20 @@ def fold_duplicates(
if reporter and reporter not in reporters:
reporters.append(reporter)
primary["reporters"] = reporters
primary["rule_ids"] = sorted(
set(primary.get("rule_ids") or []) | set(secondary.get("rule_ids") or [])
)
primary.setdefault("anchors", []).append(
{
"id": secondary_id,
"file": secondary.get("file"),
"line": secondary.get("line"),
"category": secondary.get("category"),
}
primary["rule_ids"] = bounded_rule_ids(
list(primary.get("rule_ids") or [])
+ list(secondary.get("rule_ids") or [])
)
anchors = primary.setdefault("anchors", [])
if len(anchors) < MAX_RULE_IDS_PER_FINDING:
anchors.append(
{
"id": secondary_id,
"file": secondary.get("file"),
"line": secondary.get("line"),
"category": secondary.get("category"),
}
)
primaries = [
record
for record in ordered
@ -3373,6 +3357,19 @@ def final_tally() -> None:
verified_ids = {
record["candidate"].get("id") for record in reviewed
}
def disposition_for(record: Mapping[str, Any]) -> str:
candidate = record["candidate"]
if str(candidate.get("id")) in folded:
return "duplicate"
if candidate_key(candidate) in reported_keys and record["kept"]:
return "reportable"
if record["kept"]:
return "deferred-by-cap"
if record.get("verdict") is None and use_verify:
return "verification-incomplete"
return "refuted"
for candidate in state.get("candidates") or []:
record = next(
(
@ -3391,30 +3388,14 @@ def final_tally() -> None:
)
)
continue
if str(record["candidate"].get("id")) in folded:
disposition = "duplicate"
elif candidate_key(record["candidate"]) in reported_keys and record["kept"]:
disposition = "reportable"
elif record["kept"]:
disposition = "deferred-by-cap"
elif record.get("verdict") is None and use_verify:
disposition = "verification-incomplete"
disposition = disposition_for(record)
if disposition == "verification-incomplete":
verification_incomplete += 1
else:
disposition = "refuted"
ledger.append(ledger_entry(record, disposition))
for record in sweep_reviewed:
if str(record["candidate"].get("id")) in folded:
disposition = "duplicate"
elif candidate_key(record["candidate"]) in reported_keys and record["kept"]:
disposition = "reportable"
elif record["kept"]:
disposition = "deferred-by-cap"
elif record.get("verdict") is None and use_verify:
disposition = "verification-incomplete"
disposition = disposition_for(record)
if disposition == "verification-incomplete":
verification_incomplete += 1
else:
disposition = "refuted"
ledger.append(ledger_entry(record, disposition))
votes = (
@ -3623,27 +3604,11 @@ def load_renderer() -> Any:
raise WorkflowDataError("could not load the report renderer")
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
parity_checks = (
("category list", tuple(module.CATEGORIES), CATEGORIES),
("issue type list", tuple(module.ISSUE_TYPES), ISSUE_TYPES),
("effort tiers", tuple(module.EFFORT_TIERS), EFFORT_TIERS),
("review modes", tuple(module.REVIEW_MODES), REVIEW_MODES),
)
for label, renderer_value, engine_value in parity_checks:
if renderer_value != engine_value:
raise WorkflowDataError(
f"the renderer's {label} does not match this engine"
)
if module.COMPILED_RULE_ID_RE.pattern != COMPILED_RULE_ID_RE.pattern:
raise WorkflowDataError(
"the renderer's compiled-ID pattern does not match this engine"
)
return module
def render_report() -> None:
state = load_state()
assert_workspace_unchanged(state)
products_rel = str(state["products_rel"])
evidence_rel = str(state["evidence_rel"])
metadata_rel = str(state["metadata_rel"])

3
.fabro/workflows/code-review/scripts/git_readonly.py
View file

@ -12,7 +12,8 @@ from __future__ import annotations
import sys
if not sys.flags.isolated or not sys.flags.safe_path:
safe_path = getattr(sys.flags, "safe_path", sys.flags.isolated)
if not sys.flags.isolated or not safe_path:
print(
"git_readonly.py: Python isolated mode is required; invoke with python3 -I",
file=sys.stderr,

46
.fabro/workflows/code-review/scripts/publish_pr.py
View file

@ -62,7 +62,6 @@ CANONICAL_FILE_NAMES = (
)
REVIEW_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.:-]{0,127}$")
FINDING_ID_RE = re.compile(r"^R[1-9][0-9]*$")
REPO_RE = re.compile(
r"^[A-Za-z0-9][A-Za-z0-9._-]{0,99}/[A-Za-z0-9][A-Za-z0-9._-]{0,99}$"
)
@ -873,7 +872,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 FINDING_ID_RE.fullmatch(
if not isinstance(finding_id, str) or not renderer.FINDING_ID_RE.fullmatch(
finding_id
):
fail(f"plan {field}.finding_id is invalid")
@ -1174,8 +1173,9 @@ class BatchPoster:
self.posted.update(landed & set(self.by_id))
return [fid for fid in finding_ids if fid not in landed]
def post_individually(self, finding_ids: Sequence[str]) -> None:
"""Per-comment fallback that isolates unpostable comments (R15)."""
def post_individual_pass(self, finding_ids: Sequence[str]) -> List[str]:
"""Post once and return writes with an ambiguous server result."""
ambiguous: List[str] = []
for finding_id in finding_ids:
status = self.post_review([finding_id])
if status in (200, 201):
@ -1185,24 +1185,30 @@ class BatchPoster:
"GitHub could not resolve the diff position (422)"
)
elif self.is_server_failure(status):
missing = self.reconcile([finding_id])
if missing:
# Verified missing, so one retry cannot duplicate (R14).
retry_status = self.post_review([finding_id])
if retry_status in (200, 201):
self.posted.add(finding_id)
elif retry_status == 422:
self.failed[finding_id] = (
"GitHub could not resolve the diff position (422)"
)
else:
still_missing = self.reconcile([finding_id])
if still_missing:
self.mark_failed(
still_missing, self.DROPPED_DETAIL
)
ambiguous.append(finding_id)
else:
self.failed[finding_id] = f"GitHub refused the comment (HTTP {status})"
return ambiguous
def post_individually(self, finding_ids: Sequence[str]) -> None:
"""Per-comment fallback that isolates unpostable comments (R15).
Reconcile ambiguous writes once per pass. This preserves duplicate
safety without re-paginating the complete comment history for every
failed comment.
"""
ambiguous = self.post_individual_pass(finding_ids)
if not ambiguous:
return
missing = self.reconcile(ambiguous)
if not missing:
return
# Verified missing, so one retry cannot duplicate (R14).
retry_ambiguous = self.post_individual_pass(missing)
if retry_ambiguous:
still_missing = self.reconcile(retry_ambiguous)
if still_missing:
self.mark_failed(still_missing, self.DROPPED_DETAIL)
def post_batch(self, batch_ids: Sequence[str]) -> None:
to_send = [fid for fid in batch_ids if fid not in self.posted]

43
.fabro/workflows/code-review/scripts/render_report.py
View file

@ -17,33 +17,28 @@ import json
import hashlib
import os
import re
import sys
from pathlib import Path, PurePosixPath
from typing import Any, Dict, List, Mapping, NoReturn, Sequence, Tuple
from typing import Any, Dict, List, Mapping, NoReturn, Optional, Sequence, Tuple
from urllib.parse import quote
sys.path.insert(0, str(Path(__file__).resolve().parent))
from review_contract import ( # noqa: E402
CATEGORIES,
COMPILED_RULE_ID_RE,
EFFORT_TIERS,
FINDING_ID_RE,
ISSUE_TYPES,
MAX_RULE_IDS_PER_FINDING,
REVIEW_MODES,
)
CANONICAL_SCHEMA_VERSION = 4
TEMPLATE_RELATIVE_PATH = ("..", "templates", "report.html")
PAYLOAD_PLACEHOLDER = "__CODE_REVIEW_PAYLOAD__"
CATEGORIES = (
"correctness",
"reuse",
"simplification",
"efficiency",
"altitude",
"conventions",
"test-coverage",
)
ISSUE_TYPES = (
"bug",
"security",
"performance",
"maintainability",
"test",
"style",
"documentation",
)
SEVERITIES = ("HIGH", "MEDIUM", "LOW")
FINDING_VERDICTS = ("CONFIRMED", "PLAUSIBLE", "UNVERIFIED")
VOTE_VERDICTS = ("CONFIRMED", "PLAUSIBLE", "REFUTED")
@ -56,17 +51,7 @@ DISPOSITIONS = (
)
VERIFICATION_STATUSES = ("complete", "partial", "skipped-low-effort")
COMPLETION_STATUSES = ("complete", "partial")
EFFORT_TIERS = ("low", "medium", "high", "xhigh", "max")
REVIEW_MODES = ("changes", "commit", "files")
FINDING_ID_RE = re.compile(r"^R[1-9][0-9]*$")
COMPILED_RULE_ID_RE = re.compile(
r"^(builtin|repo):"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?"
r"/"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?$"
)
MAX_TEXT = 8000
MAX_RULE_IDS_PER_FINDING = 50
MAX_LOCATION_LINES = 50
UNVERIFIED_FINDING_NOTE = (
"This finding comes from a low-effort single-pass review and was not "

39
.fabro/workflows/code-review/scripts/review_contract.py Normal file
View file

@ -0,0 +1,39 @@
#!/usr/bin/env python3
"""Shared closed-contract constants for the code-review workflow scripts.
Python 3.9-compatible. Standard library only.
"""
from __future__ import annotations
import re
CATEGORIES = (
"correctness",
"reuse",
"simplification",
"efficiency",
"altitude",
"conventions",
"test-coverage",
)
ISSUE_TYPES = (
"bug",
"security",
"performance",
"maintainability",
"test",
"style",
"documentation",
)
EFFORT_TIERS = ("low", "medium", "high", "xhigh", "max")
REVIEW_MODES = ("changes", "commit", "files")
FINDING_ID_RE = re.compile(r"^R[1-9][0-9]*$")
COMPILED_RULE_ID_RE = re.compile(
r"^(builtin|repo):"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?"
r"/"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?$"
)
MAX_RULE_IDS_PER_FINDING = 50

24
.fabro/workflows/code-review/scripts/rule_loader.py
View file

@ -21,11 +21,16 @@ from __future__ import annotations
import hashlib
import json
import re
import sys
from pathlib import Path
from typing import Any, Dict, Iterable, List, Mapping, Optional, Sequence, Tuple
import yaml
sys.path.insert(0, str(Path(__file__).resolve().parent))
from review_contract import CATEGORIES, COMPILED_RULE_ID_RE # noqa: E402
class RuleLoaderError(ValueError):
"""A deterministic rule-configuration failure."""
@ -48,25 +53,8 @@ MAX_YAML_DEPTH = 40
LAYERS = ("builtin", "repo")
MODES = ("merge", "override")
# Kept in lockstep with the engine's closed category list; the engine asserts
# equality before compiling rules.
CATEGORIES = (
"correctness",
"reuse",
"simplification",
"efficiency",
"altitude",
"conventions",
"test-coverage",
)
ID_RE = re.compile(r"^[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?$")
COMPILED_ID_RE = re.compile(
r"^(builtin|repo):"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?"
r"/"
r"[a-z0-9](?:[a-z0-9.-]{0,62}[a-z0-9])?$"
)
COMPILED_ID_RE = COMPILED_RULE_ID_RE
# The default pack applies only to files no other built-in pack matches, and
# the repository-instructions pack applies to every file. Both behaviors are

84
lib/components/fabro-workflow/src/artifact.rs
View file

@ -521,23 +521,47 @@ pub async fn resolve_text_or_blob_ref(value: &Value, run_store: &RunStoreHandle)
}
}
/// Resolve a structured JSON value from inline context or a Fabro-managed
/// blob reference.
/// Resolve a structured JSON value from inline context or Fabro-managed blob
/// references at any depth.
///
/// 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) async fn resolve_json_value(value: Value, run_store: &RunStoreHandle) -> Result<Value> {
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))
pub(crate) fn resolve_json_value<'a>(
value: Value,
run_store: &'a RunStoreHandle,
) -> BoxFuture<'a, Result<Value>> {
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),
}
})
}
/// Resolve a flat workflow context key (`context.NAME` or `NAME`) to a
@ -966,6 +990,40 @@ 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;