Skip to content

Commit 2a5eb46

Browse files
shenxianpengshenxianpeng
authored
fix: require Job/Build for new explanations and harden Test Configuration and auto-fix (#260)
* fix: look up Test Configuration credentials in the form's folder or job "Test Configuration" of the Anthropic, Qwen and Azure OpenAI providers resolved the credentials ID with no item, which falls back to the Jenkins root as SYSTEM. A folder or job form could therefore send a credential that only Jenkins itself may use to the URL typed in the form. The test now looks the credential up in the item the form belongs to, as a build of that item does. * fix: require Job/Build to ask for a new explanation The console page and the Stages page called the AI provider for any user with Job/Read, anonymous readers included, and forceNew=true let them replace the explanation a build has. A new explanation now needs Job/Build; Job/Read still shows the explanation a build already has, and the Explain Error button is offered only to users who may use it. A request while an explanation of the build is being generated is answered with a warning instead of a second provider call, and the maxLines request parameter is capped at 1000 lines. * fix: keep CI and pipeline definitions out of auto-fix unless named Auto-fix pushes its branch to the origin repository, and a CI system that builds branches runs it before anyone reviews the pull request. A change to a CI or pipeline definition, such as the Jenkinsfile, .github/workflows/*.yml or .gitlab-ci.yml, decides what that build runs and with which credentials, yet the default allow-list reached them through Jenkinsfile and *.yml. Such a file is now changed only when autoFixAllowedPaths spells out its path, with a pattern that does not start with a wildcard. Jenkinsfile leaves the default list, an empty list allows nothing instead of everything, globs are matched against the normalized path, and paths with backslashes are refused.
1 parent 48cc2a3 commit 2a5eb46

14 files changed

Lines changed: 571 additions & 84 deletions

File tree

‎README.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,10 @@ Once a build has an explanation, the button reads **Show explanation**, and the
6868
generated; the re-explain icon asks the provider again. If a request fails, the card stays open with what went wrong
6969
and a **Try again** button.
7070
71+
Asking for a new explanation needs the **Job/Build** permission, because it calls the AI provider. Users who can only
72+
read the job see the explanation a build already has, and no button when it has none. The same applies on the Stages
73+
page.
74+
7175
### On the Stages page
7276
7377
With [Pipeline Graph View](https://plugins.jenkins.io/pipeline-graph-view/) 777 or newer (Jenkins 2.534 or newer), the
@@ -137,7 +141,7 @@ the supported SCM services and the files it may change.
137141

138142
| Issue | What to check |
139143
|---|---|
140-
| No **Explain Error** button | The button appears on finished builds that did not succeed. Check that explanation is on, globally and for the job's folder, and that **Test Configuration** succeeds. On the Stages page it needs Pipeline Graph View 777 or newer |
144+
| No **Explain Error** button | The button appears on finished builds that did not succeed. Check that explanation is on, globally and for the job's folder, and that **Test Configuration** succeeds. On the Stages page it needs Pipeline Graph View 777 or newer. Asking for a new explanation needs the Job/Build permission |
141145
| Authentication or rate limit errors | The key, the provider's quota and plan, and the plugin's own [request quota](docs/usage-quota.md) |
142146
| **Test Configuration** fails | The connection diagnostics report under the error shows the proxy decision, DNS, TCP and HTTP probe; read it bottom-up to find the layer that fails, and include it when you report an issue |
143147

‎docs/auto-fix.md‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -256,10 +256,31 @@ When no pull request was created, **Copy diff** puts that file's diff on the cli
256256

257257
```
258258
pom.xml, build.gradle, build.gradle.kts, *.properties, *.yml, *.yaml,
259-
Jenkinsfile, Dockerfile, package.json, requirements.txt, go.mod
259+
Dockerfile, package.json, requirements.txt, go.mod
260260
```
261261

262-
Files outside this list are never modified, regardless of what the AI suggests.
262+
Files outside this list are never modified, regardless of what the AI suggests. A pattern without a `/`, such as
263+
`pom.xml`, matches that file name in any directory. An empty `autoFixAllowedPaths` allows no file at all.
264+
265+
### CI and pipeline definitions
266+
267+
The fix branch is pushed to your repository, and a CI system that builds branches runs it before anyone reviews
268+
the pull request. A CI or pipeline definition decides what that build runs and with which credentials, so auto-fix
269+
changes one only when a pattern spells out its path: the pattern matches the whole path and does not start with a
270+
wildcard. These count as definitions:
271+
272+
- `Jenkinsfile`, `Jenkinsfile.*` and `*.jenkinsfile`, in any directory
273+
- `.gitlab-ci.yml`, `bitbucket-pipelines.yml`, `azure-pipelines.yml`, `.travis.yml`, `.drone.yml`
274+
- files under `.github/workflows/`, `.github/actions/`, `.circleci/` and `.buildkite/`
275+
276+
`*.yml` therefore covers neither `.gitlab-ci.yml` nor `.github/workflows/release.yml`, and `**/Jenkinsfile` covers no
277+
Jenkinsfile. `Jenkinsfile`, `ci/Jenkinsfile` or `.github/workflows/*.yml` do. To let auto-fix change the Jenkinsfile
278+
at the root of the repository:
279+
280+
```groovy
281+
explainError(autoFix: true, autoFixCredentialsId: 'github-pat',
282+
autoFixAllowedPaths: 'pom.xml,Jenkinsfile')
283+
```
263284

264285
---
265286

@@ -325,7 +346,7 @@ Auto-fix works best for failures where the root cause is **specific and mechanic
325346
| Outdated/missing dependency version in `pom.xml` or `build.gradle` | Failing unit tests with logic errors |
326347
| Missing or wrong property in `*.properties` / `*.yml` | Complex refactoring required |
327348
| Wrong Java/Python/Node version in `Dockerfile` or CI config | Environment-specific issues (missing secrets, network) |
328-
| Syntax error in `Jenkinsfile` or `*.yaml` | Flaky tests |
349+
| Syntax error in `*.yaml`, or in a `Jenkinsfile` you [allowed](#ci-and-pipeline-definitions) | Flaky tests |
329350
| Missing `package.json` dependency | Multi-file architectural changes |
330351

331352
The AI sets a `confidence` level (`high`, `medium`, `low`). The plugin only proceeds when
@@ -342,8 +363,9 @@ guaranteed fix.
342363
rest is the build number and a time-based suffix
343364
- The pull request title and commit subject carry that same AI-written title as plain text, on one
344365
line and at most 72 characters
345-
- File paths are validated against: absolute-path check, path-traversal check (`../`), and the
346-
allow-list globs — in that order — before any branch is created
366+
- File paths are validated against: absolute-path check, backslash and path-traversal check (`../`), and the
367+
allow-list globs — in that order — before any branch is created. CI and pipeline definitions need a glob that
368+
spells out their path (see [CI and pipeline definitions](#ci-and-pipeline-definitions))
347369
- The diff is parsed and validated before it is applied; malformed diffs abort the workflow
348370
- The token has the minimum scope needed: write access to the single repository
349371

‎src/main/java/io/jenkins/plugins/explain_error/ConsoleExplainErrorAction.java‎

Lines changed: 59 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import edu.umd.cs.findbugs.annotations.CheckForNull;
44
import com.google.common.annotations.VisibleForTesting;
5+
import hudson.model.Item;
56
import hudson.model.Result;
67
import hudson.model.Run;
78
import jenkins.model.RunAction2;
@@ -16,6 +17,7 @@
1617
import org.kohsuke.stapler.StaplerRequest2;
1718
import org.kohsuke.stapler.StaplerResponse2;
1819
import org.kohsuke.stapler.interceptor.RequirePOST;
20+
import org.springframework.security.access.AccessDeniedException;
1921

2022
/**
2123
* Action to add "Explain Error" functionality to console output pages.
@@ -25,6 +27,15 @@ public class ConsoleExplainErrorAction implements RunAction2 {
2527

2628
private static final Logger LOGGER = Logger.getLogger(ConsoleExplainErrorAction.class.getName());
2729

30+
/** Log lines a new explanation reads when the request does not say. */
31+
static final int DEFAULT_MAX_LINES = 200;
32+
33+
/** The most log lines a request may ask for: every line is sent to the provider. */
34+
static final int MAX_LINES_LIMIT = 1000;
35+
36+
static final String ALREADY_PENDING = "An explanation of this build is already being generated."
37+
+ " It will be shown when it is ready.";
38+
2839
private transient Run<?, ?> run;
2940
private String urlString;
3041

@@ -65,7 +76,7 @@ public String getUrlName() {
6576
public void doExplainConsoleError(StaplerRequest2 req, StaplerResponse2 rsp) throws IOException {
6677
long startTimeNanos = System.nanoTime();
6778
try {
68-
run.checkPermission(hudson.model.Item.READ);
79+
run.checkPermission(Item.READ);
6980

7081
// Folder settings count: a folder can turn explanation off, or on with a provider of its own
7182
ErrorExplainer switchCheck = new ErrorExplainer();
@@ -94,37 +105,62 @@ public void doExplainConsoleError(StaplerRequest2 req, StaplerResponse2 rsp) thr
94105
return;
95106
}
96107

97-
// Optionally allow maxLines as a parameter, default to 200
98-
int maxLines = 200;
99-
String maxLinesParam = req.getParameter("maxLines");
100-
if (maxLinesParam != null) {
101-
try { maxLines = Integer.parseInt(maxLinesParam); } catch (NumberFormatException ignore) {}
108+
// A new explanation calls the provider, which costs money, and replaces the one the build has
109+
run.checkPermission(Item.BUILD);
110+
if (!PendingExplanations.tryAdd(run)) {
111+
BaseAIProvider provider = switchCheck.providerFor(run);
112+
writeJsonResponse(rsp, "warning", provider != null ? provider.getProviderName() : "Unknown",
113+
ALREADY_PENDING);
114+
return;
102115
}
103-
104-
// Fetch the last N lines of the log
105-
PipelineLogExtractor logExtractor = new PipelineLogExtractor(run, maxLines, Jenkins.getAuthentication2(),
106-
false, null);
107-
List<String> logLines = logExtractor.getFailedStepLog();
108-
this.urlString = logExtractor.getUrl();
109-
110-
String errorText = String.join("\n", logLines);
111-
112-
ErrorExplainer explainer = new ErrorExplainer();
113116
try {
114-
ErrorExplanationAction action = explainer.explainErrorText(errorText, urlString, run);
115-
writeJsonResponse(rsp, "success", action.getProviderName(), action.getExplanation(),
116-
action.getTitle());
117-
} catch (ExplanationException ee) {
118-
writeJsonResponse(rsp, ee.getLevel(), explainer.getProviderName(), ee.getMessage(), null,
119-
ProviderFailure.describe(ee));
117+
int maxLines = maxLines(req.getParameter("maxLines"));
118+
119+
// Fetch the last N lines of the log
120+
PipelineLogExtractor logExtractor = new PipelineLogExtractor(run, maxLines,
121+
Jenkins.getAuthentication2(), false, null);
122+
List<String> logLines = logExtractor.getFailedStepLog();
123+
this.urlString = logExtractor.getUrl();
124+
125+
String errorText = String.join("\n", logLines);
126+
127+
ErrorExplainer explainer = new ErrorExplainer();
128+
try {
129+
ErrorExplanationAction action = explainer.explainErrorText(errorText, urlString, run);
130+
writeJsonResponse(rsp, "success", action.getProviderName(), action.getExplanation(),
131+
action.getTitle());
132+
} catch (ExplanationException ee) {
133+
writeJsonResponse(rsp, ee.getLevel(), explainer.getProviderName(), ee.getMessage(), null,
134+
ProviderFailure.describe(ee));
135+
}
136+
} finally {
137+
PendingExplanations.remove(run);
120138
}
139+
} catch (AccessDeniedException e) {
140+
rsp.setStatus(403);
141+
writeJsonResponse(rsp, "error", "Unknown", "Error: " + e.getMessage());
121142
} catch (Exception e) {
122143
LOGGER.severe("=== EXPLAIN ERROR REQUEST FAILED ===");
123144
LOGGER.severe("Error explaining console error: " + e.getMessage());
124145
writeJsonResponse(rsp, "error", "Unknown", "Error: " + e.getMessage());
125146
}
126147
}
127148

149+
/**
150+
* The number of log lines asked for, within {@link #MAX_LINES_LIMIT}; the default when the request does
151+
* not say or says something that is not a number.
152+
*/
153+
static int maxLines(@CheckForNull String requested) {
154+
if (requested == null) {
155+
return DEFAULT_MAX_LINES;
156+
}
157+
try {
158+
return Math.min(Math.max(1, Integer.parseInt(requested.trim())), MAX_LINES_LIMIT);
159+
} catch (NumberFormatException e) {
160+
return DEFAULT_MAX_LINES;
161+
}
162+
}
163+
128164
/**
129165
* AJAX endpoint to check build status.
130166
* Returns JSON with buildingStatus to determine if button should be shown. 0 - SUCCESS, 1 - RUNNING, 2 - FINISHED and FAILURE
@@ -134,7 +170,7 @@ public void doExplainConsoleError(StaplerRequest2 req, StaplerResponse2 rsp) thr
134170
@RequirePOST
135171
public void doCheckBuildStatus(StaplerRequest2 req, StaplerResponse2 rsp) {
136172
try {
137-
run.checkPermission(hudson.model.Item.READ);
173+
run.checkPermission(Item.READ);
138174

139175
Integer buildingStatus = run.isBuilding() ? 1 : 0;
140176

‎src/main/java/io/jenkins/plugins/explain_error/ConsolePageDecorator.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import edu.umd.cs.findbugs.annotations.CheckForNull;
44
import hudson.Extension;
5+
import hudson.model.Item;
56
import hudson.model.PageDecorator;
67
import hudson.model.Run;
78
import io.jenkins.plugins.explain_error.provider.BaseAIProvider;
@@ -20,14 +21,16 @@ public ConsolePageDecorator() {
2021
}
2122

2223
/**
23-
* Whether the page offers to explain the build: explanation is enabled for it and its provider is
24-
* configured. Folder settings count, so a folder with a provider of its own gets the button while the
25-
* global switch is off, and a folder that turns explanation off does not get it.
24+
* Whether the page offers to explain the build: explanation is enabled for it, its provider is
25+
* configured and the user may build the job, which a new explanation requires. Folder settings count,
26+
* so a folder with a provider of its own gets the button while the global switch is off, and a folder
27+
* that turns explanation off does not get it. An explanation the build has is shown to anyone who can
28+
* read the build.
2629
*/
2730
public boolean isExplainErrorEnabled() {
2831
Run<?, ?> run = currentRun();
2932
if (run != null) {
30-
return new ErrorExplainer().canExplain(run);
33+
return run.hasPermission(Item.BUILD) && new ErrorExplainer().canExplain(run);
3134
}
3235
GlobalConfigurationImpl config = GlobalConfigurationImpl.get();
3336
return config.isEnableExplanation() && config.getAiProvider() != null

‎src/main/java/io/jenkins/plugins/explain_error/ExplainErrorStep.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,13 @@
2929
*/
3030
public class ExplainErrorStep extends Step {
3131

32+
/**
33+
* Files auto-fix may change unless the step says otherwise. CI and pipeline definitions such as the
34+
* Jenkinsfile are left out: a step that wants them changed lists their paths.
35+
*/
36+
public static final String DEFAULT_AUTO_FIX_ALLOWED_PATHS = "pom.xml,build.gradle,build.gradle.kts,"
37+
+ "*.properties,*.yml,*.yaml,Dockerfile,package.json,requirements.txt,go.mod";
38+
3239
private String logPattern;
3340
private int maxLines;
3441
private String language;
@@ -48,7 +55,7 @@ public class ExplainErrorStep extends Step {
4855
private String autoFixGithubEnterpriseUrl = "";
4956
private String autoFixGitlabUrl = "";
5057
private String autoFixBitbucketUrl = "";
51-
private String autoFixAllowedPaths = "pom.xml,build.gradle,build.gradle.kts,*.properties,*.yml,*.yaml,Jenkinsfile,Dockerfile,package.json,requirements.txt,go.mod";
58+
private String autoFixAllowedPaths = DEFAULT_AUTO_FIX_ALLOWED_PATHS;
5259
private boolean autoFixDraftPr = false;
5360
private int autoFixTimeoutSeconds = 120;
5461
private String autoFixPrTemplate = "";
@@ -227,8 +234,7 @@ public String getAutoFixAllowedPaths() {
227234

228235
@DataBoundSetter
229236
public void setAutoFixAllowedPaths(String autoFixAllowedPaths) {
230-
this.autoFixAllowedPaths = autoFixAllowedPaths != null ? autoFixAllowedPaths
231-
: "pom.xml,build.gradle,build.gradle.kts,*.properties,*.yml,*.yaml,Jenkinsfile,Dockerfile,package.json,requirements.txt,go.mod";
237+
this.autoFixAllowedPaths = autoFixAllowedPaths != null ? autoFixAllowedPaths : DEFAULT_AUTO_FIX_ALLOWED_PATHS;
232238
}
233239

234240
public boolean isAutoFixDraftPr() {

‎src/main/java/io/jenkins/plugins/explain_error/GraphViewExplainErrorAction.java‎

Lines changed: 38 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.google.common.annotations.VisibleForTesting;
44
import edu.umd.cs.findbugs.annotations.CheckForNull;
5+
import hudson.model.Item;
56
import hudson.model.Result;
67
import hudson.model.Run;
78
import io.jenkins.plugins.explain_error.provider.BaseAIProvider;
@@ -23,6 +24,7 @@
2324
import org.kohsuke.stapler.StaplerRequest2;
2425
import org.kohsuke.stapler.StaplerResponse2;
2526
import org.kohsuke.stapler.interceptor.RequirePOST;
27+
import org.springframework.security.access.AccessDeniedException;
2628

2729
/**
2830
* Action for the Pipeline Graph View integration.
@@ -75,7 +77,7 @@ public String getUrlName() {
7577
@RequirePOST
7678
public void doCheckBuildStatus(StaplerRequest2 req, StaplerResponse2 rsp) {
7779
try {
78-
run.checkPermission(hudson.model.Item.READ);
80+
run.checkPermission(Item.READ);
7981

8082
int buildingStatus = run.isBuilding() ? 1 : 0;
8183

@@ -109,7 +111,7 @@ public void doCheckBuildStatus(StaplerRequest2 req, StaplerResponse2 rsp) {
109111
@RequirePOST
110112
public void doCheckNodeStatus(StaplerRequest2 req, StaplerResponse2 rsp) {
111113
try {
112-
run.checkPermission(hudson.model.Item.READ);
114+
run.checkPermission(Item.READ);
113115

114116
String nodeId = req.getParameter("nodeId");
115117
boolean isFailed = isFailedNode(nodeId);
@@ -139,7 +141,7 @@ public void doCheckNodeStatus(StaplerRequest2 req, StaplerResponse2 rsp) {
139141
public void doExplainNodeError(StaplerRequest2 req, StaplerResponse2 rsp) throws IOException {
140142
long startTimeNanos = System.nanoTime();
141143
try {
142-
run.checkPermission(hudson.model.Item.READ);
144+
run.checkPermission(Item.READ);
143145

144146
String nodeId = req.getParameter("nodeId");
145147
// Check if user wants to force a new explanation
@@ -180,29 +182,44 @@ && cachedExplanationMatchesNode(existingAction, nodeId)) {
180182
return;
181183
}
182184

183-
// Extract logs from the selected node
184-
PipelineLogExtractor logExtractor = new PipelineLogExtractor(run, 200,
185-
Jenkins.getAuthentication2(), false, null);
186-
List<String> logLines = logExtractor.extractNodeLog(nodeId);
187-
this.urlString = logExtractor.getUrl();
188-
189-
if (logLines.isEmpty()) {
190-
writeJsonResponse(rsp, "error", "Unknown",
191-
"No log output found for the selected node.");
185+
// A new explanation calls the provider, which costs money, and replaces the one the build has
186+
run.checkPermission(Item.BUILD);
187+
if (!PendingExplanations.tryAdd(run)) {
188+
BaseAIProvider provider = switchCheck.providerFor(run);
189+
writeJsonResponse(rsp, "warning", provider != null ? provider.getProviderName() : "Unknown",
190+
ConsoleExplainErrorAction.ALREADY_PENDING);
192191
return;
193192
}
193+
try {
194+
// Extract logs from the selected node
195+
PipelineLogExtractor logExtractor = new PipelineLogExtractor(run,
196+
ConsoleExplainErrorAction.DEFAULT_MAX_LINES, Jenkins.getAuthentication2(), false, null);
197+
List<String> logLines = logExtractor.extractNodeLog(nodeId);
198+
this.urlString = logExtractor.getUrl();
199+
200+
if (logLines.isEmpty()) {
201+
writeJsonResponse(rsp, "error", "Unknown",
202+
"No log output found for the selected node.");
203+
return;
204+
}
194205

195-
String errorText = String.join("\n", logLines);
206+
String errorText = String.join("\n", logLines);
196207

197-
ErrorExplainer explainer = new ErrorExplainer();
198-
try {
199-
ErrorExplanationAction action = explainer.explainErrorText(errorText, urlString, run);
200-
writeJsonResponse(rsp, "success", action.getProviderName(), action.getExplanation(),
201-
action.getTitle(), null);
202-
} catch (ExplanationException ee) {
203-
writeJsonResponse(rsp, ee.getLevel(), explainer.getProviderName(), ee.getMessage(), null,
204-
ProviderFailure.describe(ee));
208+
ErrorExplainer explainer = new ErrorExplainer();
209+
try {
210+
ErrorExplanationAction action = explainer.explainErrorText(errorText, urlString, run);
211+
writeJsonResponse(rsp, "success", action.getProviderName(), action.getExplanation(),
212+
action.getTitle(), null);
213+
} catch (ExplanationException ee) {
214+
writeJsonResponse(rsp, ee.getLevel(), explainer.getProviderName(), ee.getMessage(), null,
215+
ProviderFailure.describe(ee));
216+
}
217+
} finally {
218+
PendingExplanations.remove(run);
205219
}
220+
} catch (AccessDeniedException e) {
221+
rsp.setStatus(403);
222+
writeJsonResponse(rsp, "error", "Unknown", "Error: " + e.getMessage());
206223
} catch (Exception e) {
207224
LOGGER.severe("Error explaining node error: " + e.getMessage());
208225
writeJsonResponse(rsp, "error", "Unknown", "Error: " + e.getMessage());

0 commit comments

Comments
 (0)