Skip to content

Commit c82ffec

Browse files
docs(#14) record + fix repeated Hypatia scan noise (#33)
Durable #14 investigation record (methodology, Linux+Windows results, conclusive-negative reasoning) plus root-cause hunt-down and remediation of the repeated whole-repo Hypatia PR comment: diff-scoped + baseline-aware comment step, .hypatia-ignore, .hypatia-baseline.json, and poison-tolerant lib/common/concurrency.rs. Remaining backlog tracked in #34.
1 parent abc4b49 commit c82ffec

6 files changed

Lines changed: 322 additions & 22 deletions

File tree

.github/workflows/hypatia-scan.yml

Lines changed: 60 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -167,29 +167,80 @@ jobs:
167167
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7
168168
with:
169169
script: |
170+
// Diff-scoped + baseline-aware PR comment.
171+
//
172+
// The scanner reports the WHOLE repo on every run, so a raw
173+
// comment nagged every PR with ~44 pre-existing findings it did
174+
// not introduce. We instead surface only findings that are BOTH
175+
// (a) in a file this PR changed and (b) not in the committed
176+
// baseline. If there is nothing actionable for this PR we post
177+
// no comment at all (the full set stays in the uploaded
178+
// artifact + step summary). See
179+
// docs/wiki/internals/checker-allocation-investigation.md.
170180
const fs = require('fs');
171181
const findings = JSON.parse(fs.readFileSync('hypatia-findings.json', 'utf8'));
172182
173-
const critical = findings.filter(f => f.severity === 'critical').length;
174-
const high = findings.filter(f => f.severity === 'high').length;
183+
const ws = (process.env.GITHUB_WORKSPACE || '').replace(/\/+$/, '');
184+
const relPath = (p) => {
185+
let r = String(p || '');
186+
if (ws && r.startsWith(ws + '/')) r = r.slice(ws.length + 1);
187+
return r;
188+
};
189+
const fp = (f) => `${f.rule_module}/${f.type}:${relPath(f.file)}`;
190+
191+
let baseline = new Set();
192+
try {
193+
const b = JSON.parse(fs.readFileSync('.hypatia-baseline.json', 'utf8'));
194+
baseline = new Set(b.fingerprints || []);
195+
} catch (e) { /* no baseline yet */ }
196+
197+
// Files changed by this PR (paginated).
198+
const files = await github.paginate(github.rest.pulls.listFiles, {
199+
owner: context.repo.owner,
200+
repo: context.repo.repo,
201+
pull_number: context.issue.number,
202+
per_page: 100,
203+
});
204+
const changed = new Set(files.map(f => f.filename));
205+
206+
const baselined = findings.filter(f => baseline.has(fp(f)));
207+
const relevant = findings.filter(
208+
f => changed.has(relPath(f.file)) && !baseline.has(fp(f))
209+
);
210+
211+
const summary =
212+
`Hypatia: ${findings.length} repo-wide finding(s); ` +
213+
`${relevant.length} in files this PR changed; ` +
214+
`${baselined.length} baselined; ` +
215+
`full list in the hypatia-findings artifact.`;
216+
core.notice(summary);
217+
218+
// Nothing this PR can act on -> stay silent.
219+
if (relevant.length === 0) return;
220+
221+
const critical = relevant.filter(f => f.severity === 'critical').length;
222+
const high = relevant.filter(f => f.severity === 'high').length;
175223
176-
let comment = `## 🔍 Hypatia Security Scan\n\n`;
177-
comment += `**Findings:** ${findings.length} issues detected\n\n`;
224+
let comment = `## 🔍 Hypatia Security Scan — findings in this PR's changed files\n\n`;
225+
comment += `**${relevant.length}** finding(s) in files this PR modifies `;
226+
comment += `(of ${findings.length} repo-wide; ${baselined.length} baselined, not shown).\n\n`;
178227
comment += `| Severity | Count |\n|----------|-------|\n`;
179228
comment += `| 🔴 Critical | ${critical} |\n`;
180229
comment += `| 🟠 High | ${high} |\n`;
181-
comment += `| 🟡 Medium | ${findings.length - critical - high} |\n\n`;
230+
comment += `| 🟡 Medium | ${relevant.length - critical - high} |\n\n`;
182231
183232
if (critical > 0) {
184-
comment += `⚠️ **Action Required:** Critical security issues found!\n\n`;
233+
comment += `⚠️ **Action Required:** critical finding(s) in code this PR touches.\n\n`;
185234
}
186235
187-
comment += `<details><summary>View findings</summary>\n\n`;
188-
comment += `\`\`\`json\n${JSON.stringify(findings.slice(0, 10), null, 2)}\n\`\`\`\n`;
236+
comment += `<details><summary>View findings (this PR's files only)</summary>\n\n`;
237+
comment += `\`\`\`json\n${JSON.stringify(relevant.slice(0, 10), null, 2)}\n\`\`\`\n`;
189238
comment += `</details>\n\n`;
239+
comment += `_Pre-existing repo-wide findings are triaged via the Hypatia backlog `;
240+
comment += `issue and suppressed here by \`.hypatia-baseline.json\` / \`.hypatia-ignore\`._\n\n`;
190241
comment += `*Powered by Hypatia Neurosymbolic CI/CD Intelligence*`;
191242
192-
github.rest.issues.createComment({
243+
await github.rest.issues.createComment({
193244
owner: context.repo.owner,
194245
repo: context.repo.repo,
195246
issue_number: context.issue.number,

.hypatia-baseline.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"_comment": "Workflow-owned Hypatia baseline for hyperpolymath/my-lang. Consumed by the 'Comment on PR with findings' step in .github/workflows/hypatia-scan.yml (NOT by the upstream scanner's own --baseline, whose schema is unpublished). A finding is suppressed from the PR comment if its fingerprint 'rule_module/type:repo-relative-path' appears in `fingerprints`. This freezes KNOWN pre-existing findings so only NEW or changed-file findings surface. Entries must be burned down via the tracking issue, not grown. Regenerate the full set from the `hypatia-findings` CI artifact: jq -r '.[] | \"\\(.rule_module)/\\(.type):\\(.file)\"' hypatia-findings.json | sed \"s#.*/my-lang/my-lang/##\".",
3+
"_tracking_issue": "hyperpolymath/my-lang#34 is the burn-down list; this baseline is the suppression list.",
4+
"_partial": true,
5+
"fingerprints": [
6+
"code_safety/admitted:proofs/verification/coq/Typing.v",
7+
"code_safety/unwrap_without_check:dialects/solo/compiler/src/lexer.rs"
8+
]
9+
}

.hypatia-ignore

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
# Hypatia scanner exemptions for hyperpolymath/my-lang
2+
#
3+
# Format (per hyperpolymath/hypatia .hypatia-ignore spec):
4+
# <rule_module>/<rule_type>:<path-fragment> scoped rule exemption
5+
# <rule_module>/*:<path-fragment> whole-module exemption
6+
# <path-fragment> unscoped (any rule)
7+
# Path-fragments are SUBSTRING-matched against repo-relative paths.
8+
# Lines beginning with '#' are comments.
9+
#
10+
# Rationale is recorded inline so future maintainers can re-evaluate each
11+
# exemption. See docs/wiki/internals/checker-allocation-investigation.md for
12+
# how the standing Hypatia backlog was triaged. Burn-down: issue #34.
13+
14+
# --- Non-shipping trees: separate packages, NOT in the root Cargo workspace
15+
# (see Cargo.toml `members`). They are scratch/playground and alternate
16+
# dialect code, intentionally rougher than shipped crates; scanning them on
17+
# every PR produced repeated noise unrelated to any PR's diff.
18+
playground/
19+
dialects/
20+
21+
# --- Scanner false positive: this Nickel file is a POLICY that *bans*
22+
# Dockerfile (`banned_files = ["Makefile", "Dockerfile"]`). The rule fires on
23+
# the substring "Dockerfile" even though the config enforces the very policy
24+
# the rule wants. Correct behaviour, not a defect.
25+
code_safety/ncl_docker_not_podman:.machine_readable/svc/k9/my-lang-metadata.k9.ncl
26+
27+
# --- Property-test scaffolding: deliberate unwrap()/panic! is idiomatic in
28+
# proptest generators and shrink paths (a panic IS the test signal there).
29+
code_safety/unwrap_without_check:src/proptest.rs
30+
code_safety/panic_macro:src/proptest.rs
31+
32+
# --- Safe-by-construction: char::from_digit(d, radix) where d < radix is an
33+
# invariant established immediately above the call site; the unwrap cannot
34+
# fire. Tracked in the backlog issue for an eventual expect()-with-proof.
35+
code_safety/unwrap_without_check:lib/common/string.rs
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
# Investigation Record: Checker Allocation Root-Cause (#14)
2+
3+
Status: **Resolved — root cause is not checker complexity.**
4+
Tracking issue: [hyperpolymath/my-lang#14](https://github.com/hyperpolymath/my-lang/issues/14)
5+
(follow-up to #1 / #12; related #15, #16, #31).
6+
Delivered in PR #29 (harness), PR #30 (reconstructed repro + dhat + Windows CI).
7+
8+
This is a decision/investigation log: it records *why* we did what we did,
9+
the paths taken (including dead ends and a measurement bug we had to fix),
10+
and the conclusions, so future work starts from this baseline instead of
11+
re-deriving it.
12+
13+
---
14+
15+
## 1. Problem statement
16+
17+
The original report (#1): type-checking a ~330 LOC scaffold tool allocated
18+
**16–32 GiB** on `stable-x86_64-pc-windows-msvc`, Windows 11 Pro for
19+
Workstations 26300. PR #12 added `MAX_EXPR_DEPTH = 256` so pathological
20+
inputs produce a clean `ExpressionTooDeep` diagnostic instead of OOM — that
21+
fixed the **symptom**. #14 tracks the **root cause**, which was unknown:
22+
23+
- `check_expr` / `is_assignable_from` appeared structurally linear on Linux.
24+
- The maintainer could not reproduce the OOM on Linux.
25+
- The actual ~330 LOC repro file was never attached to the issue.
26+
27+
Hypothesis to confirm or refute: the cost is **super-linear (possibly
28+
exponential) in the number of nested string-building constructs**
29+
(`str_concat` / `format`), so files well under the depth limit could still
30+
OOM.
31+
32+
## 2. Constraints that shaped the approach
33+
34+
- The exact original file is owner-only — it cannot be obtained or "installed".
35+
- The reproduction OS (Windows) cannot be run inside the Linux execution
36+
environment.
37+
- Per the issue's "out of scope": **no speculative memoisation refactor
38+
without a real measurement first.**
39+
40+
These ruled out "just reproduce it locally" and "just fix it"; the
41+
deliverable had to be *measurement + evidence*, with any fix gated on a
42+
demonstrated hotspot.
43+
44+
## 3. Design of the measurement harness
45+
46+
`crates/my-lang/tests/checker_alloc_scaling.rs`.
47+
48+
Design decisions and rationale:
49+
50+
- **Counting global allocator, gross bytes (not live heap).** The failure
51+
mode under investigation is runaway *allocation*; gross allocation traffic
52+
is what a `heaptrack` / `dhat` "bytes allocated" figure shows, so summing
53+
`Layout::size()` on every `alloc` while a `RECORDING` flag is set models
54+
the reported metric directly.
55+
- **Measure `check()` in isolation.** Parsing / AST construction happens
56+
*outside* the recorded region so parser cost cannot contaminate the
57+
checker signal.
58+
- **Two independent axes**, because "super-linear" has to be pinned to a
59+
variable:
60+
- *breadth* — many functions × many `str_concat` sites (the report's
61+
"aggregate complexity of nested string-building constructs");
62+
- *depth* — one chain whose nesting grows but stays **strictly below
63+
`MAX_EXPR_DEPTH`**, so the #12 guard never fires and we measure the
64+
*genuine* per-level cost rather than the guard's early-out.
65+
- **Doubling sweeps + a per-unit-cost ratio assertion.** Linear ⇒ per-unit
66+
cost is ~flat as input doubles; a quadratic/exponential term ⇒ the
67+
ratio climbs. The test fails with a concrete number, turning a future
68+
regression into a CI failure instead of an opaque OOM on a user's box.
69+
70+
### Dead end / bug found along the way
71+
72+
The first run reported a nonsense `2064` bytes for the smallest breadth
73+
point and a `0.00` depth ratio. Root cause: the two tests run on **parallel
74+
Cargo test threads** and share the process-global counter, so each test's
75+
counter reset raced the other's measured region. Fix: a `MEASURE_LOCK`
76+
mutex making every measured region mutually exclusive (poison-tolerant via
77+
`unwrap_or_else(|e| e.into_inner())`). Recorded here because it is an easy
78+
trap to fall back into if the harness is extended.
79+
80+
## 4. Reconstructed repro (issue item 1)
81+
82+
The original file being unavailable, `tests/fixtures/issue_14_scaffold.my`
83+
(~348 LOC) faithfully rebuilds the *shape*: a code-scaffolding tool whose
84+
output is assembled almost entirely from nested `str_concat` / `format`
85+
constructs, many independent templating sites, moderate per-expression
86+
nesting. An end-to-end test asserts it (a) type-checks cleanly, (b) does
87+
**not** trip the depth guard — proving it is a deep-but-legal program, not a
88+
depth bomb — and (c) allocates `< 64 MiB`.
89+
90+
## 5. Portable profiling (issue item 2)
91+
92+
`heaptrack` / Windows ETW are OS-specific. `dhat` (dhat-rs) is a
93+
cross-platform in-process profiler giving per-call-site allocation data, so
94+
it is the portable stand-in and runs identically on Linux and the Windows CI
95+
leg. Wired as the optional `dhat-heap` feature + the
96+
`examples/dhat_checker_profile.rs` example (emits `dhat-heap.json`,
97+
gitignored).
98+
99+
## 6. Windows coverage
100+
101+
`.github/workflows/checker-scaling.yml` runs the scaling harness on an
102+
`ubuntu-latest` + `windows-latest` matrix and uploads the Linux dhat
103+
profile. This converts "we can't run Windows here" into a permanent,
104+
per-change Windows regression guard rather than a one-off manual report.
105+
106+
## 7. Results
107+
108+
| Measurement | Result |
109+
|---|---|
110+
| Breadth (sites 72→1152) | bytes ≈ double when sites double → **linear** |
111+
| Depth (32→200, under guard) | total ≈ flat ~150–210 KB → no per-level blow-up |
112+
| Reconstructed ~330 LOC scaffold | type-checks in **~1.5 MB** (≈20,000× below 16–32 GiB) |
113+
| dhat (all workloads) | 10.8 MB total / 2.7 MB peak — no gigabyte call site |
114+
| `scaling (windows-latest)` CI | **passed** — same bounded/linear behaviour on msvc |
115+
116+
Structural reason it *must* be linear: each `check_expr` level clones only a
117+
fixed-size builtin signature (`str_concat: (Unknown, Unknown) -> String`);
118+
there is nowhere for the type representation to grow with nesting.
119+
120+
## 8. Conclusion
121+
122+
This is a **conclusive negative**, not merely "couldn't reproduce": we
123+
measured the specific mechanism the report blamed, on the axes that would
124+
expose super-linearity, on the same OS family, and the cost is provably
125+
linear with a small constant. The hypothesised failure mode is structurally
126+
absent from `check_expr` / `is_assignable_from`. Issue items 3 and 4 are
127+
answered; the "no speculative memoisation" call was correct (there was no
128+
checker hotspot to memoise — note #16/#31 later added memoisation as an
129+
independent perf improvement, not as the #14 fix).
130+
131+
## 9. Implications & follow-ups
132+
133+
- **The #12 depth guard is not load-bearing for checker memory safety.** Its
134+
real value is bounding *stack recursion* (recursive `check_expr`, recursive
135+
AST `Drop`, recursive-descent parser). The `MAX_EXPR_DEPTH` doc-comment
136+
should be reframed from "prevents heap blow-up" to "bounds stack
137+
recursion," and the limit reconsidered on its own terms (it currently
138+
rejects deep-but-legal programs at 256 for a problem that was elsewhere).
139+
- **Leading hypotheses for the original 16–32 GiB**, all *outside*
140+
`check_expr`: recursive-descent parser stack/recovery allocation (#15 /
141+
#21), recursive `Drop` of the deep `Box<Expr>` chain, a debug-vs-release
142+
or debug-info/span-table difference on the original Windows toolchain, or
143+
a construct in the exact original file not captured by the reconstruction.
144+
- The still-open *stack-recursion* angle belongs with the parser-side
145+
tracking issue (#15), not here.
146+
147+
## 10. Where the artifacts live
148+
149+
| Artifact | Path |
150+
|---|---|
151+
| Scaling harness | `crates/my-lang/tests/checker_alloc_scaling.rs` |
152+
| Reconstructed repro fixture | `crates/my-lang/tests/fixtures/issue_14_scaffold.my` |
153+
| dhat profiling example | `crates/my-lang/examples/dhat_checker_profile.rs` |
154+
| `dhat-heap` feature | `crates/my-lang/Cargo.toml` |
155+
| Windows + Linux CI | `.github/workflows/checker-scaling.yml` |
156+
157+
## 11. Appendix — Hypatia scan triage
158+
159+
While iterating on the #14 PRs, every PR (including docs-only ones) got an
160+
identical "🔍 Hypatia Security Scan — 44 issues" comment. Traced to
161+
`.github/workflows/hypatia-scan.yml`:
162+
163+
- It runs an **external** scanner (cloned/built from
164+
`github.com/hyperpolymath/hypatia`) over the **whole working tree**
165+
(`scan .`), with **no diff awareness**.
166+
- The "Comment on PR with findings" step fired on *any* PR whenever the
167+
repo-wide `findings_count > 0` and posted `findings.slice(0, 10)` — so the
168+
same standing backlog re-appeared on every unrelated PR.
169+
- It is non-blocking (`--exit-zero`, the `exit 1` is commented out) and the
170+
downstream Phase 2/3 (gitbot-fleet submit, robot-repo-automaton autofix)
171+
are unimplemented, so nothing ever cleared the backlog.
172+
173+
Remediation (companion PR):
174+
175+
1. **Diff-scoped + baseline-aware comment.** The step now lists only
176+
findings in files the PR changed and not in the baseline, and stays
177+
*silent* when there is nothing actionable. Full set still uploaded as the
178+
`hypatia-findings` artifact and written to the step summary.
179+
2. **`.hypatia-ignore`** exempts non-shipping trees (`playground/`,
180+
`dialects/`), a scanner false positive (a Nickel policy that itself bans
181+
`Dockerfile`), proptest scaffolding, and a safe-by-construction unwrap —
182+
each with inline rationale.
183+
3. **`.hypatia-baseline.json`** (workflow-owned format) freezes known
184+
pre-existing findings; marked `_partial` until regenerated from a real
185+
scan artifact.
186+
4. **`lib/common/concurrency.rs`** lock/poison unwraps fixed
187+
(`unwrap_or_else(|e| e.into_inner())`); remaining genuine items
188+
(notably a Coq `Admitted` proof hole) tracked in issue #34.

docs/wiki/internals/type-checker.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,23 @@ impl Checker {
607607
}
608608
```
609609

610+
## Allocation & Recursion Cost
611+
612+
`check_expr` / `is_assignable_from` are **linear in AST size** along both the
613+
nesting-depth and the templating-breadth axes — confirmed by measurement on
614+
Linux *and* on a `windows-latest` CI leg. There is no super-linear allocation
615+
hotspot in the checker; each `check_expr` level clones only a fixed-size
616+
builtin signature, so the type representation cannot grow with nesting.
617+
618+
Consequently the `MAX_EXPR_DEPTH` guard (`src/checker.rs`) is **not**
619+
load-bearing for checker memory safety: its purpose is bounding *stack
620+
recursion* (recursive `check_expr`, recursive AST `Drop`, recursive-descent
621+
parser), not preventing a heap blow-up.
622+
623+
Full methodology, measurements, dead ends, and implications:
624+
[Checker Allocation Investigation (#14)](./checker-allocation-investigation.md).
625+
Regression guard: `cargo test -p my-lang --test checker_alloc_scaling`.
626+
610627
## Testing
611628

612629
```rust

0 commit comments

Comments
 (0)