Skip to content

feat: [ENG-2486] Extend transport schemas with provider/model#554

Merged
bao-byterover merged 1 commit into
proj/persis-task-historyfrom
feat/ENG-2486
Apr 27, 2026
Merged

feat: [ENG-2486] Extend transport schemas with provider/model#554
bao-byterover merged 1 commit into
proj/persis-task-historyfrom
feat/ENG-2486

Conversation

@bao-byterover
Copy link
Copy Markdown
Collaborator

No description provided.

@bao-byterover bao-byterover requested a review from leehpham as a code owner April 27, 2026 03:25
@bao-byterover bao-byterover merged commit 3266fee into proj/persis-task-history Apr 27, 2026
3 checks passed
@bao-byterover bao-byterover deleted the feat/ENG-2486 branch April 27, 2026 03:25
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Claude finished @bao-byterover's task in 2m 31s —— View job


PR Review: feat: [ENG-2486] Extend transport schemas with provider/model

  • Gather context and read changed files
  • Analyze code for correctness, security, and quality
  • Post inline comments for specific issues
  • Post general summary with overall score

Summary

This PR extends four files to carry provider and model as optional string fields through the task tracking layer:

File Change
src/server/core/domain/transport/task-info.ts +model?, provider? to TaskInfo
src/server/core/domain/transport/schemas.ts +model?, provider? to TaskCreatedSchema and TaskListItemSchema
src/shared/transport/events/task-events.ts +model?, provider? to TaskListItem interface
test/unit/core/domain/transport/schemas.test.ts New test file covering both Zod schemas

The changes are backward-compatible (all new fields are optional), consistently applied across the type/interface/schema trio, and well-named with clear JSDoc.


Issues Found

questiontask-info.ts line 23: Where does model/provider actually get written? The PR only defines the fields — no implementation change populates them from ProviderConfigResponse at task-creation time. If the write path lives in a separate PR, that should be noted; otherwise these fields will silently remain undefined in all task records.

suggestionschemas.ts (TaskCreatedSchema, TaskListItemSchema): Both new fields are plain z.string(), which accepts "". Consider .min(1) to reject empty-string values that are semantically meaningless as provider/model identifiers.

suggestionschemas.test.ts: Test coverage is good for happy paths (with both fields, with one field, with neither). Adding a negative test for provider: "" / model: "" would complete the picture — especially if the .min(1) suggestion above is adopted.

nitpicktask-events.ts TaskListItem and schemas.ts TaskListItemSchema define the same shape in two places (interface vs Zod). This is a pre-existing drift risk, not introduced here, but worth noting: deriving the interface via z.infer<typeof TaskListItemSchema> in the shared transport file would eliminate the duplication.


Overall Score: 4 / 5

Clean, minimal, well-targeted. The main concern is the missing write path for the new fields — if that's not in a follow-up PR, the fields will be dead weight. All other issues are minor improvements.

/** Log entry ID set by lifecycle hook after onTaskCreate */
logId?: string
/** Active model id at task creation time */
model?: string
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.

question: Where does model (and provider below) get set on the TaskInfo object? I see it defined here but no implementation change populates these fields from ProviderConfigResponse. Without a write path, the fields will always be undefined in production task records. Can you point to the code that reads the active provider/model config and sets them at task-creation time?

@@ -517,6 +517,10 @@ export const TaskCreatedSchema = z.object({
files: z.array(z.string()).optional(),
/** Folder path for curate-folder task type */
folderPath: z.string().optional(),
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.

suggestion: Consider adding .min(1) validation to both provider and model here (and in TaskListItemSchema) to prevent meaningless empty-string values from passing validation:

Suggested change
folderPath: z.string().optional(),
model: z.string().min(1).optional(),

An empty string is technically valid as z.string() but semantically meaningless for a provider/model identifier. Failing fast with a clear error is better than silently storing "".

Fix this →

expect(result.data.provider).to.equal('byterover')
expect(result.data.model).to.equal(undefined)
}
})
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.

suggestion: The test suite covers the happy paths well, but is missing a negative test for empty strings. Since provider/model are z.string(), Zod will accept "" silently:

it('rejects empty string for provider', () => {
  const result = TaskListItemSchema.safeParse({
    ...baseEntry,
    provider: '',
  })
  expect(result.success).to.equal(false)
})

This is only useful if you add .min(1) to the schemas (see the related schema comment), but worth adding to document the intent.

Fix this →

