Skip to content

fix(recipes): retain includes and literals inside multiline attributes - #658

Merged
juliankang4 merged 2 commits into
midagedev:mainfrom
Burntgogi:fix/issue637-attribute-inputs
Oct 22, 2026
Merged

juliankang4 merged 2 commits into
midagedev:mainfrom
Burntgogi:fix/issue637-attribute-inputs

Conversation

@Burntgogi

@Burntgogi Burntgogi commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Change

Refs #637; resolves only its multiline-attribute input-discovery item, not the other tracking items.

The Rust module walk scans includes and repository-path literals on inner attribute lines before skipping module/cfg interpretation. An attribute's inner brace cannot close the enclosing inline module. This uses the existing scanners and ownership partitions, with no second grammar, key relaxation or new cache.

The existing module-selection regression includes both an inner include_str! and an inner repository-path literal. The contributor check documentation states this input contract.

Verification

Base 69b66367; head abc846f.

  • Actual compiled consumer with #[error(... include_str!("../../../outside.txt") ...)]: on unchanged main, changing the file changed program output from outside1 to outside2 while the real key/ledger CLI still returned skip. With this fix, the same edit returns run.
  • The focused module-selection regression passes for includes and path literals, with its existing feature/module cases intact.
  • The production fix (2f1d9e3) integrated cleanly with recipes: key reached inputs and persist structural reach #587 (94ee6b2) in an isolated detached worktree. The same actual compiled consumer and fresh-process key/ledger scenario returned run with the new reached-key implementation too.
  • The combined recipes: key reached inputs and persist structural reach #587 + attribute-input tree also passes the full public key/ledger regression suite: 22 tests, 67.355 s.
  • Owning static checks: fmt, check-arch, check-comments, and check-levers passed. The first concurrent check-recipes runs in two worktrees detected each other's real CPU-guard fixture processes; after restoring host isolation, the unchanged full check-recipes passed at 2f1d9e34 in 124 s. The 1,200-second bound and every check remain enabled.
  • No NVIDIA execution claim. Local pre-review cannot run without the Claude CLI; remote review/CI remain separate.

Review follow-up at abc846f

The non-blocking attribute-brace coverage note is addressed without changing production code. The existing selection fixture now has a standalone aligned } inside stringify! under a test-only inline module. Its inner include, inner path literal and following include remain test-only for the explicitly selected targets. Removing the guard via an in-memory AST mutant makes all three new cases fail by admitting ordinary build recipes.

Actual Rust compilation accepts that same fixture both normally and with --test; removing the test-only include files leaves the normal build green and makes the test build fail by filename. The final five-unit owning static run at committed abc846f8 passed 5 / 5, wall 147 s (full check-recipes 127 s), with unchanged limits and no concurrent worktree check.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo labels Oct 22, 2026
github-actions[bot]
github-actions Bot previously approved these changes Oct 22, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CLEAN: No blocking defect found in the attribute input-discovery change.

Non-blocking

⚠ Cover the attribute-brace guard: tools/recipes.py:9633.
The fixture never exercises the new attr_depth == 0 guard at tools/recipes.py:1249; removing it leaves the fixture's scans unchanged.
Extend this test with a multiline attribute containing stringify!({ ... }) inside a #[cfg(test)] inline module, with its inner } aligned to the module declaration. Assert that inputs inside and after the attribute remain test-only. Removing the guard incorrectly moves those inputs into normal build inputs.

Checked

Duplicates: no functions, types, or constants added; existing include and literal scanners reused.
Complexity: _walk adds one brace-state condition.
Silent failure: unresolved includes still reach check/key errors; no new fallback introduced.
Tests: isolated in-memory walks retain both attribute inputs; restoring the early skip loses both. Removing the brace guard leaks test-only inputs in an adversarial fixture while leaving the existing fixture's scans unchanged.
Gates: no pins or thresholds moved; no cases removed.
Escape catalog: no C1–C4 patterns match. No inference hot path changed.

Notes

No Rust file changed; dryer and crapper did not run.
Validation used extracted walker code and in-memory fixtures. Full recipe-selection and recipes self-tests were not rerun.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

@Burntgogi

Burntgogi commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Addressed the non-blocking regression-coverage note in abc846f.

The existing module-selection fixture now puts an aligned standalone } inside a multiline stringify! attribute under #[cfg(test)]. Its include, repository-path literal, and include after the attribute must select the explicit test recipes and not ordinary build recipes. The existing conservative whole-build cases keep their original contract.

Verification: the new fixture passes with the production guard. Removing only attr_depth == 0 through an in-memory AST mutation makes all three new cases fail by selecting the ordinary build recipes. The actual fixture also compiles with rustc both normally and as tests; removing its test-only includes leaves the normal build successful and makes the test build fail naming those files. No production behavior or threshold changed by this follow-up. Full owning checks at committed abc846f8 passed 5/5, wall 147 s; check-recipes ran in 127 s, with unchanged limits and no concurrent worktree check.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the last review: the attribute-brace regression request is fixed at tools/recipes.py:9688.

Verdict: CLEAN: The added regression detects the brace-state defect and preserves test-only input selection.

Checked

Duplicates: 0 new production functions, types or constants; the existing module-selection test is extended.
Complexity: module_gate_self_test gains fixture data and one assertion loop; production code is unchanged.
Silent failure: 0 new sites. Explicit set assertions reject missing inputs and inputs leaking into normal builds.
Tests: All 13 in-memory selection assertions pass. Removing the attr_depth == 0 guard at tools/recipes.py:1249 fails all three new assertions. Restoring the early attribute skip also fails the regression.
Gates: no pins or thresholds moved; no cases removed.
Escape catalog: no C1–C4 patterns match. No inference hot path changed.

Notes

No Rust file changed; dryer and crapper did not run.
Validation used production selection code with fixture I/O and justfile loading replaced in memory. The full recipes self-test suite was not rerun.

🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)

@juliankang4
juliankang4 merged commit 57d1294 into midagedev:main Oct 22, 2026
7 checks passed
@juliankang4

Copy link
Copy Markdown
Collaborator

@Burntgogi Thanks for the contribution. The review is done, so I'm merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants