Skip to content

refactor(config): remove unused timeout fields#503

Open
staticflux-adi wants to merge 2 commits into
alibaba:mainfrom
staticflux-adi:remove-timeout-fields
Open

refactor(config): remove unused timeout fields#503
staticflux-adi wants to merge 2 commits into
alibaba:mainfrom
staticflux-adi:remove-timeout-fields

Conversation

@staticflux-adi

@staticflux-adi staticflux-adi commented Jul 25, 2026

Copy link
Copy Markdown

Description

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

Comment on lines 216 to 219
type LlmConversation struct {
Timeout int `json:"timeout"`

Messages []ChatMessage `json:"messages"`
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removing the Timeout field from LlmConversation will cause a compilation error in template_test.go at line 68, which references tpl.MainTask.Timeout. The test needs to be updated (the assertion should be removed) before this change can compile.

Additionally, scan_template.json still contains "timeout" values for multiple conversations (MAIN_TASK, PLAN_TASK, DEDUP_TASK, PROJECT_SUMMARY_TASK, MEMORY_COMPRESSION_TASK, RE_LOCATION_TASK). These values will now be silently ignored during JSON unmarshalling. Consider cleaning up the stale "timeout" entries from scan_template.json to avoid confusion.

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Review

Thanks for picking up the cleanup from #474! The direction is right — removing the per-task timeout fields that are now dead code in the scan template path.

However, the PR is incomplete and will fail to compile as-is. The Timeout field on LlmConversation is still actively used by the ocr llm test command via a separate package:

Remaining references (all in the ocr llm test path)

Location Usage
internal/config/testconnection/task.json "timeout": 120
internal/config/testconnection/testconnection.go LlmConversation.Timeout field
cmd/opencodereview/llm_cmd.go:58-59 reads task.Timeout to set context.WithTimeout

Suggested fix

To complete this cleanup:

  1. Remove "timeout": 120 from internal/config/testconnection/task.json
  2. Remove the Timeout field from testconnection.LlmConversation struct
  3. Simplify llm_cmd.go:57-60 to just timeout := 60 * time.Second — this is a connectivity test that sends one short message ("who are you"), so 60s is more than enough. The global OCR_LLM_TIMEOUT / client-level 5min timeout is too generous for a quick health check.

Minor: formatting

The diff leaves trailing blank lines where "timeout" was removed in the JSON and where the Timeout field was removed in the Go structs. Please clean those up to pass go fmt / keep the JSON tidy.

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

One more note: for changes that touch core structs or remove fields used across packages, please make sure to run a full local build (make build) and tests (make test) before submitting. A compile check would have caught the missing testconnection references immediately. The PR checklist mentions make test passes, but the current diff removes a field that's still referenced — so please double-check.

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.

2 participants