SED-4157-executions history #577

lucianbaciu · 2025-12-18T16:33:55Z

No description provided.

chatgpt-codex-connector · 2025-12-18T16:33:59Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

gemini-code-assist · 2025-12-18T16:34:17Z

Summary of Changes

Hello @lucianbaciu, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request implements a foundational feature for tracking and displaying the execution history of plans. It involves extending core data models to include canonical plan identifiers and historical result snapshots, introducing a new plugin to collect this data post-execution, and refining the execution engine's logic to correctly populate these new fields. These changes are crucial for providing users with insights into the performance and outcomes of their plans over time.

Highlights

Execution History Reporting: Introduced a new ExecutionHistoryReportPlugin to automatically capture and store snapshots of past execution results for a given plan, enabling historical tracking.
Canonical Plan Naming: Enhanced ImportResult and Execution models with a canonicalPlanName field to uniquely identify and group executions of the same plan across different imports or runs.
Execution Data Model Updates: The Execution object now includes a list of ExecutionResultSnapshot objects, allowing it to store a concise history of previous runs directly.
Data Access and Indexing: Updated ExecutionAccessor and ExecutionAccessorImpl to support efficient querying of past executions by canonicalPlanName and added a new database index for performance.
Execution Engine Integration: Modified the ExecutionEngineRunner to correctly determine and assign the canonicalPlanName during plan execution, ensuring proper linkage for history tracking.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces functionality to track execution history by defining a canonical plan name. A new plugin is added to fetch and attach this history to executions. The changes are logical and well-implemented. My review includes suggestions to improve consistency in canonical name generation, remove dead code, make a hardcoded value configurable, and some minor code style improvements.

gemini-code-assist · 2025-12-18T16:35:57Z

step-plans/step-plans-core/src/main/java/step/core/execution/ExecutionEngineRunner.java

+						// plan already exists in memory
+						canonicalPlanName = plan.getId().toString();


When a plan is passed directly in ExecutionParameters, the canonical plan name is set to the plan's ID. This can lead to inconsistencies if the same plan is sometimes imported (getting a canonical name from repository parameters) and sometimes passed directly. To ensure consistency, you should try to generate the canonical name from executionParameters.getRepositoryObject() if it's available, even when the plan object is already present. The current approach might result in fragmented execution histories for the same logical plan.

This will require adding import static step.core.repositories.AbstractRepository.getCanonicalPlanName; to the file.

// plan already exists in memory RepositoryObjectReference repositoryObject = executionParameters.getRepositoryObject(); if (repositoryObject != null) { canonicalPlanName = getCanonicalPlanName(repositoryObject.getRepositoryParameters()); } else { canonicalPlanName = plan.getId().toString(); }

gemini-code-assist · 2025-12-18T16:35:57Z

...ans/step-plans-base-artefacts/src/main/java/step/reporting/ExecutionHistoryReportPlugin.java

