Add nodeId scoping to getBuildLog/searchBuildLog plus getFlowNodes for discovery - #221
Open
neeldugar wants to merge 2 commits into
Open
Add nodeId scoping to getBuildLog/searchBuildLog plus getFlowNodes for discovery#221neeldugar wants to merge 2 commits into
neeldugar wants to merge 2 commits into
Conversation
A negative limit.max inverted forward reads. SystemProperties.getInteger uses Integer.decode, which accepts "-5" rather than falling back to the default; the negative ceiling flipped limit's sign, so a request for the first 100 lines returned the single last line with no cursor to recover the rest. Clamped tail windows became unrecoverable. readTail's ring buffer keeps only the trailing `capacity` lines, but the window and line numbers came from the full total, so a window clamped by limit.max could fall entirely inside the evicted region: zero lines with hasMoreContent=true and nextCursor=null. Co-Authored-By: Claude <noreply@anthropic.com>
A multi-stage Pipeline's console log interleaves every stage's output, so getBuildLog and searchBuildLog now take an optional nodeId that scopes the read to one flow-graph node. Omitting it is unchanged behaviour. The read helpers take a LogSource (log bytes, whether more may arrive, cursor identity) instead of a Run, so one set of window-arithmetic helpers serves both whole-build and per-node reads rather than two forked copies. Cursors now carry job, build and node. The job was previously absent, so a cursor issued for one job was honoured against another and returned an offset into an unrelated log. Cursors from earlier versions are rejected. getFlowNodes lists the graph so callers can obtain node IDs, which no @exported property on WorkflowRun exposes. Paginated, since graphs reach thousands of nodes. workflow-api types stay in PipelineLogUtil and PipelineGraphExtension so BuildLogsExtension still loads without the Pipeline plugins. Lazy class resolution makes that insufficient alone, so a by-name probe converts the resulting NoClassDefFoundError into a tool error. Co-Authored-By: Claude <noreply@anthropic.com>
neeldugar
force-pushed
the
feature/node-scoped-build-logs
branch
from
August 10, 2026 02:55
d9f64b1 to
09b146f
Compare
| * @param live whether more output may still be appended | ||
| * @param scopeKey identity a cursor is bound to; must distinguish job, build, and node | ||
| */ | ||
| public record LogSource(AnnotatedLargeText<?> text, boolean live, String scopeKey) { |
Author
There was a problem hiding this comment.
not a password, just unique identifier of scope
There was a problem hiding this comment.
Pull request overview
Adds Pipeline-node discovery and node-scoped build-log reading/searching, while improving pagination bounds and cursor isolation.
Changes:
- Adds
getFlowNodeswith filtering and pagination. - Adds optional
nodeIdscoping and stronger cursor identity. - Adds extensive Pipeline, bounds, and optional-dependency tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
BuildLogsExtension.java |
Implements scoped log access and pagination fixes. |
PipelineGraphExtension.java |
Adds flow-node discovery. |
PipelineLogUtil.java |
Resolves per-node Pipeline logs. |
LogSource.java |
Abstracts log source and cursor scope. |
README.md |
Documents the new tools and workflow. |
pom.xml |
Declares workflow-api. |
EndPointTest.java |
Registers expected tool metadata. |
PipelineNodeLogTest.java |
Tests node-scoped log behavior. |
PipelineGraphExtensionTest.java |
Tests graph discovery and filtering. |
BuildLogWindowBoundsTest.java |
Tests pagination bounds. |
BuildLogsWithoutPipelineTest.java |
Tests optional Pipeline dependencies. |
Suppressed comments (1)
src/main/java/io/jenkins/plugins/mcp/server/extensions/BuildLogsExtension.java:475
- Clamping
resolvedSkipto the retained tail silently returns a different window. For example, with 30 lines,maxLimit=10,skip=-24, andlimit=1, the requested seventh line is replaced by line 21. The implementation should preserve the requested offset (for example, count then read the bounded result in a second pass) or reject an unsupported lookback rather than return incorrect log content.
long earliestRetained = Math.max(0, total - capacity);
if (resolvedSkip < earliestRetained) {
log.warn(
"End-relative window started at line {} but only the last {} of {} lines were retained;"
+ " returning from line {} instead",
resolvedSkip + 1,
capacity,
total,
earliestRetained + 1);
resolvedSkip = earliestRetained;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+118
to
+121
| if (src == null) { | ||
| // Block boundary: an empty window, not an error. | ||
| return new BuildLogResponse(List.of(), false, -1, -1, 0, null); | ||
| } |
Comment on lines
+255
to
+258
| for (BlockStartNode enclosing : node.getEnclosingBlocks()) { | ||
| LabelAction label = enclosing.getPersistentAction(LabelAction.class); | ||
| if (label != null) { | ||
| return label.getDisplayName(); |
Comment on lines
+40
to
+43
| public record LogSource(AnnotatedLargeText<?> text, boolean live, String scopeKey) { | ||
|
|
||
| public static LogSource ofRun(Run<?, ?> run) { | ||
| return new LogSource(run.getLogText(), run.isLogUpdated(), scopeKey(run, null)); |
Comment on lines
+453
to
+455
| - **Only nodes with `hasLog: true` return output.** Block boundary nodes (a `stage`'s start/end, `node` | ||
| wrappers) delegate their output to the leaf steps nested inside them, so they return an empty result — | ||
| not an error. Use `onlyWithLogs: true` to list just the useful ones. |
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
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.
A multi-stage Pipeline's console log interleaves every stage's output, so answering "what failed in the Test stage?" means reading past everything else.
getBuildLogandsearchBuildLognow take an optionalnodeIdthat scopes the read to a single node of the Pipeline flow graph.getFlowNodeslists the graph so callers can discover valid IDs.{ "jobFullName": "my-pipeline", "stageName": "Test", "onlyWithLogs": true }{ "nodes": [{ "id": "10", "displayName": "Print Message", "hasLog": true, ... }], "skip": 0, "matched": 2, "totalInGraph": 13, "hasMore": false }{ "jobFullName": "my-pipeline", "nodeId": "10", "limit": 100 }Omitting
nodeIdis byte-for-byte the previous behaviour.Notes
Two commits. The first fixes two pre-existing
getBuildLogwindow-bounds defectsand stands alone — happy to split it into a separate PR if preferred.
limit.maxsystem property inverted forward reads.SystemProperties.getIntegeruses
Integer.decode, which accepts"-5"rather than falling back to the default.readTail's ring buffer retains only thetrailing
capacitylines, but the window and line numbers came from the full total, so aclamped window could fall entirely inside the evicted region: zero lines with
hasMoreContent=trueandnextCursor=null.Refactor. The paginated read helpers now take a
LogSource(log bytes, livenesspredicate, cursor identity) instead of a
Run, so one set of helpers serves both whole-buildand per-node reads. An earlier draft forked them and the copies immediately drifted.
Cursor scope. Cursors now carry job, build and node. The job was previously absent, so a
cursor issued for one job was honoured against another. Cursors from earlier versions are
rejected with
Invalid cursor.Optional-dependency safety.
workflow-apitypes are confined toPipelineLogUtilandPipelineGraphExtension. Lazy class resolution means that is not sufficient alone:PipelineLogUtilloads fine withoutworkflow-apiand fails only on theinstanceofopening
resolveNodeLogSource, raisingNoClassDefFoundError— anErrorthatMcpToolWrapper'scatch (Exception)misses. A by-nameClass.forNameprobe convertsthat into a tool error naming the missing plugins.
workflow-apiis also now declaredexplicitly rather than relied on transitively.
Conflict note. This touches the exhaustive tool-name whitelist in
EndPointTest.testListTools, as does #215. Whichever merges second needs a one-line fixthere.
Testing done
mvn verifygreen — 273 tests, 0 failures. New coverage:PipelineNodeLogTest(13 × 2 transports) — basic read, cursor pagination, tail read,node-not-found, non-Pipeline build, block-boundary empty result, in-progress non-blocking,
cursor rejected across nodes and across jobs, malformed nodeId rejected, omitting nodeId
unchanged, searchBuildLog scoping, searchBuildLog rejects unknown node.
PipelineGraphExtensionTest(9 × 2) — listing, execution order, round-trip (IDs fromgetFlowNodesaccepted bygetBuildLog), stage attribution, filters, pagination, scalebounds.
BuildLogWindowBoundsTest(3 × 2) — uses@SetSystemPropertyto lowerlimit.maxtoa value reachable with a small fixture.
BuildLogsWithoutPipelineTest(8, noJenkinsRule, ~0.07s) — hidesorg.jenkinsci.plugins.workflow.**behind a classloader; provesBuildLogsExtensionloads and instantiates, tool signatures resolve, and the by-name Pipeline guard fires before
any
Errorcan escape.Verified by mutation: reverting
isActive()→isRunning(), dropping the cursor scope-keycheck, removing the pagination caps, or deleting the Pipeline guard call each fails the suite.
Submitter checklist