diff --git a/.fabro/workflows/code-review/code-review.fabro b/.fabro/workflows/code-review/code-review.fabro index 0300475b2..c215eda81 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 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 [ diff --git a/.fabro/workflows/code-review/rules/builtin-manifest.json b/.fabro/workflows/code-review/rules/builtin-manifest.json index d5743cf6d..98f31790f 100644 --- a/.fabro/workflows/code-review/rules/builtin-manifest.json +++ b/.fabro/workflows/code-review/rules/builtin-manifest.json @@ -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", diff --git a/.fabro/workflows/code-review/rules/builtin/language/arkts.yaml b/.fabro/workflows/code-review/rules/builtin/language/arkts.yaml index d2fc4a26c..569add24e 100644 --- a/.fabro/workflows/code-review/rules/builtin/language/arkts.yaml +++ b/.fabro/workflows/code-review/rules/builtin/language/arkts.yaml @@ -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 diff --git a/.fabro/workflows/code-review/rules/builtin/language/cpp.yaml b/.fabro/workflows/code-review/rules/builtin/language/cpp.yaml index 779c811a2..a07c69966 100644 --- a/.fabro/workflows/code-review/rules/builtin/language/cpp.yaml +++ b/.fabro/workflows/code-review/rules/builtin/language/cpp.yaml @@ -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 diff --git a/.fabro/workflows/code-review/rules/builtin/language/elm.yaml b/.fabro/workflows/code-review/rules/builtin/language/elm.yaml index 0e894acc6..e102f7b63 100644 --- a/.fabro/workflows/code-review/rules/builtin/language/elm.yaml +++ b/.fabro/workflows/code-review/rules/builtin/language/elm.yaml @@ -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: | diff --git a/.fabro/workflows/code-review/rules/builtin/language/matlab.yaml b/.fabro/workflows/code-review/rules/builtin/language/matlab.yaml index f07d57d9d..1724dc05d 100644 --- a/.fabro/workflows/code-review/rules/builtin/language/matlab.yaml +++ b/.fabro/workflows/code-review/rules/builtin/language/matlab.yaml @@ -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 diff --git a/.fabro/workflows/code-review/rules/builtin/language/python.yaml b/.fabro/workflows/code-review/rules/builtin/language/python.yaml index 4e3fa8b29..d242f8af9 100644 --- a/.fabro/workflows/code-review/rules/builtin/language/python.yaml +++ b/.fabro/workflows/code-review/rules/builtin/language/python.yaml @@ -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 diff --git a/.fabro/workflows/code-review/scripts/code_review.py b/.fabro/workflows/code-review/scripts/code_review.py index b64c61d15..0f7c758ef 100644 --- a/.fabro/workflows/code-review/scripts/code_review.py +++ b/.fabro/workflows/code-review/scripts/code_review.py @@ -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"]) diff --git a/.fabro/workflows/code-review/scripts/git_readonly.py b/.fabro/workflows/code-review/scripts/git_readonly.py index f861d83e1..2fce407a8 100644 --- a/.fabro/workflows/code-review/scripts/git_readonly.py +++ b/.fabro/workflows/code-review/scripts/git_readonly.py @@ -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, diff --git a/.fabro/workflows/code-review/scripts/publish_pr.py b/.fabro/workflows/code-review/scripts/publish_pr.py index 03a828a65..0f0782cb5 100644 --- a/.fabro/workflows/code-review/scripts/publish_pr.py +++ b/.fabro/workflows/code-review/scripts/publish_pr.py @@ -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] diff --git a/.fabro/workflows/code-review/scripts/render_report.py b/.fabro/workflows/code-review/scripts/render_report.py index a7a4e1037..170270c00 100644 --- a/.fabro/workflows/code-review/scripts/render_report.py +++ b/.fabro/workflows/code-review/scripts/render_report.py @@ -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 " diff --git a/.fabro/workflows/code-review/scripts/review_contract.py b/.fabro/workflows/code-review/scripts/review_contract.py new file mode 100644 index 000000000..31bee7ef0 --- /dev/null +++ b/.fabro/workflows/code-review/scripts/review_contract.py @@ -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 diff --git a/.fabro/workflows/code-review/scripts/rule_loader.py b/.fabro/workflows/code-review/scripts/rule_loader.py index fc6646481..3281191bb 100644 --- a/.fabro/workflows/code-review/scripts/rule_loader.py +++ b/.fabro/workflows/code-review/scripts/rule_loader.py @@ -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 diff --git a/lib/components/fabro-workflow/src/artifact.rs b/lib/components/fabro-workflow/src/artifact.rs index 6d2eb1e79..a61f0ba0e 100644 --- a/lib/components/fabro-workflow/src/artifact.rs +++ b/lib/components/fabro-workflow/src/artifact.rs @@ -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 { - 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> { + 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;