Skip to content

Cleanup code, add proper error handling#97

Open
s8sg wants to merge 4 commits into
masterfrom
code_cleanup
Open

Cleanup code, add proper error handling#97
s8sg wants to merge 4 commits into
masterfrom
code_cleanup

Conversation

@s8sg

@s8sg s8sg commented Feb 17, 2026

Copy link
Copy Markdown
Owner

This PR focuses on cleanup and more robust error handling across the flow runtime and executor code paths.

Changes:

Simplifies redundant error construction and adds Redis command error checks in the runtime.
Fixes JSON struct tags for executor requests (ensuring proper marshal/unmarshal behavior).
Prevents potential nil dereferences during executor cleanup and adds error handling around data store copying.

@s8sg s8sg self-assigned this Feb 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR focuses on cleanup and more robust error handling across the flow runtime and executor code paths.

Changes:

  • Simplifies redundant error construction and adds Redis command error checks in the runtime.
  • Fixes JSON struct tags for executor requests (ensuring proper marshal/unmarshal behavior).
  • Prevents potential nil dereferences during executor cleanup and adds error handling around data store copying.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
runtime/flow_runtime.go Removes redundant fmt.Sprintf usage and adds error handling for Redis Set/Del.
core/sdk/executor/request.go Fixes invalid JSON tags and a minor comment formatting issue.
core/sdk/executor/executor.go Adds nil-guards for dataStore.Cleanup(), improves failure logging, and handles CopyStore() errors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread core/sdk/executor/executor.go
Comment thread core/sdk/executor/executor.go Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI commented Feb 18, 2026

Copy link
Copy Markdown

@s8sg I've opened a new pull request, #98, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: s8sg <7374058+s8sg@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@s8sg

s8sg commented May 12, 2026

Copy link
Copy Markdown
Owner Author

@copilot apply changes based on the comments in this thread

Copilot AI commented May 12, 2026

Copy link
Copy Markdown

@copilot apply changes based on the comments in this thread

Applied in this branch via commits 5146465 and 612ab13. The executor failure path now preserves and reports the original error, and the local dataStore typo is corrected.

Copilot finished work on behalf of s8sg May 12, 2026 05:29
@s8sg

s8sg commented May 12, 2026

Copy link
Copy Markdown
Owner Author

@copilot Fix the test cases that are failing

Copilot AI commented May 12, 2026

Copy link
Copy Markdown

@copilot Fix the test cases that are failing

I checked the failing run and re-ran tests on this branch: go test ./... passes locally, and current GO-Flow-Build runs on code_cleanup are successful. The only failing historical run was due to deprecated actions/upload-artifact@v1, already fixed in ea5720b. If you’re seeing a different failing test/job, share that run link and I’ll fix it.

Copilot finished work on behalf of s8sg May 12, 2026 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants