fix(tracer): fix BSD-incompatible sed injection at remaining jvm/native call sites#2047
Conversation
…ve call sites jvm-tracer.sh (java/kotlin/scala/groovy) and native-tracer.sh (csharp/swift/dart/zig) used the GNU-only single-line `a\text`/`i\text` sed form at 8 more call sites beyond the one #1759 already fixed for rust — BSD sed (macOS) requires the text on the line after the backslash and otherwise errors or, when the error is swallowed by `2>/dev/null || true`, silently skips the injection entirely. Mirrors #1759's fix, but since the broken shape repeats identically across all 8 sites, extracts three shared helpers into tracer-common.sh (sedi_insert_before, sedi_insert_before_end, sedi_append_unless) that build the portable multi-line form internally, so future injection sites can't reintroduce the GNU-only shortcut — the approach the issue itself suggested. Fixing the syntax alone surfaced a second, previously-masked bug at four of the same sites (jvm-tracer.sh java/scala/groovy dump, native-tracer.sh csharp dump): `\s` is a GNU-only regex escape that BSD sed's regex engine does not treat as whitespace, so `/^\s*\}/` never matched an indented closing brace on macOS — the dump() call would still never have been injected even after the a\/i\ fix. Replaced with the portable `[[:space:]]` POSIX bracket expression already used correctly elsewhere in the same patterns.
Greptile SummaryReplaces eight BSD-incompatible GNU-only
Confidence Score: 4/5Safe to merge; the shell changes mechanically replace known-broken GNU-only sed forms with the portable multi-line equivalent and the fix is verified by new tests that run on the host's own sed. The helper implementations are correct and the per-language call sites map cleanly onto the original logic. The only notable gap is a misleading docstring on tracer-common.sh (docstring on Important Files Changed
Reviews (1): Last reviewed commit: "fix(tracer): fix BSD-incompatible sed in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Insert TEXT on the line immediately before the first line matching PATTERN. | ||
| sedi_insert_before() { |
There was a problem hiding this comment.
${pattern} i\ applies to every line that matches the pattern, not only the first. The current sole call-site happens to target a unique string (/CallTracer.Dump/), so the mismatch is harmless today — but a future caller that expects first-only semantics would silently get multiple insertions. The comment should say "every" (or restrict the implementation to a 0,/pat/-scoped address for truly first-only behaviour).
| # Insert TEXT on the line immediately before the first line matching PATTERN. | |
| sedi_insert_before() { | |
| # Insert TEXT on the line immediately before every line matching PATTERN. | |
| sedi_insert_before() { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const guardedFiles = ['jvm-tracer.sh', 'native-tracer.sh', 'go-tracer.sh']; | ||
|
|
||
| for (const name of guardedFiles) { | ||
| const code = fs | ||
| .readFileSync(path.join(TRACER_DIR, name), 'utf8') | ||
| .split('\n') | ||
| // Drop comment lines so prose that *describes* the banned form (as | ||
| // this very fix's changelog comments do) isn't mistaken for it. | ||
| .filter((line) => !line.trim().startsWith('#')) | ||
| .join('\n'); | ||
| // \S excludes the newline that must immediately follow a portable | ||
| // "i\" / "a\" — so this only matches the banned single-line form | ||
| // where real text follows the backslash on the same line. | ||
| const offender = code.match(/[ai]\\\S/); | ||
| expect( | ||
| offender, | ||
| `${name} contains a GNU-only single-line a\\/i\\ form: ${offender}`, | ||
| ).toBeNull(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
tracer-common.sh itself
The guard intentionally skips tracer-common.sh to avoid false-positives from the i\\ in the helpers' own portable implementation (the JS regex /[ai]\\\S/ would match the second backslash in i\\<newline>text). That's the right trade-off, but it means a new injection helper mistakenly written with the GNU-only form directly inside tracer-common.sh would go undetected. A comment in the test recording why the file is excluded would make this intentional gap visible to the next contributor.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codegraph Impact Analysis7 functions changed → 6 callers affected across 2 files
|
1 participant
Summary
jvm-tracer.sh(java/kotlin/scala/groovy) andnative-tracer.sh(csharp/swift/dart/zig) used the GNU-only single-linea\text/i\textsed form at 8 more call sites beyond the one rust site Rust dynamic tracer fixture has no main.rs — trace_rust always fails before producing edges #1759 already fixed — BSD sed (macOS) rejects it outright, or when the failure is swallowed by2>/dev/null || true, silently skips the injection entirely.tracer-common.sh—sedi_insert_before,sedi_insert_before_end,sedi_append_unless— that build the portable multi-line form internally, so future injection sites can't reintroduce the GNU-only shortcut. This is the approach the issue itself suggested ("consider adding a small helper... so future injection sites can't reintroduce the GNU-only shortcut").\sis a GNU-only regex escape that BSD sed's regex engine (BRE or ERE) does not treat as whitespace, so/^\s*\}/never matched an indented closing brace on macOS — the dump() call would still never have been injected even after thea\/i\fix, just without the syntax error. Replaced with the portable[[:space:]]POSIX bracket expression already used correctly elsewhere in the same file.Closes #1913
Test plan
tests/benchmarks/resolution/tracer/tracer-common.test.ts(5 tests) exercises all three helpers directly against synthetic files mirroring each of the 8 original sites' shapes, plus a regression guard that grepsjvm-tracer.sh/native-tracer.sh/go-tracer.shfor any remaining single-linea\/i\formtests/benchmarks/resolution/tracer/tracer-validation.test.ts— 42/42 passedfunc mainbranch (previously dormant/broken) and the top-level fallback branchjavacitself isn't available in this sandbox — no real JDK installed) and confirmed byte-correctCallTracer.traceCall()/CallTracer.dump()placementnpm run lint— clean;shellcheckon all touched.shfiles — no new findingsbash -nclean ontracer-common.sh,jvm-tracer.sh,native-tracer.shFiled two out-of-scope findings surfaced while getting these tracers to actually run on BSD sed for the first time:
if/while/for/catchcontrol-flow blocks (harmless today due to tracer-side dedup, but fragile)go-tracer.shhas the same\s-on-BSD-sed bug in itsmain()detection regex (separate file, not named in Tracer sed injection uses BSD-incompatible single-line a\/i\ form at 8 more sites (jvm-tracer.sh java/kotlin/scala/groovy; native-tracer.sh csharp/swift/dart/zig) #1913's scope)