Repository navigation
Conversation
github-actions
Bot
added
the
tools
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: MUST_FIX: Two reachable startup/destructor paths can change behavior without changing a green ledger key.
Must fix
1. Retain startup expansions of known source macros
tools/recipes.py:4701 exempts known macros even when their invocation is outside the reached items. A reached dependency can have an unreached module containing macros::boot!(), where a regular library exports boot! and emits an .init_array initializer calling crate::hidden::run(). The initializer runs, but the invocation, macro definition and hidden helper are all excluded from the key. Editing the helper therefore preserves the green key and skips the gate. In-memory input selection reproduces all three omissions.
Close this path before green ledger rows conceal startup changes. Conservatively retain source and qualified macro bounds; replace the condition and body with:
if (macro not in safe_macros or "::" in call
or any(macro in self.macros(rk) for rk in roots[pname])
or any(self.exported(p).get(macro) for p in linked_packages)):
opaque = True2. Include implicit impl calls from modules with other suffixes
tools/recipes.py:4591 now uses _close to discard compilation inputs, but tools/recipes.py:4203 excludes impl files whose names do not end in .rs. With #[path = "drop.rust"] mod drop;, a selected binary that constructs dep::Token and lets it leave scope runs its Drop implementation there. Both that file and a helper it calls are omitted; changing either preserves the green key. In-memory checks retain the identical implementation in drop.rs.
Fix the shared impl scanner now so destructor changes cannot inherit stale green results. Replace lines 4202–4205 with:
for f in files:
facts = self.facts(rk, f)Non-blocking
⚠ Exercise mandatory proc-macro inputs without opaque attributes: tools/recipes-key-test.py:373 activates the opaque fallback, so deleting mandatory.add(pname) at tools/recipes.py:4642 still retains every input this test asserts. The mutation was checked in memory. Isolate that branch now by adding this before the existing baseline:
ws.green("gate-linked")
with ws.changed("crates/macros/src/helper.rs", "pub fn observe() { panic!(); }\n"):
ws.assert_status(ws.keys("gate-linked", ledger=True), {"gate-linked": "run"})Checked
Duplicates: no duplication finding to validate. No dryer pairs were supplied.
Complexity: no Rust functions changed; crapper supplied no functions to prioritize.
Silent failure: omitted inputs flow into closure-scoped key assembly; a matching green key makes ledger_status skip.
Tests: in-memory graph checks reproduced both omissions and verified both proposed fixes. The mandatory-input mutant retains all three asserted files in the existing fixture; the proposed linked-only case exposes it.
Gates: no tolerances moved. The removed linkage-only mutation has a dated contract rationale in the PR body.
Escape catalog: C1–C4 did not match the changed shell code.
Notes
Dryer and crapper did not run because no Rust file changed. Full CLI and existing self-test suites were not run during validation.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
There was a problem hiding this comment.
Since the last review:
- Must-fix 1 is still open: tools/recipes.py:4701.
- Must-fix 2 is still open: tools/recipes.py:4203.
Verdict: MUST_FIX: Both paths still allow changed executable behavior to reuse a green ledger key.
Must fix
1. Retain startup expansions of known source macros
tools/recipes.py:4701 exempts known macros outside reached items. A reached dependency can contain an unreached module invoking macros::boot!(), where a regular library exports boot! and emits an .init_array initializer calling crate::hidden::run(). The invocation, macro definition and hidden helper are omitted from the key. Editing the helper preserves the green key and skips changed startup behavior.
Fix the shared scanner before more recipes record stale green results. Replace the condition and body with:
if (macro not in safe_macros or "::" in call
or any(macro in self.macros(rk) for rk in roots[pname])
or any(self.exported(p).get(macro) for p in linked_packages)):
opaque = True2. Include implicit impl calls from modules with other suffixes
tools/recipes.py:4203 excludes impl files without a .rs suffix. With #[path = "drop.rust"] mod drop;, a selected binary constructing dep::Token runs its Drop implementation there when the value leaves scope. Both the implementation file and its called helper are omitted; editing either preserves the green key.
Fix the shared impl scanner before destructor changes inherit stale green results. Replace lines 4202–4205 with:
for f in files:
facts = self.facts(rk, f)Checked
Duplicates: no new definitions in the incremental diff; no dryer pairs supplied.
Complexity: no Rust functions changed; crapper supplied no candidates.
Silent failure: both omitted-input paths flow through key filtering and closure-scoped assembly to ledger reuse.
Tests: in-memory input-selection probes reproduced both omissions. Edits to the omitted files preserved the selected-input digest; both proposed fixes retained those inputs and made their edits change the digest.
Gates: no thresholds, pins or assertions removed; extern-crate coverage was extended and dependency metadata corrected.
Escape catalog: C1–C4 do not match the incremental changes.
Notes
Dryer and crapper did not run because no Rust file changed. Full CLI and self-test suites were not run; probes used in-memory inputs with persistent caching disabled.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
|
Addressed both MUST_FIX items from #587 (review) in f9c69bb.
Four review-focused CLI methods pass. Four real CPU Cargo programs compile and execute (exported regular-library startup, local macro-generated Drop, builtin-named source macro, and nonstandard Drop path): helper edits change output source-macro1→source-macro2; caller=run, unrelated=skip. Comments, Python compilation and the 69-tool registration check pass. Both complete suites are now running on this exact published head; no complete-suite pass is claimed yet. PR body updated with current counts and pending status. |
There was a problem hiding this comment.
Since the last review:
- Must-fix 1 is fixed: known source macro invocations add structural roots or retain opaque bounds at tools/recipes.py:4692.
- Must-fix 2 is fixed: implicit impl scanning includes modules without a
.rssuffix at tools/recipes.py:4203.
Verdict: CLEAN: The incremental changes close both previously reported key omissions, with no new must-fix finding.
Checked
Duplicates: 4 added or renamed definitions checked; no duplicate owner found. No dryer pairs supplied.
Complexity: no crapper candidates; key_files adds closure iteration and macro/glob branches, and _close adds root seeding.
Silent failure: 2 empty-result paths followed. Absent extra roots preserve ordinary reach; unresolved globs force opaque retention.
Tests: in-memory mutants dropping extra roots at tools/recipes.py:4505, restoring suffix filtering at tools/recipes.py:4203, dropping mandatory proc macros at tools/recipes.py:4632, and marking enum globs opaque at tools/recipes.py:4702 violate the corresponding assertions. Helper edits change the selected-input digest; unrelated binaries exclude those helpers. A further macro-to-dependency startup scenario retains its downstream helper.
Gates: no thresholds, pins or assertions removed.
Escape catalog: C1–C4 do not match the incremental changes.
Notes
Dryer and crapper did not run because no Rust file changed. Full CLI/cache and self-test suites were not run; verification used in-memory structural probes with persistent caching bypassed.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
|
f4335da fixes the precision regression exposed by the complete f9c69bb self-test (13 unnecessary-key failures). Qualified source macros are resolved and seeded through the same structural expansion edges as reached calls, instead of unconditionally widening their callers. Per-definition bodies come from the canonical unit parser; nested expansions/attributes still conservatively bound the invoker. Unrelated opaque items elsewhere in a macro provider do not falsely widen that caller. All 86 unchanged synthetic Reach assertions pass; no assertion was removed or weakened. Four focused CLI methods pass with two added matrix variants. Six real CPU Cargo programs compile and execute, including pure qualified source-macro calls and regular-source → procedural startup: changed helper output source-macro1→source-macro2, caller=run, unrelated=skip. Pure source calls retain unused-file exclusion. Real recipe counts remain 604→560 and 604→559. Both complete suites are running on this exact published head; the PR body explicitly records the previous failure and pending full verification. |
There was a problem hiding this comment.
Since the last review:
- Earlier must-fix 1 remains fixed: source macro invocations seed structural reach.
- Earlier must-fix 2 remains fixed: implicit impl scanning includes non-
.rsmodules.
Verdict: MUST_FIX: Qualified macro expansions can omit behavior-changing caller imports from ledger keys.
Must fix
1. Keep opaque bounds for macro-generated imports
tools/recipes.py:4719: When dep::startup::run() invokes macros::boot!() with exported body () => {{ use crate::hidden::run; run(); }}, this check accepts the body and line 4745 bypasses opacity. rust_facts removes the import path from the macro's sites; Reach copies its sites and identifiers into the invoker without its import bindings. The invoker's crates/dep/src/hidden.rs is excluded from the key. Editing its output leaves the selected-input digest unchanged and an existing green recipe skippable.
Fixing this classifier now prevents stale green entries from accumulating. Retain the invoker's compilation bound for imports, including explicit crate links:
if body is None or re.search(r"\b(?:use|extern\s+crate)\b", body) or any(_key_attributes(body)) or any(
match.group(1).removeprefix("r#") not in syntax_words
for match in _MACRO_PATH_CALL.finditer(body)):
opaque = TrueAdd the imported-helper variant to the existing macro fixture and assert that editing hidden.rs changes its ledger status to run.
Checked
Duplicates: the two added definitions reuse the existing lexer and unit parser. No dryer pairs supplied.
Complexity: key_files adds qualified-call resolution and macro-body safety branches in tool code.
Silent failure: unresolved qualified calls and missing macro bodies retain opacity; macro-generated imports bypass it.
Tests: an in-memory production Reach probe excludes the imported helper and leaves its selected-file digest unchanged after an edit. The proposed guard retains the helper and changes that digest. The direct-path control still excludes the unused sibling. The inline replacement parses as valid Python and sits on an added line.
Gates: no thresholds or pins moved; no assertions removed.
Escape catalog: C1–C4 do not match the incremental diff.
Notes
Dryer and crapper did not run because no Rust file changed. Full CLI/cache suites were not run; validation used in-memory structural probes with persistent caching bypassed.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| body = _source_macro_bodies(self.text(h[1]), h[0][2]).get(h[2]) | ||
| # Nested expansions/attributes may emit names absent from source edges. | ||
| # Bound the invoker, not an unrelated item elsewhere in the macro crate. | ||
| if body is None or any(_key_attributes(body)) or any( |
There was a problem hiding this comment.
A qualified macro that imports a caller helper loses that helper's binding during reach analysis; retain the invoker's compilation bound so helper edits invalidate the key.
| if body is None or any(_key_attributes(body)) or any( | |
| if body is None or re.search(r"\b(?:use|extern\s+crate)\b", body) or any(_key_attributes(body)) or any( |
There was a problem hiding this comment.
Since the last review:
- Macro-generated imports: fixed at tools/recipes.py:4719. Import-bearing expansions retain the invoker's compilation inputs and forward dependencies.
Verdict: CLEAN: The added guard closes the reported stale-ledger-key path.
Checked
Duplicates: 0 new definitions; the imported-helper case extends the existing fixture. No dryer pairs supplied.
Complexity: key_files gains one safety condition in tool code; no engine hot path changes.
Silent failure: 1 missing-body fallback followed to retained compilation inputs.
Tests: the imported-helper variant reaches the file-presence assertion at tools/recipes-key-test.py:331 and the ledger run/skip assertion after a helper edit. Dropping the import guard restores the path that bypasses conservative retention. Both changed Python files parse.
Gates: no thresholds or pins moved; no assertions removed.
Escape catalog: C1–C4 do not match the incremental diff.
Notes
Dryer and crapper did not run because no Rust file changed. Validator checks were static code inspection and Python parsing; full CLI/cache suites were not rerun.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
|
Addressed #587 (review) in d200e41 and 758109c: source-macro bodies containing The qualified imported-helper case is now a permanent variant. I also reproduced the same root problem with a local macro expanding For context, the complete prior-head f4335da suite passed (203 recipes, 0 failures), as did all 20 CLI methods. Both suites are now running on committed 758109c; the PR body distinguishes prior-head proof from pending final-head proof. Current real-repository key sizes remain 604→560 and 604→559. |
|
Final local verification is complete on 758109c:
The PR body now records completed final-head local proof rather than pending runs, plus the scope/safety tradeoffs and review fixes. Latest observed GitHub T0 is still in progress and maintainer review is required; neither is represented as passed. No NVIDIA/model execution or merge claimed. |
|
Addressing the actual T0 timeout rather than raising its 1200-second bound. Current head: 1ca2973; upstream main f956b65 is integrated. A cProfile scenario with six repeated real cached recipe-input requests observed 12 full freshness checks and 36 calls to Python The same six-request scenario now observes 6 freshness checks and 0 compilation calls after warm-up. Exact file-set fingerprints are unchanged for both sampled GPU recipes (560 and 559 key files). This is call-count/profile evidence, not a controlled throughput claim. The existing imported-code regression now first establishes a green warm-cache lookup, then edits each analysis module while preserving both size and mtime; every changed module is still refused rather than emitted as skippable. That focused regression and the comments guard pass. All CLI regressions and both previously timed-out commands are running on this head with the original |
Current-head performance verification (1ca2973)The unchanged 1,200-second bounds now pass locally after integrating upstream main f956b65:
The first local integrated run finished below the timeout but failed English shell-diagnostic fixtures under inherited Korean locale. The C-locale run above passed all 869 card tests and all 99 tree-commit checks without changing their assertions or increasing a timeout. A same-tree six-request cached-lookup profile (before upstream integration) reduced code compilations 36 → 0 and full snapshot validations 12 → 6; both sampled input file sets and their hashes were identical. Exact source bytes are still reread, restored-mtime source changes are refused, and final key publication still revalidates its inputs. This is call-count evidence, not a controlled throughput benchmark. The temporary profiler has been removed. |
|
CI result update for 1ca2973: the EPYC 7763 run has now completed, and both recipes-self-test and check-recipes still hit the unchanged 1,200 s unit bound (rc 124); the other 102 units passed. The earlier local 859 s integrated pass is real but is not CI success. I am continuing the performance fix rather than rerunning the same failing check or raising its bound. The retained profile identifies repeated full-corpus/recorded-read validation and path resolution as the remaining cached-lookup cost; the next change will retain content and publication checks. |
|
Second CI-performance fix published at 726256e; the 1,200-second bounds are unchanged. The observed cold-rebuild profile exposed 8.8 million repeated schema-introspection calls per graph validation; schema descriptors are now reused while every value is still strictly validated (including bool ≠ int). A retained before/after checker agreed on all 384 acceptance/rejection comparisons. The two full cold profiles retain exactly 78,342 nodes and 2,260 fact records. Uncached recipe analysis now reuses immutable exact-text file syntax, and collects module ancestors once per root/file through the existing bases index rather than rescanning every module for every reached unit. Bindings, macro resolution, active-owner decisions and file/content/publication validation are not cached away. Four real recipe input path sets and their SHA-256 hashes were identical; the sampled analysis still performed all 8 full snapshot validations before and after. Profile timings are not a controlled throughput benchmark. All 20 CLI safety methods pass (59.129 s). Full |
|
Two-CPU smoke update at the same 726256e source:
|
|
726256e actual CI has completed: 103/104 units passed. |
|
Local full-check result for Upstream T0 for this exact head is still running: https://github.com/midagedev/bloomery/actions/runs/39594669632 . This local result is not yet a claim that upstream CI passed. |
|
Exact-head upstream T0 for
https://github.com/midagedev/bloomery/actions/runs/39594669632/job/119203910635 The unchanged limit is 1,200 s per unit. This runner was an Intel Xeon Platinum 8573C; the preceding failed run used a different CPU, so these are pass/deadline results, not a controlled speedup benchmark. The local unchanged integrated checker also passed all 146 blocks / 0 ledger skips. The timeout blocker is resolved on this head; ready for maintainer review. |
There was a problem hiding this comment.
Verdict: MUST_FIX: valid Rust calls can be omitted from ledger keys and reuse an outdated green result.
Must fix
1. Keep inputs behind absolute paths and raw identifiers
tools/recipes.py:3431 The path parser ignores ::dep::used::run() and truncates dep::r#type::run(). In-memory Graph/Reach probes confirm that the new key filter drops the called module in both cases. Editing that module leaves the key unchanged, so a recorded green result can skip the changed gate.
Fix this before v2 green entries accumulate with missing inputs. Conservatively mark these forms opaque by replacing this return argument:
opaque or bool(re.search(r"(?<![\w:>\]])::|r#[A-Za-z_]", shape)),Add CLI regressions that edit each called module after recording a green key.
2. Traverse selected source roots without a .rs suffix
tools/recipes.py:4681 A Cargo target with path = "main.rust" and fn main() { dep::used::run(); } retains its own source but drops dep/used.rs. Reach.units assigns the root a data node, so its calls never seed reach. Editing the called module can therefore reuse a green result. The same probe with main.rs retains the dependency.
Fix source classification before these targets produce reusable green keys. Replace Reach.units:
def units(self, rk: RootKey, f: str) -> list[Hit]:
"""Every unit of a Rust module; a non-source data file uses index -1."""
if not f.endswith(".rs") and f not in self.mods(rk).values():
return [(rk, f, -1)]
n = len(self.facts(rk, f).units)
return [(rk, f, i) for i in range(n)] or [(rk, f, -1)]Add a CLI regression with an explicit non-.rs target path.
3. Date the removed FAIL-first coverage
tools/recipes.py:11140 The plain-link hf mutation and its two assertions were removed without a dated reason. AGENTS requires a dated reason for removed coverage.
Record the contract change while its reason is known. Insert after the docstring:
# PIN(2026-10-22): Drop the plain-link hf mutation: v2 keys exclude uncalled runtime code.Checked
Duplicates: 78 new functions, types and constants checked, including nested functions; no proven duplicate. Dryer supplied no pairs.
Complexity: no Rust functions changed; no crapper targets. The change touches tools, not inference hot paths.
Silent failure: 3 cache fallback sites followed; invalid snapshots rebuild, and failed publication retains the computed graph.
Tests: in-memory Graph/Reach probes reproduce the absolute/raw-path and non-.rs-root omissions; the proposed fixes retain the missing inputs. A normal .rs-path control retains its dependency. Both inline suggestions target added lines and parse as Python.
Gates: no thresholds moved; the removed mutation lacks a dated reason (item 3).
Escape catalog: no C1–C4 pattern matched the changed executable code.
Notes
Dryer and crapper did not run because no Rust file changed.
The full CLI suite was not run because its fixtures write files outside the permitted outputs. The PR body reports passing runs but supplies no FAIL-first output for the new tests.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| calls = frozenset(match.group(1) for match in _MACRO_PATH_CALL.finditer(shape)) | ||
| return KeySyntax( | ||
| frozenset(re.findall(r"\bextern\s+crate\s+([A-Za-z_]\w*)", shape)), | ||
| opaque, |
There was a problem hiding this comment.
Absolute and raw paths escape structural reach; retain their owner bounds so a called module edit changes the key.
| opaque, | |
| opaque or bool(re.search(r"(?<![\w:>\]])::|r#[A-Za-z_]", shape)), |
| and the chains `why --recipe` prints.""" | ||
| import copy | ||
|
|
||
| """The runtime key/reach pins and genuine Cargo-linkage explanation chains on the real tree.""" |
There was a problem hiding this comment.
Removed coverage requires a dated reason under AGENTS.
| """The runtime key/reach pins and genuine Cargo-linkage explanation chains on the real tree.""" | |
| """The runtime key/reach pins and genuine Cargo-linkage explanation chains on the real tree.""" | |
| # PIN(2026-10-22): Drop the plain-link hf mutation: v2 keys exclude uncalled runtime code. |
github-actions
Bot
added
the
documentation
|
Addressed all three MUST_FIX items from #587 (review) in 94ee6b2, on current upstream main 69b6636. The new public key/ledger regressions failed before the fix in all three executable-input cases; all 22 CLI methods now pass. Separate compiled Rust consumers change their output and correctly invalidate the called recipe, preserving the unrelated linked recipe. Full owning static verification: 5 units / 0 red, check-recipes 146 blocks / 0 skips, unchanged 1,200 s bounds. The dated hf-coverage rationale is recorded. Full commands/results and the local pre-review prerequisite failure are in the updated PR body. Fresh GitHub review/CI are not yet claimed green. |
|
Additional compatibility check with the independently scoped multiline-attribute input fix in #658: it applies cleanly to |
|
Current-head review handoff: the review gate selected |
1 participant
Change
Closes #529.
Graph.inputsremains the broad compilation/affected/narrow/release bound. New ledger-specificGraph.key_inputsuses uncut structural Reach, not the profile-pruned runtime view, and closes recipe dependencies separately. The v2 key format intentionally invalidates old green keys once.Mandatory inputs remain: the selected target's module/root sources, build scripts/helpers/literals/build dependencies, proc-macro compilation closures, manifest views/globals, dep-info, scripts and explicit file/directory reads. Broad never-skip/tree-scope safety detectors are unchanged.
targets,whyandgapsuse/report the ledger view where appropriate.Compiled invocations of resolved source macros add structural roots even in otherwise unreached modules, so generated startup code and implicit impls retain transitive helpers. Qualified source calls reuse the existing expansion edges. Generated imports, nested expansions and attributes conservatively bound the invoking owner; shadowed or unknown calls remain opaque. Nonstandard
#[path]modules participate in shared implicit-impl rules.Opaque macro expansion conservatively retains its owner's compilation and forward-dependency bounds. Compiled runtime-active owners are scanned for startup attributes (including nested
cfg_attr), unused imports/explicit crate links, and generated/foreign-link obligations. A selected Rust module's filename need not end in.rs. Ambiguous generated includes and foreign linkage retain broad inputs. This does not claim arbitrary proc-macro filesystem completeness.Persistent structural cache
tools/bloomery/reach_cache.pystores the complete structural facts/nodes/edges/rules/indexes as private JSON under~/.cache/bloomery/reach/<checkout-id>/graph.json, not per-recipe reach lists or green verdicts. Code, metadata, source corpus/enumeration, symlink referents and observed reads/probes are content checked independently of git/mtime. Fixed schema, checksum and index validation precede hydration; input stability is rechecked before publication/key output. Corrupt/foreign/stale caches rebuild. Imported code is bound to its actual loaded code marker; cache data is never executable code.A measured publication hotspot used millions of token-sized Python writes; publication now uses the standard C JSON encoder with one write, preserving the JSON protocol.
Current performance cutover
The private snapshot format is now 2 (the ledger key format remains v2). Only structural state is persisted: the recomputable
_resolvedlookup memo is omitted, and hydration discards any eager pre-snapshot resolver results. Nodes, edges, facts, rules, dispatch/index validation and fresh code/source/metadata/read/probe checks remain. Obsolete validation clauses for the omitted memo were removed; no persisted structural validation was skipped.The exact full
key_self_testprofile on the preceding head passed every assertion and recorded 1,938,101,399 calls; encoding accounted for 228 cumulative seconds. A separate real-tree field probe identified 299,230 resolver-memo entries / 75,153,569 encoded bytes. This cutover removes that derived data rather than raising deadlines or dropping coverage.Current-head verification (94ee6b2)
Current upstream main
69b66367is integrated. All three MUST_FIX items in the 2026-10-22 review are addressed:.rssuffix; non-source data remains a data node.Verification:
skip.used1/raw1toused2/raw2; the public ledger returnedrunfor the called recipe andskipfor the unrelated linked recipe. The source mutation preserved size/mtime; the temporary consumer's dep build was cleaned before recompilation to separate Cargo's own mtime reuse from the content-key check.LC_ALL=C bash tools/contrib-check.sh --tier t0 --only '^(fmt|check-recipes|check-comments|check-arch|check-levers)$': 5 units / 0 red, wall 620 s. The fullcheck-recipesran 146 blocks / 0 green-key skips and completed in 598 s; the per-unit limit remains 1,200 s. Header:1c0765bd+local, the same corrected sources subsequently committed as94ee6b2.docs/contrib/local-check.md. No deadline, selector, numeric band or coverage was relaxed.just pre-reviewwas attempted after committing, but exited 75: no Claude CLI is installed, so it reviewed nothing. New-head GitHub review/CI are pending; the earlier green T0 at6e6e15bis not relabelled as this head's CI. No NVIDIA execution or performance claim.