You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Code-health suggestion (proactive — staged for your review)
Category: test-gap / green signal that measures nothing
Where:
clangwalk/src/nativeMain/kotlin/AstWalk.kt:62-67 (the three early returns)
.github/workflows/root-build.yml:42 (clangwalk/** in the trigger)
docs/clang-runbook.md:103 (row 4 of the verification table)
Two halves of the same gap
docs/clang-runbook.md:98-103 is a four-row "what proves the clang path works" table. #222 pulled
rows 1–3 into root-build.yml. Row 4 — :clangwalk:runReleaseExecutableKlinker — is still the
residue: no workflow compiles :clangwalk, let alone runs it. And the walk itself, when run, has
three paths that exit 0 having asserted nothing.
1. No CI job compiles :clangwalk, but its path IS in the trigger
root-build.yml fires on clangwalk/** (:42, with the honest comment "in settings.gradle.kts, so
ktlintCheck covers it"). Both of its jobs were dry-run here, each with a positive control on the
same invocation:
$ ./gradlew --no-daemon :krapper:nativeTest :krapper_model:nativeTest \ :feature-tests:nativeTest ktlintCheck --dry-run # root-build job 1tasks in graph 113:clangwalk: tasks that are not ktlint tasks 0:krapper: compile tasks (control) 2
$ ./gradlew --no-daemon :featuregen:nativeTest :cppfixture:nativeTest \ :krapper:noncopyableDeterminismCheck --dry-run # root-build job 2tasks in graph 40:clangwalk: tasks of any kind 0:featuregen: compile tasks (control) 2
$ ./gradlew --no-daemon ktlintCheck --dry-runtasks in graph 80matching compile|link|kplusplusSync|cinterop 0matching ktlint (control) 80
So a PR whose whole content is clangwalk/triggers root-build, goes green, and nothing compiled
the change.:clangwalk is the only module in settings.gradle.kts in that state.
It is not expensive and it is not broken — both measured on this box just now:
$ ./gradlew --no-daemon :clangwalk:linkDebugExecutableKlinker
> Task :clangwalk:kplusplusSync # executed, not UP-TO-DATE
> Task :clangwalk:cinteropKplusplusNative
> Task :clangwalk:compileKotlinNative
> Task :clangwalk:linkDebugExecutableKlinkerBUILD SUCCESSFUL in 1m 10s
$ LD_LIBRARY_PATH=$(llvm-config --libdir) ./clangwalk/build/bin/klinker/clangwalkDebug/clangwalk [PASS] walked exactly 10 top-level decls — got 10 ... 15 checks, all PASS ...clangwalk: all self-checks passed — walked 10 top-level decls via real libclang-cppEXIT=0
15 working assertions over the committed stage-0 Clang-AST seed (kpp.frontend.clangwalk=seed), for
~70 s, running nowhere. #73 — "clangwalk release generation broken on LLVM-22.1.6" — is what this
catches, and it was found by hand.
2. The walk reports success on the failure it exists to detect
AstWalk.kt:32-34 states the intent: "let main() exit non-zero if ANY failed — so :clangwalk:runReleaseExecutableKlinker is self-verifying instead of a println demo." Three lines
below, it is not:
val unit = buildASTFromCode(source, "input.cc")
?:return@memScoped println("clangwalk: buildASTFromCode returned null") // :62-63val context = unit.getASTContext()
?:return@memScoped println("clangwalk: no ASTContext") // :64-65val tu = context.getTranslationUnitDecl()
?:return@memScoped println("clangwalk: no TranslationUnitDecl") // :66-67
All three println and return normally. main returns Unit, the process exits 0, the exitProcess(1) at :190-193 is never reached, and runReleaseExecutableKlinker reports BUILD SUCCESSFUL. Those three are exactly the shape of a broken binding set: the wrapper still
links, the API still exists, and it returns null at runtime — the runtime half of #73.
Compounding it, failures == 0 is also satisfied by zero checks having run. There is no
"at least N checks executed" guard, so the pass condition cannot tell a full run from an aborted one.
Compare noncopyableDeterminismCheck (krapper/.../CppParser.kt:203-209), which does carry that
guard — "Sanity: the fixture really did parse (an empty TU would pass both checks vacuously)" — so
the correct shape already exists in this repo, one module over.
Proposed:
AstWalk.kt: make the three early returns check(...)-and-fail (or exitProcess(1)), and add a check("N self-checks ran", checksRun >= 15) so a truncated run cannot report success. Watch it fail before accepting it: point buildASTFromCode at unparseable source and confirm
the exit status is non-zero — today it is 0.
root-build.yml: add :clangwalk:runDebugExecutableKlinker (debug, not release — linkerOpts("--gc-sections") already makes debug link, per clangwalk/build.gradle.kts:43) to a
job. It needs no in-tree :krapper release link, so it fits the cheaper unit-tests job. A
non-zero exit from the walk fails the task directly, so it needs no JUnit-XML gate.
Why: it converts a documented, working, cheap, end-to-end verification into a standing one, and
it removes a "PASSED" that a real failure produces. Item 1 is worth doing on its own even if item 2
is declined — a self-check that reports success when the thing it checks is absent is worse than no
self-check.
Filed by the nightly code-health pass. To act on it: relabel agent-workable (it'll flow into triage→work). To decline: close it — it won't be suggested again.
Code-health suggestion (proactive — staged for your review)
Category: test-gap / green signal that measures nothing
Where:
clangwalk/src/nativeMain/kotlin/AstWalk.kt:62-67(the three early returns).github/workflows/root-build.yml:42(clangwalk/**in the trigger)docs/clang-runbook.md:103(row 4 of the verification table)Two halves of the same gap
docs/clang-runbook.md:98-103is a four-row "what proves the clang path works" table. #222 pulledrows 1–3 into
root-build.yml. Row 4 —:clangwalk:runReleaseExecutableKlinker— is still theresidue: no workflow compiles
:clangwalk, let alone runs it. And the walk itself, when run, hasthree paths that exit 0 having asserted nothing.
1. No CI job compiles
:clangwalk, but its path IS in the triggerroot-build.ymlfires onclangwalk/**(:42, with the honest comment "in settings.gradle.kts, soktlintCheck covers it"). Both of its jobs were dry-run here, each with a positive control on the
same invocation:
So a PR whose whole content is
clangwalk/triggers root-build, goes green, and nothing compiledthe change.
:clangwalkis the only module insettings.gradle.ktsin that state.It is not expensive and it is not broken — both measured on this box just now:
15 working assertions over the committed stage-0 Clang-AST seed (
kpp.frontend.clangwalk=seed), for~70 s, running nowhere. #73 — "clangwalk release generation broken on LLVM-22.1.6" — is what this
catches, and it was found by hand.
2. The walk reports success on the failure it exists to detect
AstWalk.kt:32-34states the intent: "let main() exit non-zero if ANY failed — so:clangwalk:runReleaseExecutableKlinkeris self-verifying instead of a println demo." Three linesbelow, it is not:
All three
printlnand return normally.mainreturns Unit, the process exits 0, theexitProcess(1)at:190-193is never reached, andrunReleaseExecutableKlinkerreportsBUILD SUCCESSFUL. Those three are exactly the shape of a broken binding set: the wrapper stilllinks, the API still exists, and it returns null at runtime — the runtime half of #73.
Compounding it,
failures == 0is also satisfied by zero checks having run. There is no"at least N checks executed" guard, so the pass condition cannot tell a full run from an aborted one.
Compare
noncopyableDeterminismCheck(krapper/.../CppParser.kt:203-209), which does carry thatguard — "Sanity: the fixture really did parse (an empty TU would pass both checks vacuously)" — so
the correct shape already exists in this repo, one module over.
Proposed:
AstWalk.kt: make the three early returnscheck(...)-and-fail (orexitProcess(1)), and add acheck("N self-checks ran", checksRun >= 15)so a truncated run cannot report success.Watch it fail before accepting it: point
buildASTFromCodeat unparseable source and confirmthe exit status is non-zero — today it is 0.
root-build.yml: add:clangwalk:runDebugExecutableKlinker(debug, not release —linkerOpts("--gc-sections")already makes debug link, perclangwalk/build.gradle.kts:43) to ajob. It needs no in-tree
:krapperrelease link, so it fits the cheaperunit-testsjob. Anon-zero exit from the walk fails the task directly, so it needs no JUnit-XML gate.
docs/clang-runbook.md:102says of row 3 "Wired into:krapper:check(No CI workflow runs the root Gradle build: 566 tests, all ktlint, and the #101 determinism lock execute on developer machines only #222), so it no longerneeds to be typed by hand." Row 4 should be able to say the same.
Why: it converts a documented, working, cheap, end-to-end verification into a standing one, and
it removes a "PASSED" that a real failure produces. Item 1 is worth doing on its own even if item 2
is declined — a self-check that reports success when the thing it checks is absent is worse than no
self-check.
Effort: mechanical (1), mechanical (2), trivial (3).
Refs #222, Refs #73, Refs #8.
Filed by the nightly code-health pass. To act on it: relabel
agent-workable(it'll flow into triage→work). To decline: close it — it won't be suggested again.