+
+    @Override
+    public void executionStart(ExecutionContext context) {
+        // TOOD collect the executions here too


There is a typo in the comment. It should be TODO.

Suggested change

// TOOD collect the executions here too

// TODO collect the executions here too

gemini-code-assist · 2025-12-18T16:35:57Z

...ans/step-plans-base-artefacts/src/main/java/step/reporting/ExecutionHistoryReportPlugin.java

+        Execution execution = context.getExecutionManager().getExecution();
+        // endTime is not set here
+        long searchBeforeTimestamp = System.currentTimeMillis() - 1;
+        List<ExecutionResultSnapshot> pastExecutionsSnapshots = executionAccessor.getLastEndedExecutionsByCanonicalPlanName(execution.getCanonicalPlanName(), 10, searchBeforeTimestamp)


The number of past executions to fetch is hardcoded to 10. This value should be made configurable to allow for flexibility in different environments. I recommend extracting it into a constant, and ideally, loading it from a configuration file or system property.

agreed, get the value from the configuration (step.properties) with a default value of 10.

gemini-code-assist · 2025-12-18T16:35:57Z

step-plans/step-plans-core/src/main/java/step/core/repositories/AbstractRepository.java

+        return repositoryParameters.entrySet().stream()
+                .sorted(Map.Entry.comparingByKey())
+                .map(e -> e.getKey() + "=" + e.getValue())
+                .collect(java.util.stream.Collectors.joining("&"));


The fully qualified name java.util.stream.Collectors is used here, but java.util.stream.Collectors is already imported. You can simplify this by using just Collectors.joining("&").

Suggested change

.collect(java.util.stream.Collectors.joining("&"));

.collect(Collectors.joining("&"));

lucianbaciu · 2025-12-18T16:36:26Z

@david-stephan please take a look on this before leaving in vacation, if you can. Ignore the cleanup/formatting part. Let me know if the approach is fine.

david-stephan

@lucianbaciu in addition to the feedback from Gemini which are valid too, please see my comments

david-stephan · 2025-12-19T07:55:39Z

...ans/step-plans-base-artefacts/src/main/java/step/reporting/ExecutionHistoryReportPlugin.java

+        ExecutionAccessor executionAccessor = context.getExecutionAccessor();
+        Execution execution = context.getExecutionManager().getExecution();
+        // endTime is not set here
+        long searchBeforeTimestamp = System.currentTimeMillis() - 1;


This might be true now, but can change in the future, you should first check the execution end time and only if not set set use the current time.

david-stephan · 2025-12-19T07:56:46Z

...ans/step-plans-base-artefacts/src/main/java/step/reporting/ExecutionHistoryReportPlugin.java

+        Execution execution = context.getExecutionManager().getExecution();
+        // endTime is not set here
+        long searchBeforeTimestamp = System.currentTimeMillis() - 1;
+        List<ExecutionResultSnapshot> pastExecutionsSnapshots = executionAccessor.getLastEndedExecutionsByCanonicalPlanName(execution.getCanonicalPlanName(), 10, searchBeforeTimestamp)


agreed, get the value from the configuration (step.properties) with a default value of 10.

david-stephan · 2025-12-19T07:59:15Z

...ans/step-plans-base-artefacts/src/main/java/step/reporting/ExecutionHistoryReportPlugin.java

+        // endTime is not set here
+        long searchBeforeTimestamp = System.currentTimeMillis() - 1;
+        List<ExecutionResultSnapshot> pastExecutionsSnapshots = executionAccessor.getLastEndedExecutionsByCanonicalPlanName(execution.getCanonicalPlanName(), 10, searchBeforeTimestamp)
+                        .stream()


getLastEndedExecutionsByCanonicalPlanName should return a stream directly

david-stephan · 2025-12-19T08:00:52Z

...ans/step-plans-base-artefacts/src/main/java/step/reporting/ExecutionHistoryReportPlugin.java

+        long searchBeforeTimestamp = System.currentTimeMillis() - 1;
+        List<ExecutionResultSnapshot> pastExecutionsSnapshots = executionAccessor.getLastEndedExecutionsByCanonicalPlanName(execution.getCanonicalPlanName(), 10, searchBeforeTimestamp)
+                        .stream()
+                                .map(e -> new ExecutionResultSnapshot()


Think of the information you'll need to also display a tooltip, at least the execution start time would be required too

david-stephan · 2025-12-19T08:05:37Z

step-plans/step-plans-core/src/main/java/step/core/execution/model/Execution.java

+		return historyResults;
+	}
+
+	public Execution setHistoryResults(List<ExecutionResultSnapshot> historyResults) {


Use at standard setter, must be void

david-stephan · 2025-12-19T09:02:14Z

step-plans/step-plans-core/src/main/java/step/core/execution/ExecutionEngineRunner.java

 				try {
-					Plan plan = getPlanFromExecutionParametersOrImport();
+					ExecutionParameters executionParameters = executionContext.getExecutionParameters();
+					Plan plan = executionParameters.getPlan();


You could simply check where we use the executionParameters.setPlan to see that this case is used for interactive execution (see startInteractiveSession in InteractiveServices), if you want more details or debug the case. This case is edgy and it would actually be cleaner and more correct to not create any historical statuses for them (or even ingest any time-series for them). For now I would then not set the canonical name and handle null value in the plugin

david-stephan · 2025-12-19T09:09:19Z

step-plans/step-plans-core/src/main/java/step/core/repositories/AbstractRepository.java


+    public static String getCanonicalPlanName(Map<String, String> repositoryParameters) {
+        if (repositoryParameters == null || repositoryParameters.isEmpty()) {
+            return "";


We should return null to be consistent and let the caller know that no canonical name exist

david-stephan · 2025-12-19T09:10:44Z

step-plans/step-plans-core/src/main/java/step/core/repositories/AbstractRepository.java

+        }
+
+        return repositoryParameters.entrySet().stream()
+                .sorted(Map.Entry.comparingByKey())


You are not filtering by canonicalRepositoryParameters, this is not a canonical representation.

david-stephan · 2025-12-19T09:11:00Z

step-plans/step-plans-core/src/main/java/step/core/repositories/AbstractRepository.java

                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)) : null;
    }

+    public static String getCanonicalPlanName(Map<String, String> repositoryParameters) {


This cannot be a static method since you need access to the canonical keys set

david-stephan · 2025-12-19T09:14:09Z

step-plans/step-plans-core/src/main/java/step/core/repositories/RepositoryObjectManager.java

 		Repository repository = getRepository(respositoryId);
-		return repository.importArtefact(context, artefact.getRepositoryParameters());
+		ImportResult importResult = repository.importArtefact(context, artefact.getRepositoryParameters());
+		importResult.setCanonicalPlanName(getCanonicalPlanName(artefact.getRepositoryParameters()));


Cannot be a static call, you have the repository instance right above.

SED-4157-executions history

416aafc

gemini-code-assist bot reviewed Dec 18, 2025

View reviewed changes

david-stephan requested changes Dec 19, 2025

View reviewed changes

		// plan already exists in memory
		canonicalPlanName = plan.getId().toString();

	// TOOD collect the executions here too
	// TODO collect the executions here too

	.collect(java.util.stream.Collectors.joining("&"));
	.collect(Collectors.joining("&"));

SED-4157-executions history #577

Are you sure you want to change the base?

SED-4157-executions history #577

Uh oh!

Conversation

lucianbaciu commented Dec 18, 2025

Uh oh!

chatgpt-codex-connector bot commented Dec 18, 2025

Uh oh!

gemini-code-assist bot commented Dec 18, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

lucianbaciu commented Dec 18, 2025

Uh oh!

david-stephan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants