Skip to content

fix(config): remove duplicate annotate on provider timeout/chunkTimeout (F1)#4

Closed
tesdal wants to merge 1 commit intophase-ab-basefrom
audit-f1-double-annotate
Closed

fix(config): remove duplicate annotate on provider timeout/chunkTimeout (F1)#4
tesdal wants to merge 1 commit intophase-ab-basefrom
audit-f1-double-annotate

Conversation

@tesdal
Copy link
Copy Markdown
Owner

@tesdal tesdal commented Apr 23, 2026

Summary

Audit finding F1 from 2026-04-22 diamond audit: ConfigProvider.Info schema double-annotates timeout and chunkTimeout. Inner Schema.Int.pipe(Schema.greaterThanOrEqualTo(0)).annotations({ description, default }) already attaches description + default; outer .annotations({...}) wrapper overwrites with identical values. Pure duplication.

Changes

  • packages/opencode/src/config/provider.ts — drop outer .annotations() on both fields. Inner description preserved character-for-character.
  • packages/opencode/test/config/provider-schema.test.ts — new, 2 AST-based tests that serialize ConfigProvider.Info.ast and assert each description appears exactly once. Fail before fix, pass after (verified by reviewer reverting + reapplying).

Verification

  • bun typecheck — clean
  • bun test test/config/provider-schema.test.ts — 2/2 pass
  • bun test test/session/subagent-hang-regression.test.ts — 2/2 pass (unaffected)
  • SDK regen (./packages/sdk/js/script/build.ts) — zero diff to types.gen.ts, codegen dedupes at HTTP layer. Fix is still valid at AST level.

Diamond review status

  • Spec compliance (codex-5.3): 9/11 ✅, 2 minor deviations accepted (test uses ConfigProvider.Info.ast directly vs. Schema.asSchema(...) — functionally equivalent).
  • Code quality (Opus): APPROVE. Three Minor nits (test duplication, needle brittleness, plan wording) deemed deferrable. Reviewer independently verified tests fail at base and pass at head.

Context

Part of 14-task audit remediation plan (docs/superpowers/plans/2026-04-23-audit-remediation.md). Base phase-ab-base now tracks b2cc4ac8d (local/integration-v2 HEAD). Draft PR opened for Copilot review only — merge target is local integration branch, not upstream.

…Timeout

The inner Union schema already carried the description. The outer
Schema.optional annotation re-applied it, producing duplicate entries
in SDK codegen output. Collapses to a single annotation.

Regression test: test/config/provider-schema.test.ts asserts each
description substring appears exactly once in the schema AST.

Addresses audit finding F1 (Opus diamond review, 2026-04-22).
@tesdal tesdal requested a review from Copilot April 23, 2026 21:22
@github-actions
Copy link
Copy Markdown

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Remediates audit finding F1 by removing duplicate description annotations on ConfigProvider.Info’s timeout and chunkTimeout fields, and adds regression tests to ensure the schema AST contains each description exactly once.

Changes:

  • Remove redundant outer .annotate({ description: ... }) wrappers for timeout and chunkTimeout in ConfigProvider.Info.
  • Add Bun tests that serialize ConfigProvider.Info.ast and assert the relevant description strings occur exactly once.

Reviewed changes

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

File Description
packages/opencode/src/config/provider.ts Drops duplicate .annotate(...) on timeout and chunkTimeout fields to avoid double annotation in the schema AST.
packages/opencode/test/config/provider-schema.test.ts Adds AST-based regression tests to prevent reintroducing duplicate description annotations.

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

@tesdal
Copy link
Copy Markdown
Owner Author

tesdal commented Apr 23, 2026

Copilot review: no comments. All three reviewers (codex-5.3 spec 9/11, Opus quality APPROVE, Copilot clean) agree. Merging --no-ff into local/integration-v2. PR closed as review-only.

@tesdal tesdal closed this Apr 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants