Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a67f1c95-032a-47bb-8029-43b855b977d6

Reviewing files that changed from the base of the PR and between 953b253 and ad702ab.

• .changeset/lcm-observed-work-density.md
• .changeset/lcm-observed-work-extraction.md
• .changeset/lcm-task-bridge-suggestions.md
• openclaw.plugin.json
• specs/lcm-observed-work-density-option-b.md
• specs/lcm-task-bridge-option-c-experimental.md
• src/db/migration.ts
• src/engine.ts
• src/observed-work-extractor.ts
• src/plugin/index.ts
• src/store/observed-work-store.ts
• src/store/task-bridge-suggestion-store.ts
• src/tools/lcm-work-density-tool.ts
• test/observed-work-store.test.ts
• test/task-bridge-suggestion-store.test.ts

• GitHub Check: smoke-latest-openclaw
• GitHub Check: smoke-latest-openclaw
• GitHub Check: Agent

src/engine.ts (4)

73-73: Looks good.

The new ObservedWorkStore import matches the rest of the engine wiring and is used by the added store field/accessor path.

---

1830-1833: Safe compatibility tweak.

Defaulting now to new Date() preserves the existing behavior for current callers while making the helper easier to use in tests and future call sites.

---

1872-2000: Observed-work store wiring is consistent.

The new field is initialized after migrations, which keeps the store creation aligned with the existing DB-backed store lifecycle.

---

8017-8019: Accessor looks fine.

Exposing getObservedWorkStore() is a clean way to surface the new read model without leaking the internal field directly.

src/plugin/index.ts (1)

24-24: Tool wiring looks correct.

lcm_work_density is imported and registered with the same dependency/session wiring pattern as existing LCM tools.

Also applies to: 2308-2314

openclaw.plugin.json (1)

14-15: Manifest update is consistent with runtime registration.

Adding lcm_work_density to contracts.tools is correct and keeps plugin contracts aligned.

src/observed-work-extractor.ts (1)

197-313: Extraction flow and failure isolation are solid.

The row-cursor strategy plus per-summary savepoints provides good idempotency and retry behavior for partial-write failures.

Also applies to: 315-423

test/observed-work-store.test.ts (1)

65-867: Test coverage is strong and targeted.

The suite covers cursor semantics, rollback/idempotency, period handling, source redaction, and output-budget trimming with deterministic clocks.

test/task-bridge-suggestion-store.test.ts (1)

64-294: Good validation of suggestion lifecycle behavior.

Tests verify non-mutating semantics, review transitions, and guardrails for malformed or non-authoritative inputs.

specs/lcm-task-bridge-option-c-experimental.md (1)

1-211: Spec direction is clear and safely constrained.

The explicit non-goals and opt-in gates are well-defined for an experimental bridge.

src/store/task-bridge-suggestion-store.ts (1)

131-143: Consider chunking source-ID validation for SQLite compatibility across versions.

At line 135, the single IN (...) query expands all sourceIds into one parameter list. While modern SQLite (3.32.0+, default limit 32766) handles this well, older versions (pre-3.32.0, limit 999) will fail with "too many SQL variables" when sourceIds.length + 1 exceeds 999 bind parameters. If the codebase targets older SQLite versions or supports them in production, chunking by ~500 items prevents this failure across all versions and is a low-effort safeguard.

💡 Proposed fix

   private assertSourceIdsBelongToWorkItem(
     workItemId: string,
     sourceIds: string[]
   ): void {
-    const placeholders = sourceIds.map(() => "?").join(", ");
-    const rows = this.db
-      .prepare(
-        `SELECT DISTINCT source_id
-         FROM lcm_observed_work_sources
-         WHERE work_item_id = ?
-           AND source_id IN (${placeholders})`
-      )
-      .all(workItemId, ...sourceIds) as Array<{ source_id: string }>;
-    const found = new Set(rows.map((row) => row.source_id));
+    const found = new Set<string>();
+    for (let offset = 0; offset < sourceIds.length; offset += 500) {
+      const chunk = sourceIds.slice(offset, offset + 500);
+      const placeholders = chunk.map(() => "?").join(", ");
+      const rows = this.db
+        .prepare(
+          `SELECT DISTINCT source_id
+           FROM lcm_observed_work_sources
+           WHERE work_item_id = ?
+             AND source_id IN (${placeholders})`
+        )
+        .all(workItemId, ...chunk) as Array<{ source_id: string }>;
+      for (const row of rows) found.add(row.source_id);
+    }
     const missing = sourceIds.filter((sourceId) => !found.has(sourceId));
     if (missing.length > 0) {
       throw new Error(
         `source IDs must reference observed-work evidence for this work item: ${missing.join(", ")}`
       );
     }
   }

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 79420f68-8ebb-4fdd-b84f-102fce699173

Reviewing files that changed from the base of the PR and between ad702ab and cf7bdfb.

• src/tools/lcm-work-density-tool.ts
• test/observed-work-store.test.ts

• GitHub Check: smoke-latest-openclaw
• GitHub Check: smoke-latest-openclaw

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8eee747f-11f7-4c8a-a89d-dbfb37f948c0

Reviewing files that changed from the base of the PR and between cf7bdfb and b827631.

• specs/lcm-observed-work-density-option-b.md
• src/engine.ts
• src/store/observed-work-store.ts
• src/store/task-bridge-suggestion-store.ts
• src/timezone-windows.ts
• src/tools/lcm-recent-tool.ts
• src/tools/lcm-work-density-tool.ts
• test/observed-work-store.test.ts

• GitHub Check: smoke-latest-openclaw

src/timezone-windows.ts (1)

