Remove Deck URI when OutputReader is nil

[Future-Outlier]

Tracking issue

#5574
#3936

Why are the changes needed?

if the reader is nil, we should remove the deck URI.
we should only check if the reader exists, then the deck will exist.
Note: it's possible to render deck in agent's any task and now it's only possible when executing databricks agent.
related code snippet:

flyte/flyteplugins/go/tasks/plugins/webapi/agent/plugin.go

Lines 365 to 385 in 1f533ab

	func writeOutput(ctx context.Context, taskCtx webapi.StatusContext, outputs *flyteIdl.LiteralMap) error {
	taskTemplate, err := taskCtx.TaskReader().Read(ctx)
	if err != nil {
	return err
	}
	if taskTemplate.GetInterface() == nil || taskTemplate.GetInterface().GetOutputs() == nil || taskTemplate.Interface.Outputs.Variables == nil {
	logger.Debugf(ctx, "The task declares no outputs. Skipping writing the outputs.")
	return nil
	}
	var opReader flyteIO.OutputReader
	if outputs != nil {
	logger.Debugf(ctx, "AgentDeployment returned an output.")
	opReader = ioutils.NewInMemoryOutputReader(outputs, nil, nil)
	} else {
	logger.Debugf(ctx, "AgentDeployment didn't return any output, assuming file based outputs.")
	opReader = ioutils.NewRemoteFileOutputReader(ctx, taskCtx.DataStore(), taskCtx.OutputWriter(), 0)
	}
	return taskCtx.OutputWriter().Put(ctx, opReader)
	}

What changes were proposed in this pull request?

if the reader is nil, remove the deck URI.

How was this patch tested?

remote execution with a sensor agent task.

Setup process

single binary.

Screenshots

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Docs link

Summary by Bito

This PR enhances the task handler by improving Deck URI removal logic when deck files are absent. The changes include adding null checks for OutputInfo, separating error handling from existence checks, and modifying the ValidateOutput method signature. These modifications result in more robust code with clearer error handling patterns.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

[flyte-bot]

Code Review Agent Run #54fd06

Actionable Suggestions - 2

• flytepropeller/pkg/controller/nodes/task/handler.go - 2

  • Consider separating error and existence checks · Line 107-107
  • Consider keeping pointer receiver for consistency · Line 780-780

Review Details

• Files reviewed - 1 · Commit Range: 8b97c36..8b97c36

  • flytepropeller/pkg/controller/nodes/task/handler.go
• Files skipped - 0
• Tools

  • Golangci-lint (Linter) - ✖︎ Failed
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

AI Code Review powered by

[flyte-bot]

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Bug Fix - Fix Deck URI Handling in Task Handler	- handler.go - Improved handling of Deck URI removal and validation output method signature

[flyte-bot]

Consider separating error and existence checks

Consider separating error handling from existence check to improve code clarity. The current combined condition if err != nil || !exists { makes the error handling less explicit.

Code suggestion

Check the AI-generated fix before applying

 -	if err != nil || !exists {
 -		p.execInfo.OutputInfo.DeckURI = nil
 -	}
 -
 -	if err != nil {
 +	if err != nil {
 +		p.execInfo.OutputInfo.DeckURI = nil
  		return regErrors.Wrapf(err, "failed to check existence of deck file")
 +	}
 +
 +	if !exists {
 +		p.execInfo.OutputInfo.DeckURI = nil
  	}

Code Review Run #54fd06

---

Is this a valid issue, or was it incorrectly flagged by the Agent?

• it was incorrectly flagged

[flyte-bot]

Consider keeping pointer receiver for consistency

Consider keeping the pointer receiver *Handler instead of value receiver Handler for consistency. The ValidateOutput method appears to be used in multiple places and changing to value receiver could lead to unnecessary copying.

Code suggestion

Check the AI-generated fix before applying

Suggested change

	func (t Handler) ValidateOutput(ctx context.Context, nodeID v1alpha1.NodeID, i io.InputReader,
	func (t *Handler) ValidateOutput(ctx context.Context, nodeID v1alpha1.NodeID, i io.InputReader,

Code Review Run #54fd06

---

Is this a valid issue, or was it incorrectly flagged by the Agent?

• it was incorrectly flagged

[codecov]

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 36.86%. Comparing base (588a619) to head (d3fcc2d).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...lytepropeller/pkg/controller/nodes/task/handler.go	28.57%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #6223       +/-   ##
===========================================
- Coverage   49.69%   36.86%   -12.84%     
===========================================
  Files         872     1318      +446     
  Lines       66911   134534    +67623     
===========================================
+ Hits        33252    49596    +16344     
- Misses      30636    80617    +49981     
- Partials     3023     4321     +1298     

Flag	Coverage Δ
unittests-datacatalog	51.58% <ø> (ø)
unittests-flyteadmin	51.94% <ø> (?)
unittests-flytecopilot	30.99% <ø> (ø)
unittests-flytectl	62.33% <ø> (+0.04%)	⬆️
unittests-flyteidl	7.23% <ø> (?)
unittests-flyteplugins	53.91% <ø> (ø)
unittests-flytepropeller	42.78% <28.57%> (-0.01%)	⬇️
unittests-flytestdlib	55.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[flyte-bot]

Code Review Agent Run Status

• Limitations and other issues: ❌ Failure - Bito Code Review Agent couldn't review this pull request because it is no longer valid. It may have been merged, or the source/target branch may not exist.