bao-byterover added a commit that referenced this pull request May 9, 2026
* feat: [ENG-2486] Extend transport schemas with provider/model (#554)

* feat: [ENG-2487] TaskRouter stamps provider + model on task:create (#555)

* feat: [ENG-2488] Wire resolveActiveProvider in daemon bootstrap (#556)

* feat: [ENG-2491] Domain entity + store interface (Level 2 schema) (#559)

* feat: [ENG-2492] FileTaskHistoryStore: Index + Data API (basic save/get/list)

* feat: [ENG-2498] Transport schemas (pagination, get, delete events) (#560)

* feat: [ENG-2495] FileTaskHistoryStore: Delete + Clear API (#561)

* feat: [ENG-2494] FileTaskHistoryStore: Stale recovery on read (#562)

* feat: [ENG-2496] Daemon llmservice accumulator + real-time TaskHistoryHook (#565)

* feat: [ENG-2497] Daemon bootstrap wiring + per-project store factory + startup audit (#567)

* feat: [ENG-2499] Daemon handlers (list paginated, get, delete, bulk, clear) + task:deleted broadcast (#568)

* feat: [ENG-2493] FileTaskHistoryStore: Prune + Compaction (#569)

* Feat/eng 2515 (#575)

* feat: [ENG-2515] Task history persistence — correctness fixes before WebUI ships

* fix: [ENG-2515] retention design + WebUI provider/model payload

Two correctness fixes surfaced while preparing the M2.10–M2.13 WebUI
handover:

1. Disable age-based task history retention by default. Age prune (30
   days) was a design mistake: it silently deletes user tasks even when
   the count cap (1000) has plenty of room left. Task history is a
   business artifact for audit/review, not a log — count-based rotation
   is the correct retention policy. Set the default to 0; deployments
   that want time-based eviction can still opt in via the constructor
   option.

2. Expose provider/model on the WebUI task:created subscriber. Daemon
   stamps both fields on every task:create (M1) but the local
   TaskCreatedPayload omitted them, so the chip M1.04 builds on would
   have no source. Extend the payload type and forward both fields into
   upsertStatus.

* Feat/eng 2515 (#576)

* feat: [ENG-2515] Task history persistence — correctness fixes before WebUI ships

* fix: [ENG-2515] retention design + WebUI provider/model payload

Two correctness fixes surfaced while preparing the M2.10–M2.13 WebUI
handover:

1. Disable age-based task history retention by default. Age prune (30
   days) was a design mistake: it silently deletes user tasks even when
   the count cap (1000) has plenty of room left. Task history is a
   business artifact for audit/review, not a log — count-based rotation
   is the correct retention policy. Set the default to 0; deployments
   that want time-based eviction can still opt in via the constructor
   option.

2. Expose provider/model on the WebUI task:created subscriber. Daemon
   stamps both fields on every task:create (M1) but the local
   TaskCreatedPayload omitted them, so the chip M1.04 builds on would
   have no source. Extend the payload type and forward both fields into
   upsertStatus.

* fix: [ENG-2515] eliminate task-history test flake — close cache-invalidation race and add deterministic flush API

Two pathologies surfaced as ~13% flake on local and similar on CI:

1. Cache-invalidation race in production code. A firePrune pass that
   started before a save() could finish AFTER the save's
   indexCache = undefined, overwriting the invalidation with a snapshot
   missing the just-saved row. Symptom: list(), clear(), deleteMany()
   returning N-1 entries after N awaited saves. Fix: bump indexEpoch on
   every index write (save / tombstone / recovery / compaction);
   doReadIndexDedup samples the epoch at the start and skips its
   setCache step if the epoch advanced. Also clear indexDedupInFlight
   on every write so a list() cannot pick up a pre-write in-flight
   promise.

2. Timing-based polling in tests. waitForPruneToSettle polled at 80 x
   4ms with a 4-stable-check exit, which under CI parallel-worker load
   missed the setTimeout(0) macrotask window. Fix: expose
   flushPendingOperations() — each firePrune wraps its setTimeout in a
   promise assigned to pruneChain; the public method awaits the chain
   (loops because pruneRequested re-extends it) and drains
   operationLock. Test callers replace polling with this single
   await store.flushPendingOperations().

Plus: tempDir entropy upgraded from Math.random() to randomUUID() in
two test files to remove a residual collision risk in CI parallel runs.

Verification: a new race-regression test
(file-task-history-store-cache-race.test.ts) reproduces the race at
~27% rate on production code and drops to 0% post-fix; running each
affected test file 30x consecutively yields 120/120 pass; full suite
npm test reports 7103 passing, 0 failing.

* fix: [ENG-2515] FileTaskHistoryStore — chunk tombstone appends under 4 KB to keep concurrent saves from corrupting JSON lines

Large clear() / deleteMany() batches previously issued a single appendFile
of all tombstones joined. POSIX guarantees nothing about regular-file write
atomicity (PIPE_BUF applies to FIFOs/sockets only). On filesystems that
don't serialize appends per inode, an unlocked concurrent save() can
interleave content into the middle of a multi-line tombstone write,
corrupting a JSON line. The line is then silently skipped by
IndexLineSchema.safeParse, so the tombstone is "missing" from the dedup
map even though the data file was already unlinked → list() returns a
ghost row whose getById() returns undefined.

Fix: introduce chunkLinesByBytes (exported for direct unit-test) and
issue tombstone appends in chunks under MAX_APPEND_CHUNK_BYTES (3.5 KB).
Each individual appendFile call stays under the 4 KB page boundary so
the kernel page-cache write path treats it as a single sector-level
write on common filesystems. Chunks are sequential under the existing
withOperationLock so concurrent saves can only land BETWEEN chunks,
never WITHIN one.

Tests: chunker unit test covers boundary + oversized-line + 200-tombstone
realistic case; integration test covers (a) clear() with 200 entries
removing all tombstones + data files, (b) every appendFile chunk well-formed
JSON post-clear, (c) clear() interleaved with 50 concurrent fresh saves
keeping the index parseable.

* refactor: [ENG-2515] move TaskHistoryEntry into shared/ + runtime layer-isolation test

The persisted-entry shape lived in src/server/core/domain/entities/. The
WebUI Tasks tab needs the same TS type (TaskGetResponse, stored detail),
which forced a webui→server import — inverting the boundary the ESLint
rule on tui/ already enforces.

Move the TS shape (TaskHistoryEntry, TaskHistoryEntryBase, TaskErrorData,
TaskHistoryStatus, TASK_HISTORY_SCHEMA_VERSION) into
src/shared/transport/events/task-events.ts. The Zod schema stays in
src/server/core/domain/entities/task-history-entry.ts and now carries
`satisfies z.ZodType<TaskHistoryEntry>`, so any drift between the runtime
schema and the type is a typecheck error. The entity file re-exports the
type so existing server-side imports continue to compile unchanged.

test/unit/shared/layer-isolation.test.ts: runtime walk over src/shared/
asserting no `from '...server/...'` (or other higher layers) appears in
any .ts/.tsx file. Catches regressions even if a future ESLint rule is
bypassed.

Incidental: src/server/infra/process/task-router.ts picks up formatting
churn from the prettier/eslint-fix pass on the same change set (line
wrapping only, no logic change).

* feat: [ENG-2490] add provider chip column to WebUI task list

* feat: [ENG-2500] wire WebUI delete + bulk delete + clear-completed mutations

* feat: [ENG-2501] render task detail from disk via task:get for cold tasks

* feat: [ENG-2503] dim styling + tooltip for daemon-interrupted task entries

* feat: [ENG-2490] cover (provider, '') edge case in formatProviderModel

* feat: [ENG-2500] keep selection on bulk-delete failure + cover Delete failed fallback

* feat: [ENG-2501] gate task:get on live status + pin enabled + infinite staleTime

* feat: [ENG-2503] tighten isInterrupted status type + cover precedence

* feat: [ENG-2503] use design-system Tooltip for interrupted-row hint

* feat: [ENG-2539] M2.16 task:list server-side filter/search + numbered… (#594)

* feat: [ENG-2539] M2.16 task:list server-side filter/search + numbered pagination

Schema: drop cursor (before/limit/nextCursor), add page/pageSize/total/pageCount
+ searchText, provider[], model[], createdAfter/Before, minDuration/maxDurationMs.
Response adds counts (Model A: matches current filter scope), availableProviders,
availableModels (history-derived, exclude pivots so dropdowns dont shrink).

Handler: 2-pass search. Pass-1 over index summary (content + error.message).
Pass-2 lazy-cracks data file via store.getById for completed tasks where full
result text might match. In-memory tasks read task.result directly (no I/O).
Race-safe: getById errors swallowed with log-once-per-query.

Store: ITaskHistoryStore.list rename after/before -> createdAfter/createdBefore,
add provider/model coarse pre-filter, drop limit (handler paginates).

Tests: 5 new test files updated. New cases cover all 7 filter dims, AND
combination, pass-2 happy path (full result match past byte 1500), pass-2
in-memory path, file-race swallow, counts Model A invariant counts.all === total,
availableProviders/Models exclude pivots + symmetric empty-string guard.

* fix: [ENG-2539] M2.16 PR review fixes

Address all 8 inline comments from PR #594:

- Docs: counts is Model A post-filter, counts.all === total invariant
  (was incorrectly described as pre-status-filter)
- Docs: page is echoed back as-sent without upper-bound clamp against
  pageCount; documented contract on JSDoc
- Interface: document why provider/model/status options remain on
  ITaskHistoryStore.list (direct-caller use; handler bypasses pivots)
- mapBounded signature: explicit fn returns R | undefined
- handleTaskList: refactor 2-pass to single-pass over candidatesNoSearch;
  derive merged inline; pass-2 reuses the map (was 2xN, now 1xN)
- Comment: clarify pass2Filter spread auto-inherits new non-pivot dims
- Schemas: bind via satisfies z.ZodType<X> to task-events.ts interfaces;
  re-export TaskListRequest/Response/Counts/AvailableModel from shared so
  the interface is single source of truth (compile-time drift prevention)
- Tests: add maxDurationMs upper-bound + no-store fallback coverage

* feat: [ENG-2502] paginate task list with useInfiniteQuery + Load more table row

* feat: [ENG-2502] keyboard-accessible Load more row + indent describe blocks

* feat: [ENG-2547] M2.17 WebUI numbered pagination + Filter dropdown + active filter tags

* feat: [ENG-2547] address bot review: memo serverStatus + closeTask, dedup useGetProviders + isDurationPreset, memo tags

* feat: [ENG-2610] M2.18 pagination polish + default page size 20 + friendly provider names

* feat: [ENG-2610] memoize providerNames against providersResponse, not derived array

---------

Co-authored-by: ncnthien <nhatthien185@gmail.com>
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.

1 participant