28-40: Good extraction and correct ISO-week Monday logic.

• The helper is deterministic and correctly handles all weekdays (Sunday -> previous Monday).
• Reusing assertValidPlainDate + addDays keeps validation/date arithmetic consistent across callers.

src/tools/lcm-recent-tool.ts (1)

13-14: Nice deduplication of week-boundary logic.

• Centralizing startOfWeekDayString removes drift risk between lcm_recent and lcm_work_density.
• The import-and-use path is clean and maintainable.

Also applies to: 268-269

src/store/task-bridge-suggestion-store.ts (1)

331-339: Good pending-only transition guard in reviewSuggestion

WHERE suggestion_id = ? AND status = 'pending' cleanly prevents accidental re-review of already finalized suggestions.

src/engine.ts (1)

73-73: ObservedWorkStore integration is clean and complete.

• The dependency is wired consistently (import, field, constructor init, public accessor).
• This keeps tool-facing access explicit and avoids hidden singleton/state coupling.

Also applies to: 1872-1872, 1999-1999, 8017-8019

src/tools/lcm-work-density-tool.ts (6)

366-369: Honor explicit since/before even when period is invalid.

resolvePeriodBounds is called before parsing explicit bounds. If period is invalid (e.g., "quarter"), it throws on line 366 before lines 368-369 execute—rejecting requests that provide valid explicit timestamps.

Proposed fix

-        const periodBounds = resolvePeriodBounds(p.period, lcm.timezone, () => input.deps.clock.now());
-        periodLabel = periodBounds.label;
-        since = parseIsoTimestampParam(p, "since")?.toISOString() ?? periodBounds.since;
-        before = parseIsoTimestampParam(p, "before")?.toISOString() ?? periodBounds.before;
+        const explicitSince = parseIsoTimestampParam(p, "since")?.toISOString();
+        const explicitBefore = parseIsoTimestampParam(p, "before")?.toISOString();
+        const periodBounds =
+          explicitSince || explicitBefore
+            ? {}
+            : resolvePeriodBounds(p.period, lcm.timezone, () => input.deps.clock.now());
+        periodLabel = periodBounds.label;
+        since = explicitSince ?? periodBounds.since;
+        before = explicitBefore ?? periodBounds.before;

---

1-68: LGTM!

• Imports and constants correctly defined
• TypeBox schema uses Type.Literal unions for enum-like values with proper constraints
• Schema matches the store's ObservedWorkDensityQuery contract

---

70-144: LGTM!

Period resolution helpers:

• resolvePeriodBounds correctly returns empty object for null/empty period
• dayBounds and nextMonthStartDay compute correct UTC boundaries
• Handles all documented period formats: today, yesterday, date:YYYY-MM-DD, 7d, 30d, week, month

---

146-198: LGTM!

Input validation helpers are robust:

• parseBoundedInteger validates type, finiteness, and integer constraint
• parseBoundedNumber validates type and range
• arrayParam validates array type and validates each entry against allowed values

---

200-323: LGTM!

Output budgeting implementation:

• Guard limit of 10,000 iterations prevents infinite loops
• Trim strategy: sources first, then items
• Accounting fields updated inline during trim loop for accurate token estimates
• Final estimatedOutputTokens computed after all trimming

Minor: DETAIL_ARRAY_KEYS includes staleItems and transitions which aren't populated in the response, but this doesn't cause issues and may be intentional for forward compatibility.

---

325-432: LGTM!

Tool factory correctly:

• Resolves conversation scope with proper rejection of allConversations=true
• Validates numeric parameters with bounded integer/number helpers
• Constructs query for getDensity with all optional fields
• Shapes response based on detailLevel (compact vs full)
• Applies output budget trimming before returning

test/observed-work-store.test.ts (4)

1-51: LGTM!

Test setup helpers are well-structured:

• makeDb() enables PRAGMA foreign_keys = ON matching production
• createConversation() and insertLeafSummary() provide clean test fixtures

---

53-204: LGTM!

Strong test coverage for store/extractor behavior:

• Rowid cursor ensures same-second summaries aren't skipped
• Retry idempotency prevents evidence count inflation
• Savepoint rollback on mid-extraction failure ensures clean recovery
• Cursor fallback tests cover rowid drift and missing summary ID scenarios

---

695-815: LGTM!

Previous review concern about proving the tool uses deps.clock.now() is resolved:

• Lines 739-744: Uses const clockNow = vi.fn(() => now) as a deterministic stub
• Line 811: Asserts expect(clockNow).toHaveBeenCalled() to verify the tool routes through the injected clock

This ensures the test fails if the tool regresses to new Date().

---

817-880: LGTM!

Output budget test validates:

• 20 items with 5 sources each exceeds the 256-token budget
• budgetTruncated is set to true
• itemsReturned < 20 confirms trimming occurred
• estimatedOutputTokens <= 256 confirms budget is honored

specs/lcm-observed-work-density-option-b.md (3)

1-75: LGTM!

Specification clearly establishes:

• Option B philosophy: observed work density, not a second task system
• Vocabulary distinction: "observed work" vs authoritative "tasks"
• Use cases and example UX

---

76-183: LGTM!

Previous review concern about enum/response drift is resolved. The spec now includes:

• kind enum with other (line 85)
• source_type with message (line 112)
• observed_status with decision_recorded (line 84)
• Response contract with decisionRecorded count and decisions/dismissedItems arrays (lines 167, 172-173)

---

185-270: LGTM!

Behavioral rules are clear:

• Read-only tool (no mutations from reads)
• Source IDs gated behind includeSources=true
• Explicit non-goals documented

Implementation scaffold section accurately reflects the current PR state.