fix: register a bound callId atomically, and enforce LogSafe's documented reach - #788
Open
devops-thiago wants to merge 4 commits into
Open
fix: register a bound callId atomically, and enforce LogSafe's documented reach#788devops-thiago wants to merge 4 commits into
devops-thiago wants to merge 4 commits into
Conversation
…ge is never dropped `bind` registered a call in two steps — look the session's set up with `computeIfAbsent`, then add to it — and the add ran outside the per-bin lock the lookup held. A sibling stream finishing in between unmapped the set the add then wrote to: `invalidate(sessionId, callId)` returns null once the set empties, which drops the mapping, and `invalidate(sessionId)` drops it outright. Neither window is exotic; the second needs no bulk invalidate at all, only another batch for the same session finishing first. The add then landed on an orphaned set, `isActiveCall` read a mapping that no longer held the callId, and a live stream's usage callback was discarded as stale. Those tokens never reached `ReviewTokenLedger`, so the per-review spend ceiling (#509) under-counted: a review that should have hit the ceiling and degraded keeps spending, which is the opposite of what the control exists to do. Reviews run their batches on virtual threads, so concurrent bind and invalidate for one session is the ordinary case here. The mirror window in `invalidate` was closed with `computeIfPresent` when it was first flagged; this is the half that was missed. A single `compute` puts the add back under the lock that decides whether the mapping survives. The test drives the interleaving rather than racing it. The session's set is swapped for one whose `add` is a rendezvous point — the only instant that matters, since it is reached inside the bin lock once registration is atomic and outside it before — and the binder then waits for whichever of the two states follows: the sibling invalidate completed (the old shape) or parked on the bin this thread holds (the fixed shape). Both are observed states rather than elapsed time, so the test cannot deadlock against the fix and does not depend on who wins a sleep.
… that bypass it
`LogSafe`'s class javadoc claims every value a log statement splices in that the
bot did not choose itself goes through it. Five rounds have now found that
untrue, and each one fixed a list of sites someone had noticed: the ASCII-only
collapse class, `LogSafe`'s own `\p{IsZs}` gap, three `ReviewPublisher` DEBUG
lines waved through because debug is off by default, four rate-limit headers in
`GitHubApiError`, and now three INFO lines carrying a model-supplied finding
title and path. The enumeration is what keeps failing: it is a list a human
maintains, so the next call site written is outside it by default.
So this states the invariant as a test rather than as a list. It derives the
untrusted accessor names from the AI response records themselves — every
String-typed component of every record reachable from a `*Response` class in
`review.ai`, read reflectively — and scans every log call in `src/main/java` for
one interpolated without `LogSafe`. Both halves extend themselves: a field added
to `ReviewResponse.Finding` is covered the moment it exists, and a log line
added anywhere in main is scanned the moment it is written. A textual scan
cannot resolve types, so it decides by accessor name and errs towards
reporting; the answer to a report is to wrap the value, which is never wrong for
a logged string. Numeric conversions are exempt because a `%d` can forge
neither a boundary nor an escape. The scanner is itself pinned against
known-bad and known-good sources, because a structural test that silently
matches nothing reads exactly like a clean code base.
Running it found nine sites, not the three that were enumerated. The three INFO
lines are there — and the conversation-clear line carries two model-supplied
values, since its `locator` is composed from the finding's own file, so wrapping
only the title would have left half of it open. The other six are a WARN naming
a repo-supplied glob and six DEBUG lines naming a model-supplied path; DEBUG was
excluded by hand in an earlier round on the grounds that it is off by default,
which is exactly the kind of judgement that put this defect class on its fifth
appearance.
The three INFO lines each get a test that drives the real production path and
reads the LogRecord a handler is handed, alongside the six already there.
`LogSafe`'s javadoc now says which half is machine-checked: the model-supplied
one, because response records name their own fields. A GitHub error body has no
such record to read, so that half stays a convention its call sites keep.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesMakes ReviewSessionContext.bind register a new stream invocation atomically inside one ACTIVE_CALLS.compute, so a sibling invalidate emptying the session can no longer orphan the set the add writes to (fixes #763), and wraps all remaining raw model-supplied values in main-source INFO/DEBUG/WARN log calls with LogSafe.oneLine while adding a source-scanning invariant test, LogSafe javadoc clarification, and behavioural log-record tests (fixes #764). Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramsequenceDiagram
participant B as Binder thread
participant M as ACTIVE_CALLS map
participant I as Sibling invalidator
B->>M: bind(): ACTIVE_CALLS.compute(sessionId, lambda)
note over M: lambda runs under the per-bin lock
M->>M: active.add(callId) inside the lock
B-->>I: atAdd.countDown() releases the sibling
I->>M: invalidate(sessionId, otherCallId): computeIfPresent
note over M,I: sibling parks on the same bin lock (BLOCKED)
M-->>B: add completes; set is still mapped
I->>M: remove(otherCallId); set non-empty, mapping kept
M-->>B: isActiveCall(bound) is true
Changes Overview
Changed Files
Risk Assessment
Key Findings
|
| Check | Type | Status | Detail |
|---|---|---|---|
| dependency-review | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
bug
… it mentions LogSafe
The invariant skipped an argument as soon as it contained "LogSafe." anywhere,
which reads one wrapped value as proof the rest is safe. An argument mixing the
two passed with the raw half interpolated at its own conversion:
Log.infof("path %s", LogSafe.oneLine(finding.title()) + finding.file())
That is the shape a partial fix produces — someone wraps the value the finding
names and leaves the one beside it — so it is the shape the guard most needs to
catch. The local-declaration half had the same skip and the same hole.
Both now remove the sanitized spans and scan the remainder, so the check cannot
be satisfied by mentioning the sanitizer. Spans are matched by balancing
parentheses rather than to the first close paren, so a nested call inside the
argument does not end the span early.
Three cases pinned in the scanner's self-check: the mixed argument, a local
composed from a wrapped value and a raw one, and the nested call that must still
count as sanitized.
Reported by the reviewer on this PR.
Contributor
🤖 ThrillhouseBot — changes since the last review
|
…regexes
The javadoc the scanner shipped with claimed it "reads every log call in
src/main/java and fails on any that interpolates one of them without LogSafe",
and LogSafe's own javadoc repeated it. What the scan sees is two shapes: an
untrusted accessor called inside the log call's own argument text, and a
same-file local whose initializer calls one. A value that crosses a method
boundary is a bare identifier no declaration in the file marks tainted, so
void logPath(String file) { Log.warnf("rule file %s", file); }
called with finding.file() reports nothing. Confirmed against the scanner
before touching it: that case asserts 1 and gets 0.
Which is #764's own defect — a documented reach wider than the actual one —
re-installed in the checker's documentation, so the sentence had to change
whatever else did.
Extending the scan instead was tried and measured rather than waved off.
Without type resolution the only caller-to-callee join available is name plus
argument position; over main that taints 203 parameters and produces 31 further
reports, and the sampled ones are collisions: `req` in ReviewDispatcher, a
record whose owner() is what actually reaches the line, and `line` in
DocGenerationService, an int at a %d beside an already-wrapped path. Neither is
a value LogSafe.oneLine accepts, so the report's own prescribed fix would not
compile — and a check that asks for impossible fixes is one a reader learns to
skip, which costs more than the shapes it would have caught. So both javadocs
now state the two shapes and name the escape hatch (wrap the value where the
accessor is read), and the miss is pinned by a test, so the reach and the
sentence describing it can only move together.
The two scanning regexes were quadratic, which matters because this file reads
every source in main. CONVERSION's flag class and width overlap on 0, the
zero-pad flag being also a digit, so a run of zeros splits every way between
them; LOCAL_DECLARATION's \s* after the = overlaps [^;] on whitespace the same
way. Possessive quantifiers remove both splits without removing a match: 7030ms
to 2ms and 2494ms to under 1ms on a 24 000-character run, and a differential
over 400 000 random inputs plus 41 curated ones found nothing the old and new
forms answer differently. The flag-and-width case is now pinned, since dropping
the flag class slides the numeric exemption onto the wrong argument. The
per-report \s+ is compiled once rather than at each reported call site.
Reported by the reviewer on this PR.
|
Contributor
🤖 ThrillhouseBot — changes since the last review
|
| private static final Pattern LOCAL_DECLARATION = | ||
| Pattern.compile( | ||
| "\\b(?:final\\s+)?(?:String|var)\\s++(\\w+)\\s*+=\\s*+([^;]++);", Pattern.DOTALL); | ||
|
|
Contributor
There was a problem hiding this comment.
🟡 MEDIUM — Scanner silently misses a same-file local whose initializer contains ';' inside a string literal
LOCAL_DECLARATION captures a local's initializer with ([^;]++);, and [^;]++ stops at any semicolon, including one inside a string literal. For String msg = "prefix; " + finding.title(); the captured initializer is just "prefix, which withoutLiterals then strips as an unterminated literal, so the accessor call is never seen; msg is never marked tainted and a later Log.infof("... %s", msg) interpolates a model-supplied value with no report. That contradicts the reach both javadocs claim: the test's class javadoc says the scan "fails on two shapes: ... a same-file local whose initializer calls one" (lines 45-47), and LogSafe.java's javadoc says it "fails on any log statement in src/main/java that calls one of them in the call's own arguments, or that logs a same-file local composed from such a call" — the local's initializer does call finding.title(), yet the scan stays silent. This is the same documented-reach-exceeds-actual-reach class #764 exists to end, now inside the guard itself, and because it is a false negative no pin or current source exposes it. No file in the current diff exhibits the shape, so the risk materializes only for the next site written. Fix direction: make the initializer capture literal-aware so a ';' inside a string/char literal (or text block) does not terminate it, or add a pin test for this shape so the miss is at least not silent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
Two unrelated defects from the merged-PR tech-debt audit, one commit each.
#763 — a bound callId was registered in two steps
ReviewSessionContext.bindlooked the session's in-flight set up withcomputeIfAbsentand then added to it, so the add ran outside the per-bin lock thelookup had held:
A sibling stream finishing in between unmapped the set the add then wrote to.
invalidate(sessionId, callId)returns null once the set empties, which drops themapping, and
invalidate(sessionId)drops it outright — the first needs no bulkinvalidate at all, only another batch for the same session finishing first. The add
landed on an orphan,
isActiveCallread a mapping that no longer held the callId, anda live stream's usage callback was discarded as stale. Those tokens never reached
ReviewTokenLedger, so the per-review spend ceiling (#509) under-counted: a reviewthat should have hit the ceiling and degraded keeps spending. Reviews run their
batches on virtual threads, so concurrent bind and invalidate for one session is the
ordinary case here.
The mirror window in
invalidatewas closed withcomputeIfPresentwhen it was firstflagged on #374; this is the half that was missed. A single
computeputs the add backunder the lock that decides whether the mapping survives.
The interleaving is driven, not raced. The session's set is swapped for one whose
addis a rendezvous point — the only instant that matters, since it is reached insidethe bin lock once registration is atomic and outside it before. The sibling
invalidateis released exactly there, and the binder then waits for whichever of the two states
follows: the invalidate completed (it was never excluded — the old shape) or it
parked on the bin this thread holds (it was excluded — the fixed shape). Both are
observed states rather than elapsed time, so the test cannot deadlock against the fix,
does not depend on who wins a sleep, and fails in 7 ms against the old code rather than
after a timeout.
#764 — the fifth round of "a sanitizer whose documented reach exceeds its actual reach"
LogSafe's class javadoc claims every value a log statement splices in that the bot didnot choose itself goes through it. Five rounds have now found that untrue, and every one
of them fixed a list of sites someone had noticed. The enumeration is what keeps failing:
it is a list a human maintains, so the next call site written is outside it by default.
So the second half of this PR states the invariant as a test rather than as a list.
LogSafeInvariantTestderives the untrusted accessor names from the AI response recordsthemselves — every
String-typed component of every record reachable from a*Responseclass in
review.ai, read reflectively — and scans every log call insrc/main/javaforone interpolated without
LogSafe. Both halves extend themselves: a field added toReviewResponse.Findingis covered the moment it exists, and a log line added anywhere inmain is scanned the moment it is written. Nothing is kept in sync by hand.
Design notes, since a source-scanning test is unusual:
accessor name; an unrelated record's
file()is reported too. The answer to a report isto wrap the value, which is never wrong for a logged string.
%dsplices a number, which can forge neither arecord boundary nor an escape — and a finding's
line()is the common case.FollowUpAnalyzer'slocatorisfile() + ":" + line()built a few statements above the log call, so a check reading only the call's own
accessors would see a bare identifier and pass it.
like a clean code base, so the scanner is pinned against known-bad and known-good
sources, and the run asserts the derived accessor set still contains its known members
before it trusts a clean result.
Running it found nine sites, not the three the issue enumerated. The three INFO lines
are there, and the conversation-clear line carries two model-supplied values rather than
one, so wrapping only its title would have left the path open. The other six: a WARN
naming a repo-supplied glob (
PathScopedInstructions), and six DEBUG lines naming amodel-supplied path (
DocGenerationService,PrImprovementService). DEBUG was excludedby hand in an earlier round on the grounds that it is off by default — which is the kind
of judgement that put this defect class on its fifth appearance, so these are wrapped too.
Review round: the javadoc claimed a reach the scan does not have
The reviewer was right, and it was verified against the scanner before anything moved. The
javadoc said the scan "reads every log call in
src/main/javaand fails on any thatinterpolates one of them without
LogSafe". It sees two shapes: an untrusted accessorcalled inside the log call's own argument text, and a same-file local whose initializer
calls one. A value crossing a method boundary is a bare identifier no declaration in the
file marks tainted:
Asserted against the unmodified scanner:
Narrowed the javadocs rather than extending the scan, and the extension was measured
rather than dismissed. A textual scan has no call graph; the only caller-to-callee join
available without type resolution is name plus argument position. Prototyped over main,
that taints 203 parameters and produces 31 further reports, and the sampled ones are
name collisions rather than findings:
ReviewDispatcher:83reqreq.owner()/req.repo()—reqis a recordDocGenerationService:431lineintat a%d, beside an already-wrappeddoc.file()PrLabeler:167requestrequest.owner()/request.repo()LogSafe.oneLinetakes neither a record nor anint, so the report's own prescribed fixwould not compile. A check that asks for impossible fixes is one a reader learns to skip,
which costs more than the shapes it would have caught — and since #764's defect is a
documented reach exceeding an actual one, an accurate sentence is what cures it; a wider
scan would only move the boundary, not the mismatch.
So both javadocs now state the two shapes and name the escape hatch (wrap the value where
the accessor is read), and the miss is pinned by
theScannerDoesNotSeeAValueThatCrossedAMethodBoundary, so if the scan ever grows adataflow step the test fails and the failure lands on the two sentences that have to widen
with it. The reach and its description can only move together.
SonarCloud — three issues, all fixed, none suppressed
Both
java:S8786reports are true. This file reads every source in main, so the cost is real.CONVERSION(line 77): the flag class and the width overlap on0— the zero-pad flagis also a digit — so a run of zeros splits every way between them.
LOCAL_DECLARATION(line 87):\s*after the=overlaps[^;]on whitespace the same way.Possessive quantifiers remove both splits. No match is lost — a width may not begin with
0, since a leading one is the flag, so the greedy split was the only correct one anyway.Measured on a 24 000-character run, and differenced old form against new over 400 000
random inputs plus 41 curated format/declaration cases:
java:S9142(line 262) is mechanical: the per-report\s+is now aPatterncompiledonce instead of at each reported call site.
No assertion was weakened. One was added —
%,08d|%spins that a flagged, zero-padded,width-bearing conversion still binds to its own argument, since the flag/width parse was
previously unpinned. Mutation-checked by dropping the flag class from
CONVERSION:LogSafe's javadoc now says which half is machine-checked: the model-supplied one,because response records name their own fields. A GitHub error body has no such record to
read, so that half stays a convention its call sites keep (
GitHubApiErrorcollapses thebody and each diagnostic it appends). That is the honest statement of its reach.
Related Issues
Closes #763
Closes #764
How Has This Been Tested?
Red/green for both, then the full suite and the coverage gate.
#763 —
bindRegistersAtomicallyAgainstASiblingInvalidateEmptyingTheSessionagainst theunfixed
bind:Green after the
compute, and green on five consecutive runs.#764 —
LogSafeInvariantTestagainst the unfixed sources reports 14 unsanitizedinterpolations across 9 log statements:
The three INFO sites also get a behavioural test each, driving the real production path and
reading the
LogRecorda handler is handed, alongside the six already inModelSuppliedTextInLogLinesTest. Against the unfixed sources:The third proves the composed locator, not just the title.
Gates, in order:
Coverage on the changed lines, intersecting
target/site/jacoco/jacoco.xmlwithgit diff -U0 origin/main --: every added or changed line insrc/main/javais covered,zero uncovered branches (the new
computelambda's null/non-null branch is covered bothways by the existing bind tests). Re-run after the review round — the
LogSafechange isjavadoc only, so it adds no executable line: 0 uncovered lines, 0 uncovered branches across
all seven changed main sources.
Checklist
Additional Notes
The structural guard is the part worth reviewing hardest, since it is the piece meant to
stop a sixth round. Two properties are deliberate and worth confirming you agree with:
it reports rather than resolves (an unrelated
file()gets wrapped, and wrapping ischeap), and its reach is stated rather than assumed — it covers what can be derived from
a record, which is the model-supplied half.
Two directions it does not cover, both now stated in the javadoc rather than left implied:
GitHub-derived values have no response record to enumerate, so extending the same scan to
them needs a marker (an annotation on the untrusted accessors, say) rather than reflection
over record components; and values crossing a method boundary need a call graph, which
reading source text does not have. The second is pinned by a test, so it is a known limit
rather than a claim that quietly stopped being true.