feat(summary): add unified summary workspace backend by wanyiwang06 · Pull Request #234 · Mininglamp-OSS/octo-smart-summary

wanyiwang06 · 2026-09-01T07:42:11Z

Adds the unified summary workspace backend, Agent and Workflow routing, idempotent workflow creation, citation/source propagation, history compaction, channel discovery safeguards, database migrations, and regression coverage. Validation: core Agent, Service, Pipeline, DB, auth, config, model, notification, streaming, and timing tests pass. Four CGO-dependent packages cannot link on this host because libtokenizers is unavailable.

Jerry-Xin

The PR is repository-relevant, but two workflow lifecycle defects block merge.

🔴 Blocking

🔴 Critical — Team confirmation is unreachable. DeriveSummaryRoute returns SummaryRouteTeamWorkflow directly whenever participants and a requirement exist; it never returns SummaryRouteTeamConfirmation. Consequently, the handler branch that persists a proposal cannot execute, and team workflows bypass the documented confirmation step. See summary_route_policy.go and agent_summary_workspace.go. Route initial team requests to confirmation and add an end-to-end proposal/confirmation test.
🔴 Critical — Worker dispatch can be permanently lost after task creation. The task and idempotency binding commit first, then dispatch occurs in an untracked goroutine. Network errors and non-2xx responses are only logged; retries return an idempotent replay without a WorkerTrigger, so the pending task is never dispatched again. See agent_summary_workspace.go, agent_summary_workspace.go, and agent_summary_workspace.go. Use a transactional outbox or another durable retry mechanism, and treat non-success HTTP statuses as failures.

💬 Non-blocking

🟡 Warning — Handler tests could not link on this host because libtokenizers is unavailable. Service, agent, and database packages passed.

✅ Highlights

Strong space/user/session isolation for messages and cached handles.
Good optimistic-version and ownership checks around preview saving.
Terminal-response validation and transcript sanitization have substantial regression coverage.

yujiawei

Code Review — PR #234 (octo-smart-summary)

Reviewed at head 2c0dcf26255ee0468d67ef67ed1c950b96a5e9bc against merge-base eb9e0e3adabd03208b7d3695290d7bb5c034fae4 (96 files, +10818/-547).

Verification performed locally at this SHA: go build ./... clean, go vet clean, and the full go test ./... suite green (22 packages) — including the four CGO packages the PR description says could not link on the author's host. Those are fine here; libtokenizers links and internal/agent's //go:build cgo tests all pass. Two throwaway probe programs were used to confirm routing claims below and were deleted afterwards.

This is a large, genuinely careful PR. The idempotency store, the preview-save TOCTOU handling in agent_summary_save.go, and the legacy space_id='' namespace isolation in agent_message_repo.go are all well done. The problems below are concentrated in the routing layer and in what the diff does outside its stated scope.

1. Scope / specification compliance

Spec: ❌

The PR adds docs/unified-summary-safe-track.md as its own specification. Measured against that document, three things do not line up.

S1 — Missing: the mandated team-creation confirmation gate is unreachable

docs/unified-summary-safe-track.md:26-30 states the Agent entry point must "require a current server-side proposal version before team creation."

DeriveSummaryRoute never returns SummaryRouteTeamConfirmation:

// internal/service/summary_route_policy.go:104-116
case SummaryIntentGenerate:
    if in.HasOtherParticipants {
        if !in.HasSelectedTemplate && !in.HasRequirement {
            return SummaryRouteClarification
        }
        return SummaryRouteTeamWorkflow     // straight to task creation
    }

I swept DeriveSummaryRoute exhaustively (4 Action values × 4 Intent values × 2048 boolean flag combinations = 32,768 inputs). team_workflow_confirmation is returned zero times. Consequences:

internal/api/handler/agent_summary_workspace.go:301-302 (case service.SummaryRouteTeamConfirmation:) and completeWorkspaceProposal (:367-406) are dead.
No row ever gets pending_proposal_status = 'pending', so BeginProposalConfirmation (internal/api/handler/agent_workspace_store.go:685-691) always returns ErrWorkspaceProposalStale, and the new route POST /api/v1/agent/summary-sessions/:session/proposals/:version/confirm (internal/api/router/router.go:170) can only ever answer 409 / 40901.
state.PendingProposal in the client contract is never populated.

S2 — Over-build: the maximum time range was tripled on three endpoints this PR does not claim to touch

docs/unified-summary-safe-track.md:31 states: "The legacy endpoint still uses pipeline.DefaultTimeRangeDays (currently 31) for compatibility." It does not.

// merge-base internal/api/handler/task.go:339-377
maxDays := pipeline.DefaultTimeRangeDays      // 31 — used as BOTH default and ceiling
... service.ValidatePersonalWorkflow(..., maxDays)

// HEAD internal/api/handler/task.go:55 and :362
summaryWorkflow: service.NewSummaryWorkflowService(db, imDB, pipeline.MaxTimeRangeDays),  // 90

The author preserved the default (legacySummaryDefaultTimeRangeDays = 31, internal/service/summary_workflow.go:38) and even added a test for it, but the same constant was also the validation ceiling, and that half moved to 90. The widening reaches three surfaces outside the workspace:

Surface	Line	Before	After
`POST /api/v1/summaries`	`internal/api/handler/task.go:55`, `:362`	31	90
`POST /api/v1/bot/summaries`	`internal/api/handler/bot_summary_create.go:272-273`	31	90
Worker fetch pipeline	`internal/pipeline/fetch.go:917`	31	90

A request with a 60-day time_range that previously returned 400 / 40002 now creates a task and fetches 60 days of history. With MaxSafetyLimit = 100000 messages per channel, this triples the worst-case fetch and LLM spend per request, on a bot-facing API at a different trust tier. The only trace in the review surface is a one-line comment edit at internal/service/snapshot_validator.go:115. If this widening is an approved product decision, it belongs in its own PR with a note; either way the doc added by this PR is now wrong.

S3 — Over-build: a substantial unreachable subsystem ships as if it works

Beyond the proposal flow in S1, these have zero production call sites at HEAD:

newSummaryWorkspaceProposalToken — internal/api/handler/agent_summary_workspace.go:1623 (superseded by workspaceStoreToken, agent_workspace_store.go:916)
writeWorkspaceServiceError — agent_summary_workspace.go:1850
AgentWorkspaceStore.MarkPreviewSaved — agent_workspace_store.go:870 (superseded by markWorkspacePreviewSaved, agent_summary_save.go:249)
AgentWorkspaceStore.ValidateProposal — agent_workspace_store.go:770, an unlocked twin of the FOR UPDATE check at :685; referenced only by tests
hasExplicitWorkspaceRunIntent (:1064) and summaryWorkspaceRequirement (:1226) — referenced only by summary_workspace_contract_test.go, never by production. The tests give a false coverage signal for behavior the product does not execute.
SummaryRouteAgentPreviewSave (summary_route_policy.go:36) — returned by no reachable path and handled by no case in the consumer switch (agent_summary_workspace.go:296-317). If it were ever produced it would silently fall into default: and return a clarification instead of saving.

Leftover scaffolding with a stale comment:

// internal/api/handler/agent_summary_workspace.go:2123-2130
// ... the concrete endpoint methods are below once the persistence store has acquired/replay
// semantics available.
var (
    _ = fmt.Sprintf
    _ = middleware.GetUserID
    _ = agent.SummaryResultAgentPreview
)

S4 — Divergence: the doc added by this PR is already stale at HEAD

docs/unified-summary-safe-track.md:36-42 lists as "Deferred until the core Agent PRs stabilize": registry.go/runner.go terminal-tool support, agent_chat.go JSON/SSE/history contract changes, agent_message result metadata, preview/revision save enforcement, and team proposal persistence. All five land in this PR (commits 97d6624 and 2c0dcf2). The file was added at commit 05d39b8 and never updated.

2. Code quality

Quality: Changes-Requested

P1-1 — A team summary task is created, and other users are invited, from an ordinary chat sentence

Same code as S1, but the runtime consequence is independent of the doc. The risk gradient is inverted: the lower-impact personal workflow requires an explicit run intent; the higher-impact team workflow (creates a task and materializes invitations for other humans) does not.

// internal/service/summary_route_policy.go:116-119
        return SummaryRouteTeamWorkflow                       // no HasExplicitRunIntent
    }
    if in.HasExplicitRunIntent && in.HasSelectedSource && in.HasValidSource && in.HasSelectedTemplate {
        return SummaryRoutePersonalWorkflow                   // requires it
    }

Probe output at this SHA:

team route with NO explicit run intent = "team_workflow"
personal route with NO explicit run intent = "agent_preview"

HasRequirement is satisfied by any non-empty user message (summaryWorkspaceUserRequirement, agent_summary_workspace.go:1197-1209), and classifySummaryWorkspaceIntent defaults to Generate (:1042).

Failure scenario: a user has 3 participants selected in the workbench and types 行，就这样 (no ?, no negation, no explain keyword) → intent Generate → HasOtherParticipants && HasRequirement → SummaryRouteTeamWorkflow → completeTeamWorkspaceWorkflow (:471) creates the task and invites all three. No confirmation step, no execute affordance.

P1-2 — The `explanation` route bypasses source validation, and the two content-reading tools are space-blind

Two gaps compose into a space-boundary bypass.

(a) explanation returns before the source-validity guard:

// internal/service/summary_route_policy.go:84-92
if in.Intent == SummaryIntentExplain {
    return SummaryRouteExplanation      // returns BEFORE the guard below
}
if in.HasHardMissingData || (in.HasSelectedSource && !in.HasValidSource) || ... {
    return SummaryRouteClarification
}

Intent is keyword-classified from the user's own message; a trailing ？ is sufficient (agent_summary_workspace.go:1039-1041). SummaryRouteExplanation is dispatched to a full Agent turn with the complete workspace toolset (:303-304, internal/agent/profile.go:89-101).

(b) The request-scoped allowlist for that turn is built from raw client input, not from the validated set:

// internal/api/handler/agent_summary_workspace.go:573-592
for _, channel := range contextValue.SelectedChannels {     // client-supplied
    allowedChannels = append(allowedChannels, agent.ChannelScope{ChannelID: channel.ChatID, ...})
}
ctx = agent.WithAllowedChannelScope(ctx, allowedChannels)

validateSources (:1657) — the function that actually checks channel.SpaceID != spaceID — feeds only the routing boolean, never allowedChannels. And fetch_channel's independent accessibility check is space-blind:

// internal/agent/tool_fetch_channel.go:133-137
options := []pipeline.ChannelQueryOption{pipeline.WithIncludeArchived(req.IncludeArchived)}
...
accessibleChannels, err := pipeline.GetUserChannels(ctx, uid, imDB, options...)   // no WithSpaceID

Identical at internal/agent/tool_peek_channel.go:110-124. FilterChannelsForWorkspace — the whole point of the new internal/agent/workspace_discovery_scope.go — is wired into only the three discovery tools (tool_list_channels.go:70, tool_narrow_channels.go:57, tool_find_shared.go:62), never the two read tools.

Failure scenario: from the space-A workbench, POST profile=summary_workspace with selected_channels: [{chat_type:"group", chat_id:"<a group in space B>"}] and a message ending in ？. validateSources returns false and is ignored; the allowlist admits the channel; fetch_channel re-checks membership without a space filter and passes. Space-B content is summarized and persisted into space-A agent_message / evidence / summary_run rows.

Scope of the impact, stated precisely: GetUserChannels is still bounded by the caller's own IM memberships, so this is not a cross-user read and not a privilege escalation. It is a complete bypass of the space boundary this PR exists to add, for the caller's own data, with cross-space content becoming durable state in the wrong tenant's records. The DM variant is worse in kind: workspace_discovery_scope.go:64-71 exists specifically to require a DM peer be an active space_member, and the read path has no equivalent.

Suggested fix: build allowedChannels from the validateSources-approved set, and pass pipeline.WithSpaceID(agent.WorkspaceSpaceID(ctx)) inside fetch_channel / peek_channel. (WithSpaceID already exists and is used by the worker at internal/pipeline/fetch.go:941 — it is just not used by the agent tools.)

P1-3 — `AuthorizeDiscoveredChannels` replaces the allowlist instead of extending it, contradicting its own doc comment

// internal/agent/types.go:232-251
// AuthorizeDiscoveredChannels adds channels returned by a trusted discovery
// operation to an open request scope.
func AuthorizeDiscoveredChannels(...) bool {
    ...
    scope.replace(grants, uid)      // :250 — wipes, does not add
}

// internal/agent/types.go:200-208
func (s *mutableChannelScope) replace(channels []ChannelScope, uid string) {
    ...
    s.allowed = make(map[int]map[string]ChannelScope)   // everything previously granted is gone
    s.addLocked(channels, uid)
}

The accumulating add method (:177) is dead outside the constructor (:170) — grep confirms two call sites total.

Failure scenario: "总结项目相关群，以及我和张三的共同群" → narrow_channels_by_topic grants {G1,G2} (tool_narrow_channels.go:99) → find_shared_channels grants {G3} (tool_find_shared.go:87) and wipes G1,G2 → fetch_channel(G1) fails with ErrChannelOutsideSelectedScope, classified Retryable=false, Fatal=false (tool_error.go:80-84), so the model gets no repair path. The run silently produces a summary covering 1 of 3 requested channels. The same wipe also drops the UI-seeded initial scope passed at agent_summary_workspace.go:590.

No test distinguishes accumulate from replace: TestDiscoverableChannelScopeStartsClosedAndGrantsConfirmedChannels (channel_scope_test.go:40-64) calls it exactly once.

P1-4 — Deleting a completed summary permanently 500s the workspace session

// internal/api/handler/agent_summary_workspace.go:944
if snapshot.Session.WorkflowTaskID > 0 && snapshot.Session.WorkflowTerminalMessageID == 0 {
    ... ReconcileWorkflow ...     // skipped once a terminal message exists
}

// internal/api/handler/agent_summary_workspace.go:1915-1921
if snapshot.Session.WorkflowTaskID > 0 {
    if err := w.db.WithContext(ctx).
        Where("id = ? AND space_id = ? AND creator_id = ? AND deleted_at IS NULL", ...).
        Take(&task).Error; err != nil {
        return state, fmt.Errorf("load workspace workflow task: %w", err)
    }

A completed workflow leaves workflow_task_id = T and workflow_terminal_message_id > 0 (workspaceWorkflowTerminalState returns clearWorkflow = false for StatusCompleted, :1000-1001). The creator then soft-deletes T through the normal product path, DELETE /api/v1/summaries/:id (internal/api/router/router.go:117 → task.go:1889).

From then on the reconcile branch is skipped (terminal id is non-zero) and stateFromSnapshot cannot find the row → GET /api/v1/agent/chat/history?profile=summary_workspace returns 500 on every call, and turnFromSnapshot makes every subsequent chat turn in that session return 500 too. Note the StatusFailed / StatusCancelled paths do set clearWorkflow = true (:1003-1005), so only the successful path is affected.

P1-5 — The whole session transcript is re-read, unbounded, inside every locked transaction

// internal/api/handler/agent_workspace_store.go:577-579
if err := db.Where("space_id = ? AND user_id = ? AND session_id = ?", ...).
    Order("id ASC").Find(&snapshot.Messages).Error; err != nil {

No LIMIT, no scope_version filter. loadWorkspaceSnapshotTx is called from BeginTurn (three sites), CompleteTurn, BeginProposalConfirmation (three sites), ReconcileWorkflow, and MarkPreviewSaved — all inside SELECT ... FOR UPDATE transactions.

What accumulates is not just chat turns: workspacePersistAgentMessages (agent_summary_workspace.go:541-560) persists every message from the run, including assistant tool-call turns and tool results — which for fetch_channel are channel message dumps. Compare LoadHistory on the same struct, which does cap (Limit(maxHistoryRows) = 200, agent_workspace_store.go:606), and historyFromSnapshot, which returns all of them to the client with no pagination (:2081-2114).

A long-lived session therefore makes every turn read more rows than the last, while holding row locks. Suggest a LIMIT + cursor on the snapshot read, or splitting the "state" read from the "transcript" read.

P1-6 — An idempotent replay cannot recover a dropped worker dispatch

// internal/service/summary_workflow.go:587-592  (replay path)
result := CreateSummaryWorkflowResult{
    Task: task, Target: in.target, Inferred: in.inferred, Replayed: true,
}                                     // WorkerTrigger is nil

Dispatch is a fire-and-forget goroutine built after commit, with no retry and errors only logged (internal/api/handler/task.go:391-393, :1871-1886; agent_summary_workspace.go:429, :504).

Failure scenario: the transaction commits, then the worker POST fails (worker restarting, 5xx, or the process is killed before the goroutine runs). The task sits at StatusPending. The client retries with the same Idempotency-Key → 200 OK, Replayed: true, WorkerTrigger == nil → still never dispatched. Before this PR the retry created a fresh task that did dispatch, so the user had a self-service workaround; the PR removes it without adding a reconciler or sweeper. No test covers "replay after a dropped dispatch".

P1-7 — No length cap on any `summary_context` string, and no request-body limit on the route

// internal/api/handler/summary_workspace_contract.go:174-179
channel.ChatID = strings.TrimSpace(channel.ChatID)
channel.Name   = strings.TrimSpace(channel.Name)
if channel.ChatID == "" || channel.Name == "" { return out, ... }

Emptiness is the only check. Uncapped: chat_id, channel name, participant.user_id, participant.user_name, template.template_id, template.label, template.requirement, time_range.label. Element counts are capped (50 / 100 / 20), payload size is not.

This is a regression against the path it supersedes — internal/api/handler/agent_chat.go:318-325 truncates the same fields (truncateRunes(..., 512) / (..., 200)). And unlike bot_summary_create.go:137 (io.LimitReader(body, 1<<20)), the agent chat routes use a bare c.ShouldBindJSON (agent_chat.go:589, :886). message is capped at maxMessageLen runes (agent_summary_workspace.go:151); summary_context is not.

Failure scenario: an authenticated user posts a summary_context with 50 channels × 10 MB names plus a 50 MB template.requirement. It is json.Marshaled and SHA-256'd on every turn (marshalSummaryWorkspaceContext, :254), written to scope_json (a MySQL JSON column — exceeding max_allowed_packet yields a 500, not a 400), folded into the LLM prompt via summaryWorkspaceExecutionRequirement (:1215), and echoed back on every subsequent turn and in History. Single-request cost amplification with no cap in the chain. Note a gateway client_max_body_size would mitigate this; I could not verify the deployment config, so please confirm.

P1-8 — Workspace state has no retention path, and the new evidence guard depends on a retirement step that does not exist

Three facts that only bite in combination:

(a) expires_at on agent_summary_session is declared and indexed (migrations/sql/20260827-01-agent-summary-workspace.sql:46, :53) but written by nothing. Its only other occurrence in the tree is the struct field at internal/model/agent_summary_workspace.go:41. Every row will have expires_at = NULL forever and idx_agent_summary_session_expires indexes an all-NULL column.

(b) There is no DELETE against agent_summary_session, agent_summary_turn, or summary_workflow_idempotency anywhere in the codebase, and no lease reaper (idx_agent_summary_turn_lease (status, lease_expires_at), :78, has no reader — expiry is evaluated in Go on an already-loaded row).

(c) The one retention job that did exist was narrowed to exclude workspace rows:

-- internal/api/handler/agent_session_cleanup.go:96-105
DELETE FROM agent_message
WHERE space_id = ''
  AND (user_id, session_id) IN ( SELECT ... FROM agent_message WHERE space_id = '' ... )

with the justification in the doc comment: "summary workspace 由 agent_summary_session 管理生命周期，不在这里清理" (:16). That lifecycle management is (a) and (b) — it does not exist.

The sharpest form is the new evidence guard:

-- internal/api/handler/agent_session_cleanup.go:152-157
  AND NOT EXISTS (
    SELECT 1 FROM agent_summary_session
    WHERE agent_summary_session.user_id = agent_message_evidence.user_id
      AND agent_summary_session.agent_session_id = agent_message_evidence.session_id
  )

Its own comment states the precondition: "workspace retirement must remove that session before this Legacy cleanup may collect its evidence" (:137-138). Nothing retires a session. So every workspace session permanently pins its agent_message_evidence rows — MEDIUMTEXT JSON snapshots of full message batches (migrations/sql/20260717-01-add-agent-message-evidence.sql:11). Net effect: agent_summary_session, agent_summary_turn, summary_workflow_idempotency, all space_id <> '' rows in agent_message, and the evidence rows they pin all grow monotonically for the life of the deployment. The expires_at column and its index are the shape of a retention design with no implementation behind it.

Either land the reaper in this PR or make the guard time-bounded rather than existence-bounded.

P1-9 — Migration `20260827-01` is a full-table COPY rebuild of `agent_message`, unannotated

-- migrations/sql/20260827-01-agent-summary-workspace.sql:4-16
ALTER TABLE `agent_message`
    MODIFY COLUMN `session_id` VARCHAR(128) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_bin NOT NULL,
    ADD COLUMN `space_id` VARCHAR(64) NOT NULL DEFAULT '' AFTER `id`,
    ... 8 more ADD COLUMN ... AFTER ...
    ADD KEY `idx_agent_message_workspace_created` (...),
    ADD KEY `idx_agent_message_turn` (`turn_id`);

session_id participates in two existing indexes (idx_user_session_created, idx_session_created, from 20260707-01-add-agent-message.sql:23-25). Per MySQL's online DDL limitations, a collation change on an indexed VARCHAR cannot go INPLACE and falls back to ALGORITHM=COPY — Permits Concurrent DML = No. INSTANT also cannot be combined in one ALTER with actions that do not support it, so the whole statement degrades to the slowest member. No ALGORITHM= / LOCK= is specified, so there is no guard rail and no failure signal.

agent_message is the multi-turn chat log (MEDIUMTEXT content + JSON tool_calls) — the hottest write target in this schema. During the rebuild every AppendMessages blocks. Compounding it, internal/db/migrate.go:15-16, 33-41 takes GET_LOCK('smart_summary_migration', 30) and cmd/summary-api/main.go:62-64 / cmd/summary-worker/main.go:71-73 log.Fatalf on failure — so on a rolling deploy, replicas that cannot get the lock within 30 s crash-loop for the entire duration of the rebuild.

The 30 s lock + Fatalf is pre-existing, and 20260814-01-summary-source-unique-key.sql:16 already introduced utf8mb4_0900_bin (so MySQL 8.0 is an established assumption, not a new one). But 20260814-01 applied it to summary_source and kept the collation change in its own statement; this is the first migration in the repo that can plausibly run for minutes against a hot table. Please size agent_message and state the maintenance window — see §3.

Two related points on the same file:

The Down block correctly restores utf8mb4_unicode_ci (verified against the original DDL, where session_id had no explicit collation and inherited the table default at 20260707-01:25). But Down is unconditionally data-destroying — it drops both new tables and nine agent_message columns including response_payload_json and saved_task_id — and internal/db/migrate.go:52 only ever calls migrate.Up, so no code path can execute it. Worth a comment so nobody treats it as a rollback safety net.
The file contains three independent statements (one ALTER, two CREATE TABLE) and MySQL auto-commits DDL, so sql-migrate's transaction wrapper is a no-op. If the second CREATE fails, the first two are committed but the gorp_migrations row is not written → next boot replays the file → ERROR 1060 Duplicate column name 'space_id' → log.Fatalf → permanent crash-loop needing manual DBA repair. Neither CREATE TABLE uses IF NOT EXISTS. This is house style (20260728-01:19, 20260810-01:46 do the same), but this is the first file combining a long ALTER with two CREATEs.

P1-10 — Max time range 31 → 90 on three out-of-scope surfaces

See S2. Listed here as well because the resource impact is real independent of the doc question.

P2 (non-blocking)

Concurrency / correctness

Proposal-confirm has a lock gap — BeginProposalConfirmation validates the proposal under FOR UPDATE (agent_workspace_store.go:685-691) but commits without bumping pending_proposal_version or flipping the status; CompleteTurn (:365-375) re-checks only the turn lease and scope version. Serialization then rests entirely on the 6-minute active_turn_id lease (agent_summary_workspace.go:28). If a confirm stalls past the lease, a second confirm with a different Idempotency-Key re-validates the same still-pending proposal and creates a second team task. Currently unreachable because no proposal is ever created (S1) — but it becomes live the moment the confirmation route is wired up, so please fix it together with S1. Remedy: bump the version (or set status='confirming') inside the validating transaction.
Idempotency-key namespace is shared across entry points — summary_workflow.go:301-309 drops the time range from the hash when it is absent, and :673-680 collapses both defaults to the token "default", but the agent default is 7 days and the legacy default is 31. Both write to the same (space_id, user_id, idempotency_key) row, and the workspace key is client-supplied (agent_summary_workspace.go:423). Reusing a key across POST /summaries and a workspace turn replays the wrong task instead of returning 40009.
Deleted-summary tombstone bricks the key — summary_workflow.go:573-584 returns reason: "deleted_summary", recovery_action: "start_new_summary" forever; the binding row is never repaired. A client that derives the key deterministically from the request payload can never comply. TestSummaryWorkflowDeletedTaskKeepsIdempotencyTombstone enshrines this as intended, so flag if it is.
INSERT IGNORE + SELECT ... FOR UPDATE inside a long write transaction — createSummaryWorkflowIdempotencyBinding (summary_workflow.go:600-615) runs after 4+ inserts in the same tx, and the S→X lock upgrade against an uncommitted winner is the classic InnoDB lock-wait/deadlock shape. Also, the method doc at :187-188 claims "never performs network I/O" while :463 queries the IM database and :474/:504 call the swappable ResolveUserName resolver.
json.Marshal errors swallowed on security-relevant fingerprints — summary_workspace_contract.go:268 (payload, _ := in summaryWorkspaceRequestHash) and agent_workspace_store.go:830. A marshal failure yields the SHA-256 of null, collapsing distinct requests onto one hash.
time.Now() where the project mandates timezone.Now() — internal/timezone/timezone.go's package doc is explicit ("All 'current time' reads must go through Now()", citing a prior 8-hour scheduler skew), and the handler package uses timezone.Now() in 27 places. New code uses raw time.Now() at agent_workspace_store.go:158, 407, 517, 538, 650, 841, 852, 905, agent_summary_save.go:276, and agent_summary_workspace.go:1262. The last one is consequential: it feeds the default 7-day fetch window at :1297. Note agent_summary.go:539 correctly uses timezone.Now(), so the two clocks are mixed inside one save transaction.

Agent loop

Free-text rejection breaks role alternation — runner.go:255-266 appends a user nudge without first appending the offending assistant turn, producing two consecutive user messages. The sibling block immediately below (:277-290) appends Message{Role: "assistant", Content: ""} for exactly this reason. Match the established pattern.
turn.Truncated is never recorded on the terminal path — runner.go:300-303 records it for the free-text final; the terminal branch (:330-372) does not, yet still reads the tracker at :364. Every workspace preview loses the truncation disclosure.
Terminal tool advertises 4 unreachable result types — EmitSummaryResponseTool publishes all 8 result types plus workflow/confirmation payloads (tool_emit_summary_response.go:117-167), but the only production caller's allowlist is always a subset of {agent_preview, agent_revision, explanation, clarification} (agent_summary_workspace.go:635-645). When the model picks one the schema invites, it burns a full LLM step on result_type %q is not allowed; at MaxSteps-1 that becomes a hard 500 (runner.go:355-357). Narrow the schema per request from the same trusted allowlist.
narrow_channels_by_topic drops the archived-thread bridge — tool_narrow_channels.go:53 calls GetUserChannels(ctx, uid, imDB) with no options, unlike the other four channel tools. Archived threads never reach candidates, so under an open scope they can never be authorized.
commit_scope=true grants the entire visible surface — tool_list_channels.go:75-79. Combined with P1-3's replace semantics and an agent that reads untrusted message bodies, a single model-authored boolean is a wide grant whose only defense is a prompt sentence (prompts/summary_workspace.md:5).
AuthorizeDiscoveredChannels's return value is discarded by tool_narrow_channels.go:99 and tool_find_shared.go:87; only list_channels surfaces scope_committed.
sanitizeToolProtocolHistory silently drops the whole transcript when no user role is present (history.go:213-222 → return nil), with no log. Applied to every terminal-policy run (runner.go:139-141).
preview.effective_scope overstates provenance under open scope — agent_summary_workspace.go:679-681 reports everything granted, not everything fetched.
No guardrail couples profile.Tools to Policy.TerminalTool — if a profile registers a terminal tool without naming it in the policy, terminalOnly is always false and every call gets "terminal tool must be the only tool call in its step" (runner.go:560-565), which is untrue and unrecoverable. Only summary_workspace uses this today, so it is latent.

Handlers / data

ScopeJSON is a bare string against a nullable JSON column — internal/model/agent_summary_workspace.go:19 (ScopeJSON string vs scope_json JSON NULL), while every sibling nullable JSON field is *string (PendingProposalJSON:30, AgentSummaryTurn.ResponseJSON:65, AgentMessage.ResponsePayload:30). A GORM Save with ScopeJSON == "" emits scope_json = '', which MySQL rejects with ERROR 3140: Invalid JSON text: The document is empty. Currently unreachable — the only INSERT path (agent_workspace_store.go:333) is gated by validateWorkspaceBegin:301, 304 — but it is one careless tx.Save(&session) away, and on SQLite (where JSON is just TEXT) every test in this PR would still pass. The read side already treats "" as the NULL stand-in (agent_summary_workspace.go:1863, agent_summary_save.go:189), which is exactly what *string would express in the type system.
Three declared indexes with zero readers — idx_agent_summary_turn_session (session_id) (20260827-01:77; nothing queries the turn table by bare session_id, and it is not a left-prefix of uk_agent_summary_turn_request), idx_agent_summary_turn_lease (:78), and idx_agent_summary_session_expires (:53). They cost write amplification on every turn insert. idx_agent_summary_turn_session is worth dropping or extending to (space_id, user_id, session_id, status) specifically because a bare-session_id index is the affordance that makes a future ad-hoc admin query look fast without a tenant predicate.
scope_version is filtered outside the index — agent_workspace_store.go:605. The 3-column prefix still bounds the scan to one session, so this is amplification rather than a full scan, but LIMIT 200 is applied after the filter, so a session with many superseded scope versions walks far past 200 index entries.
Collation mix in the new evidence guard survives only via an obscure rule — agent_session_cleanup.go:156 joins agent_summary_session.agent_session_id (utf8mb4_0900_bin) against agent_message_evidence.session_id (utf8mb4_unicode_ci, table default from 20260717-01:9). I expected ERROR 1267 and checked: it does not error, because both operands have coercibility 2 in the same charset and MySQL's third coercibility sub-rule says _bin wins. Values are prefixed hex hashes so the _ci → _bin shift is inert, and the inner index stays usable. But flip agent_session_id to any _ci collation and this becomes ERROR 1267 on every tick — which per :145-147 returns early and silently stops the entire evidence cleanup. Add an explicit COLLATE or a comment.
agent_session_id length comment is wrong — summary_workspace_identity.go:16 says "below varchar(128)"; the column is VARCHAR(80) (20260827-01:23) and the output is "summaryws:" (10) + 64 hex = 74 chars. Six bytes of headroom against a bound the comment misstates; lengthen the prefix and you get ERROR 1406 in strict mode.
uniqueIndex without priority: — internal/model/summary_workflow_idempotency.go:11-13. GORM falls back to struct field order, which happens to match 20260826-01:17. Reorder the struct and the test schema's index silently diverges from MySQL's.
Preamble stripping is now skipped for the new result types — stripAgentPreamble's only non-test call site is gated by if !workspaceSave (agent_summary.go:482-488). For agent_preview/agent_revision the content is unstripped on the JSON response, the SSE frame, and the persisted row. The rationale (the deliverable now arrives as a structured tool argument) is reasonable, but preview.content is still LLM text and the preamble now persists into the saved SummaryTask. Defense-in-depth removed with no compensating check.
The save idempotency hash covers fields the server then discards — agent_summary.go:209 hashes req including Sources/Participants/OriginChannelID, then :245 overwrites all of them from the persisted scope. Two retries of the same logical save with a cosmetically different (and ignored) sources array hash differently → spurious conflict instead of replay.
Legacy save ignores the request space — agent_summary_save.go:330-345 pins space_id = "" but the resulting SummaryTask is created with the middleware spaceID (agent_summary.go:557), so a legacy agent session generated in space A can be materialized as a summary in space B. Same user throughout, so no escalation, but it is an unaudited cross-tenant data-movement path.
Variadic optional tenant filter — buildSnapshotV1(..., workspaceSpaceID ...string) (agent_summary.go:926-936) defaults a security-relevant predicate to the space_id='' namespace. Any future caller that forgets the argument gets it silently, with no compile error. Make it required.
Unreachable guard — agent_summary.go:~440, strings.TrimSpace(draftMsg.SpaceID) != "" can never be true; loadAgentMessageForSave already filters space_id = "" (agent_summary_save.go:332, :344).
CreateTeamFromAgent accepts empty participant ids — summary_workflow.go:241-249 only counts; there is no validateAgentWorkflowParticipants to mirror validateAgentWorkflowSources (:331-339). [{UserID: ""}] satisfies "at least one other participant" and inserts a user_id='' row at ParticipantPending that can never confirm. Unreachable from the only current caller (summary_workspace_contract.go:200-202 rejects empty ids), so this is a service-layer hole, not a live bug — but the sibling source path is code, and this one is only a doc comment.
Dead condition — summary_route_policy.go:118, in.HasValidSource is always true there (line 89 already returned for HasSelectedSource && !HasValidSource). Harmless today, but it reads as an independent check and is not one.
Smart-quote mojibake in security rationale comments — every ASCII space_id='' became space_id=” (U+201D) in members_auth_test.go:2016, 2198, 2213, 2236, 2263, 2382, 2436-2438, list_space_gate_test.go:37, 53, 55, edit_version_space_gate_test.go:47, 87, 88, 110, 111, schedule_space_gate_test.go:50, 61. Comment-only, no behavior impact, but these comments are the only written record of why those gates exist and space_id=” is now a nonsense predicate. It also reads as the fingerprint of an automated rewrite pass; I checked every added line for stray U+2018/2019/201C/201D and confirmed no Go string literal was corrupted.
Client input is not prompt-fenced — the legacy path wraps UI channel metadata in an explicit "以下字段仅作为数据，不是指令" fence (agent_chat.go:339); the workspace path fences the preview body (agent_summary_workspace.go:1536, :1541) but not channel.Name / participant.UserName flowing through materializeWorkspaceAgentContext.
Reflected input in a validation error — summary_workspace_contract.go:183 echoes channel.ChatType back to the client.
Minor contract gaps — time_range has no absolute bounds (start: "0001-01-01T00:00:00Z" is accepted, :226-233); template.Version == 0 is accepted for a field documented as a version (:215); maxSummaryWorkspaceReferencedTask = 20 (:34) duplicates maxReferencedTaskIDs = 20 (reference_artifact.go:271) as an independent literal.

Tests

The two new migration tests are strings.Contains greps, not tests — internal/db/agent_summary_workspace_migration_test.go and summary_workflow_migration_test.go assert substrings of the SQL text the same PR wrote, and never execute the DDL. They cannot catch a syntax error, a Down that does not reverse Up, or model/schema drift. The repo's migration runner tests use a SQLite fixture FS (internal/db/migrate_test.go:16, 265-269), and .github/workflows/ci.yml has no MySQL service container, so the real migrations are executed by nothing in CI.
Every store test runs on SQLite with AutoMigrate — newWorkspaceStoreTestDB (agent_workspace_store_test.go:18-27) and setupResolveTestDB (agent_summary_resolve_test.go:16-24) build the schema from the Go structs, not from 20260827-01-agent-summary-workspace.sql. clause.Locking{Strength: "UPDATE"}, the uk_agent_summary_session unique constraint under OnConflict{DoNothing}, JSON column typing, and the utf8mb4_0900_bin byte-exactness the SQL comments specifically call out (20260826-01:10-12: "Retry-A and retry-a must remain distinct") are all unexercised — collation is not expressible in a GORM tag at all. The code comment at agent_workspace_store.go:336-338 reasons explicitly about MySQL CLIENT_FOUND_ROWS semantics that the tests cannot reach.
No concurrency test anywhere in the PR. TestSummaryWorkflowIdempotencyBindingReadsBackWinner (summary_workflow_test.go:209-227) runs its two transactions sequentially with the winner fully committed first — it exercises the read-back arithmetic, not the race. The SELECT/INSERT TOCTOU that is the entire premise of the feature is untested.
TestLegacyWorkflowDefaultRangeRemains31DaysWhenMaximumIs90Days (:72-84) tests the half that did not break — it wires NewSummaryWorkflowService(db, nil, 90), asserts the default stayed 31, and never asserts the ceiling did. S2 lives in exactly that gap.
task_workflow_service_test.go:59-101 cannot detect a double dispatch — it constructs NewTaskHandler(db, imDB, ""), so triggerWorker returns before doing anything. No-double-dispatch is the headline claim of the feature.
summary_route_policy_test.go:119-127 is a tautology — {Intent: Generate, HasValidSource: true, HasOtherParticipants: true} with both HasRequirement and HasSelectedTemplate false returns Clarification via policy.go:113 regardless. Delete the (HasOtherParticipants && !ParticipantsValid) guard at policy.go:90 and the test named "invalid participant blocks side effects" still passes. (The source-guard case at :109-118 is a real test — the contrast is instructive.)
FilterChannelsForWorkspace has zero direct tests, and every indirect test runs with WorkspaceSpaceID(ctx) == "", which hits the early return at workspace_discovery_scope.go:17-20. TestListChannelsCommitScopeControlsOpenScopeAuthorization (tool_channel_access_cgo_test.go:344-393) seeds space_id='test-space' rows but never calls WithWorkspaceSpaceID, so the group/thread space check, the space_member DM-peer query, the fail-closed default: arm, and dmPeerID are executed by no test. The PR's headline safeguard is untested.
TestChannelReadToolsAcceptLogicalDMID (tool_channel_access_cgo_test.go:257-277) passes on any unexpected error — the assertion is if err != nil && (strings.Contains(...) || ...), so a missing session_id, a nil-DB failure, or a silent zero-message success all pass.
TestGetProfile_SummaryWorkspace asserts len(reg.Schemas()) != 12 (profile_test.go:186-188) — a bare count that passes if any tool is swapped for another, and it does not assert Policy.TerminalTool names a tool present in profile.Tools.
TestTerminalMixedWithOrdinaryToolIsRejectedAndRetried (runner_terminal_test.go:81-135) is genuinely good — it verifies the rejection is transient-only and visible to the next planner turn. Noting it because it is the exception in this diff.

Findings raised during review and rejected after verification

Recorded so they are not re-raised:

"The mixed-terminal rejection in runToolBatch skips tool_end, hanging the SSE stream." False. tool_end is emitted at runner.go:601-611, before the if err != nil { if terminalRejected { return } } branch at :616-619. No event is lost.
"The worker now fails open on space_id='', inverting the space gates." Not a regression. Before this PR channelScopeOpts was nil unless ChannelScopeEnabled, so Layer 1 had no space filter at all. This PR adds one (worker/processor.go:787-790, personal_processor.go:724-727 → pipeline/fetch.go:940). Legacy space_id='' rows remain unscoped, which is unchanged behavior, not new. Worth a follow-up (backfill or an explicit refusal), not a blocker here.
"utf8mb4_0900_bin is an undeclared MySQL 8.0 requirement." Already established on main by migrations/sql/20260814-01-summary-source-unique-key.sql:16. Not introduced here.
"The four modified space-gate tests were weakened to accommodate a behavior change." Checked line by line: edit_version_space_gate_test.go, list_space_gate_test.go, schedule_space_gate_test.go, and members_auth_test.go contain only comment and gofmt churn. Zero assertions, seeds, expected status codes, or assertCrossSpace404 calls were touched.
"pipeline.WithSpaceID has no production callers." It has one: internal/pipeline/fetch.go:941 (the worker path). It is the agent tools that do not use it — see P1-2.
"The new /summary-workbench/capabilities route is unauthenticated." It is registered on the v1 group, which applies StrictAuthMiddleware + StrictSpaceMiddleware at router.go:76-77. Registering it outside the { } block reads as ungrouped, which is stylistically misleading, but gin applies group middleware regardless.
snapshot_scope_input.go deletion. Not a dropped validation: channelIDsFromSources had exactly one caller and is replaced byte-equivalently by workflowChannelIDs (summary_workflow.go:617-625), still feeding ValidatePersonalWorkflow.
agent_content_strip.go is not PII stripping. It removes LLM conversational preamble before the first markdown heading. The diff to it is gofmt-only.

3. What I would like a human to verify before merge

This PR carries the needs-human-review label. Specifically:

Was the 31 → 90 day widening (S2) an approved product decision? If yes, it should be its own PR with a changelog note, and the doc line must be corrected. If no, revert it on the legacy and bot endpoints.
Was the proposal/confirmation flow (S1) meant to ship wired up, or meant to be dark? If dark, please say so in the PR description and add a // not yet wired marker — right now it reads as a working feature and reviewers will assume the team-creation gate exists.
Migration 20260827-01's ALTER TABLE agent_message (P1-9). Please confirm the current agent_message row count and the maintenance window, and check whether octo-deployment runs migrations as a pre-deploy Job rather than at process start — if it does, the crash-loop half of P1-9 is already neutralized and only the write-blocking half remains. No test in this PR executes this DDL against any database; CI has no MySQL service container, so nothing verifies it at all.
Is there a gateway-level request body limit in front of /api/v1/agent/chat? That determines whether P1-7 is a real DoS or merely a missing defense-in-depth.
What is the intended retention policy for workspace sessions (P1-8)? The schema was designed for one (expires_at + its index) and the evidence cleanup guard explicitly depends on one existing.

4. Additional observations outside the review scope

Legacy space_id='' isolation is done well. agent_message_repo.go pins the legacy namespace on both read and write, existing rows default to '', and TestAgentMessageRepoLegacySpaceIsolation / TestLoadAgentMessageForSaveLegacySpaceIsolation exercise the real repository against a real DB. I ran both; they pass and they test what they claim. No backfill is required.
The preview-save TOCTOU handling is the strongest part of the PR. The unlocked preflight at agent_summary.go:232 is advisory; inside the transaction :590 re-runs loadWorkspacePreviewForSave with FOR UPDATE on both rows, and the write uses conditional WHERE ... saved_task_id = 0 with RowsAffected != 1 → error. errWorkspacePreviewSaveStale also collapses all staleness reasons into one 409 with no message-id oracle. Correct, and worth preserving through any refactor prompted by this review.
The summary_workspace legacy profile=summary endpoint remains reachable with no space scoping (pre-existing, not a regression). The workspace's space isolation is only as strong as that endpoint being unavailable to the same clients.

Coverage note — what this review did not verify

Stated so the gaps are not mistaken for clean bills of health:

No MySQL instance was available. Everything about the migration (P1-9), the collation coercibility rule in the evidence-cleanup join, the INSERT IGNORE + SELECT ... FOR UPDATE lock-upgrade shape in createSummaryWorkflowIdempotencyBinding, and Retry-A vs retry-a byte-exactness is derived from the MySQL 8.0 documentation and from reading the DDL — not measured. Running ALTER ... ALGORITHM=INSTANT then ALGORITHM=INPLACE against a real 8.0 instance would settle P1-9 definitively, and the severity there is a direct function of the production agent_message row count, which I do not have.
No end-to-end run. The server was not started. P1-1, P1-2, P1-4 and P1-6 were traced statically through the handler → routing → tool call chain and, where possible, confirmed with throwaway unit probes against the real functions; none was reproduced against a live stack. P1-2 in particular assumes no upstream middleware rejects a selected_channels entry whose space differs from the request's — I checked the two middlewares on agentGroup and found none, but did not audit the full chain.
octo-deployment was not consulted. If it runs migrations as a pre-deploy Job rather than at process start, the crash-loop half of P1-9 is already neutralized. This is the single highest-value unchecked item.
Concurrency was reasoned about, not exercised. There is no MySQL, so the lock ordering between the two SELECT ... FOR UPDATE sites in agent_workspace_store.go (:312, :339) and any deadlock potential against summary_task are unexamined by both this review and the PR's own tests.
Blast radius of P1-2 beyond the requesting user. I confirmed cross-space content lands in space-A agent_message / evidence / summary_run rows, which are read user-scoped. I did not exhaustively trace whether any saved workspace artifact becomes readable by other space-A members. If one does, P1-2 escalates from "own data in the wrong tenant's records" to a genuine cross-user leak.
Prompt-level defenses (prompts/summary_workspace.md) were read but their actual effect on model behavior — especially the commit_scope discipline — is not verifiable by inspection.

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — MR !15 (octo-card-forge)

Reviewer: Octo-Q (automated review)
Head da3d9138789595d1b15a67792cd345e858b4dcf1 · 比较基 75e9dca12ed116a04cb02d04bb27efeb5483262b（two-dot）· 轮次 1 · 4 文件 / 220 行

Summary

本 MR 把 deploy-files 更新路径从「CI 创建部署 MR、评审后合并」改为「CI 直推生产分支」：sync_code / sync_catalog 不再创建 deploy/* 源分支与 merge_request.* push options，改为直接 git push "$DEPLOY_REPO_URL" "HEAD:$TARGET_BRANCH"；手动 argocd_sync 成为唯一生产上线闸门，其镜像由 $GIT_IMAGE 换为 $CURL_IMAGE，并移除了同步前对 deploy-files 的 clone + digest grep 校验。README 与部署文档同步改写，测试新增对应断言。这是一次有意的审批边界收敛（文档明确「人工 argocd_sync 作为唯一的生产上线审批边界」），审查重点放在该改动新引入的运行时契约：并发写同一分支、配置完整性、以及人工闸门处机器校验的缺失。

Verification

Static analysis only at head da3d9138; build and tests not executed in this environment.

✅ Diff 完整性 — 内嵌 two-dot diff 与 MR 头统计精确一致（4 文件 / 220 行，逐行计数吻合）；以下结论全部基于该完整 diff。
⚠️ 仓库上下文不可验证 — 审查方无法访问源仓库（GitLab 需认证、工作区 GitHub token 失效、该仓库未在 Octo-Q 工作区注册），因此 .gitlab-ci.yml 全文（variables: 定义、job DAG/needs:、deploy-files 分支保护）无法核对；凡涉及 diff 之外上下文的结论均显式标注「需确认」。
✅ 测试断言与 diff 交叉核对 — tests/gitlab-deployment.test.ts:92 的正则与两条新 push 行逐字匹配，toHaveLength(2) 与 diff 一致；not.toContain('SOURCE_BRANCH="deploy/')、not.toContain("merge_request.create") 均与 diff 一致。

Findings

一个 P1（需作者举证或修复），两个 P2。

P1 — 两个作业直推同一生产分支，可见 diff 内无串行化或 rebase-retry，引入新的 push 冲突失败模式（`.gitlab-ci.yml:281`、`:312`）

diff-scope 三问：(1) 本 PR 新引入——旧设计各作业推各自的 deploy/${APP_NAME}-*-${CI_JOB_ID} 分支、MR 串行合并，物理上不会互相冲突；(2) 本 PR 正是修改了该路径的行为契约（deploy-files 如何被更新）；(3) 非 pre-existing。
sync_code 与 sync_catalog 现在都对同一 $APP_NAME-prod 分支执行 git push "$DEPLOY_REPO_URL" "HEAD:$TARGET_BRANCH"。若两个作业可并发——同一 bootstrap 流水线内并行，或两次发布时间重叠的流水线——后推者将以 non-fast-forward 被拒：作业失败、deploy-files 只被部分更新（Forge 与 Catalog digest 未落在同一提交），需人工重跑。旧世界同样场景会成功（双分支 + 顺序合并）。可见 diff 未新增任何 needs: / resource_group / stage 排序，也没有 git pull --rebase + 重试包裹这两处 push。
升级判据：若能举证 bootstrap 流水线中两个作业已串行（指出相应 needs:/resource_group/stage 行），此条即解除；否则按「本来能工作的并发/连续发布路径在生产中变得不可用」成立。
修复方向（二选一）：为两个作业加 resource_group: deploy-files 互斥；或将 push 步骤改为有界重试的 git pull --rebase origin "$TARGET_BRANCH" && git push "$DEPLOY_REPO_URL" "HEAD:$TARGET_BRANCH"。

P2 — argocd_sync 移除同步前唯一机器校验，人工闸门从此无交叉核对（`.gitlab-ci.yml:331`，旧 346–350 行被删）

diff-scope 三问：校验逻辑由本 PR 删除（new）；本 PR 同时移除 MR 评审闸门，二者叠加使「CI 写入 deploy-files」与「人工点同步」之间不存在任何校验（amplified）；argocd_sync 作业本身 pre-existing 但其行为契约被本 PR 修改。
被删除的是：clone $APP_NAME-prod 分支 + 对 manifests/deploy.yaml 做 grep -Fq "image: \"${CONTAINER_IMAGE}@${IMAGE_DIGEST}\"" / @${CATALOG_IMAGE_DIGEST} 校验——这是把「操作者预期的 digest」与「分支真实内容」绑定的最后一道机器核对。删除后，若先前 sync 作业失败、或分支被更晚的流水线改写，操作者将在无任何信号的情况下同步陈旧/部分状态。该删除是有意且已用测试固化（tests/gitlab-deployment.test.ts:96-97 断言 argocd 作业无 git clone、无 CODE_TOKEN），文档亦声明人工为唯一边界——故定 P2 而非 P1。
建议：在 curl sync 前至少打印 deploy-files 分支 HEAD commit 与 manifests/deploy.yaml 的实际 image 行，让人工闸门有可核对的内容；无需重新引入 CODE_TOKEN（可经流水线变量或作业上下文传入）。

P2 — `image: $CURL_IMAGE` 的定义不在 diff 内，需确认（`.gitlab-ci.yml:323`）

diff-scope 三问：本 PR 将 $GIT_IMAGE 改为 $CURL_IMAGE（new）。$GIT_IMAGE 理应在 variables: 块已有定义，但 $CURL_IMAGE 的定义不在本 diff 可见范围内。若未定义，image: 展开为空 → 作业配置非法 → 流水线创建失败（若真未定义则为 P1）。MR 流水线理应已暴露该问题，但本评审无法访问 GitLab CI 佐证。
建议：指出 CURL_IMAGE 在 variables: 中的定义行；若尚未定义则补上。

Human-verify

以下事项超出 diff 可见范围，本环境无法核验，请逐项确认（在确认前，本评审按 fail-closed 维持 CHANGES_REQUESTED）：

$CURL_IMAGE 已定义（见上）。若未定义则为合并阻塞项。
deploy-files 仓库的分支保护允许 CODE_TOKEN 直推 $APP_NAME-prod 分支；否则两个 sync 作业在生产必然失败。属部署前置条件，非代码缺陷。
sync_code 与 sync_catalog 在 bootstrap 流水线中的排序/串行化证据（见 P1）。若可并发则为合并阻塞项。
文档将组合 Smoke Test 时机改为「更新生产分支前，用目标 Catalog 镜像和当前生产 Forge 镜像」，但本 diff 无 smoke test 相关 CI 改动；请确认既有 smoke_prod: / smoke 作业在流水线 DAG 上先于 sync_catalog 执行。不阻塞合并（运行时 /readyz 兜底仍在），但文档与 CI 必须一致。

数据流回溯（逐被消费数据）

$TARGET_BRANCH ← "$APP_NAME-prod"（作业内定义）→ 被 git push "HEAD:$TARGET_BRANCH" 消费；两个作业一致 ✅。
$DEPLOY_REPO_URL ← https://__token__:${CODE_TOKEN}@${DEPLOY_FILES_REPO}（未变）→ 被 clone/push 消费 ✅；argocd_sync 不再消费 CODE_TOKEN（密钥面缩小 ✅）。
image: $CURL_IMAGE ← 上游定义未出现在 diff → 未验证（见 P2）。
$IMAGE_DIGEST / $CATALOG_IMAGE_DIGEST ← web/bootstrap 变量 → 原消费点（argocd_sync 的 grep）被删除，现仅存于提交信息与后续 /readyz 校验 ✅ 无悬空引用。
sync_catalog 的 grep -q 门禁仍在 fresh clone 的生产分支 manifest 上、commit 之前执行，顺序未变 ✅。
测试数据流：readFile(".gitlab-ci.yml") → 正则/计数断言；所有被断言字符串均与本 diff 逐字核对 ✅；ci.slice(indexOf("argocd_sync:"), indexOf("smoke_prod:")) 依赖既有文件布局（argocd_sync 在 smoke_prod 之前）。

盲点 checklist（C1–C6）

C1 双路径 parity：hit → 已验证。sync_code/sync_catalog 成对改动对称（同删 SOURCE_BRANCH、同改直推、push 语句逐字一致）；回滚路径（恢复 digest → push → argocd_sync）不受影响。
C2 control-flow ordering / 复用：hit → 同一 push 模式被两个可并发的作业复用，产生冲突面（P1）；argocd_sync 控制流由「先校验后同步」改为「直接同步」（P2）。
C3 授权边界：hit → CODE_TOKEN 从「创建 MR」升级为「直推生产分支」能力，分支保护/token 范围无法在 diff 内验证（Human-verify 2）；正向变化：argocd_sync 不再持有 CODE_TOKEN。
C4 授权生命周期 / 级联：N/A — 本 diff 无鉴权/状态级联改动。
C5 build pass ≠ 运行期路径正确：hit → 本评审无法运行 GitLab CI、也无法读取 CI 全文；镜像变量、作业 DAG、分支保护等运行时事实均需作者举证（Human-verify）。这是 fail-closed 裁决的主因。
C6 治理/文档自洽性：hit → 已验证。README/文档中所有「部署 MR」表述一致替换；失败模式表、审批表、Registry 保留策略（删去「未合并部署 MR 引用的 digest」保留项）均与新模型自洽；提醒切换窗口确认无 in-flight 部署 MR 的 digest 会因此立即失去保留保护。

跨轮 blocker 复检（R6）

N/A — 本 MR 第 1 轮评审。

Things I checked that are fine

新测试断言与 diff 逐字一致：两条直推语句、无 merge_request.create、无 SOURCE_BRANCH="deploy/、argocd 作业 when: manual + image: $CURL_IMAGE、无 git clone / CODE_TOKEN。
文档的运行时兜底叙述未变且正确：不兼容 → /readyz 失败 → 新 Pod 不接流量、旧 Pod 继续服务；回滚仅需恢复 Catalog digest。
sync_catalog 提交前的三项 grep -q 门禁（image/digest/revision 元数据存在性）完整保留。
bootstrap 语义在 README 与 docs 之间一致（「用两个 digest 直接更新 deploy-files 生产分支」）。

Verdict: CHANGES_REQUESTED

改动方向（取消部署 MR、收敛为人工 argocd_sync 唯一闸门）是有意设计决策，文档/测试自洽，我们不阻塞设计决策本身。阻塞点是本 PR 新引入、且在 diff 内无法自证的运行时契约：(a) 两个作业写同一生产分支的 push 冲突面需要串行化/重试证据或修复；(b) $CURL_IMAGE 是否已定义。同时本评审无法访问源仓库核验 diff 之外上下文，按本轨道 fail-closed 要求，在上述事项确认前给 CHANGES_REQUESTED。

[Octo-Q] verdict: REQUEST_CHANGES — P1（sync_code/sync_catalog 直推同一生产分支的并发冲突面，无可见串行化/重试）+ 2 个 P2（argocd_sync 失去同步前机器校验；$CURL_IMAGE 定义需确认）；审查方无法访问源仓库验证 diff 外上下文，按 fail-closed 卡住。

Reviewer: Octo-Q (automated review)

附录 A — 数据流回溯

contextValue（summary_context）→ 客户端入参 → normalizeSummaryWorkspaceContext（上限/去重/类型/时间范围校验）→ canonicalizeSummaryWorkspaceContextForActor（DM 归一化、剔除 actor 自身）→ scope_hash 入 turn 请求哈希；运行时所有消费点（路由、校验、工作流创建、保存）均以服务端持久化/再校验后的值为准，客户端回显不可信。✔ 真流到消费点。
SelectedChannels 授权 → pipeline.GetUserChannels（IM 库成员关系：group_member/space_member + g.status=1）→ FilterChannelsForWorkspace（space 过滤 + DM 对端活跃成员校验）→ validateSources 白名单比对；findMostRecentAuthorizedChannel 同源管道。✔ 消费前全部重校验。
预览内容 → emit_summary_response（DisallowUnknownFields + result_type allowlist）→ CompleteTurn 落 response_payload_json → 保存时 loadWorkspacePreviewForSave（事务内加锁重读，版本/身份逐项比对）→ content = payload.Preview.Content。✔ 无提前 return/空转路径；旧预览的 effective_scope 回填（hydrate）后仍会过 validateSources。
工作流创建 → CreatePersonalFromAgent/CreateTeamFromAgent（ActorID 恒为 creator，无 creator override）→ 事务内建 task/sources/participants + 幂等绑定（insert + FOR UPDATE 回读写胜者）→ 提交后 go triggerWorker（与既有模式一致，worker 侧 scanStuckPersonalTasks 兜底）。✔。
DEFAULT_TIME_RANGE_DAYS → config 读取 → cmd/*/main.go 赋值 pipeline.DefaultTimeRangeDays → 无任何生产消费点（本 PR 移除最后消费者）。✘ 数据流断裂 → 见 finding 1。
显式 specifiedSources → ApplySourceConstraints 与 discovery 交集 → 若被新 space 过滤丢弃则候选静默缩减。✘ 无显式失败路径 → 见 finding 2。

附录 B — 盲点 checklist（security_sensitive 全项）

C1 双路径 parity：已查。add↔remove 对：legacy 保存删除（space_id='' 限定）与 workspace 保存标记（不删除）分离正确；cleanup cron 只清 legacy 行、且 evidence 有 NOT EXISTS agent_summary_session 保护；subscribe↔unsubscribe：BeginTurn 作用域升级会复位 preview/proposal/workflow 指针，CompleteTurn 各 result 类型互斥清理对称。✔
C2 control-flow / 嵌套复用：completeTeamWorkspaceWorkflow 被 chat 直发与 confirm 端点复用；两处均先过 validate* 再创建，但 confirm 路径参与者取自请求而非提案（见 finding 3）。buildRegistryWithUID 增加 terminal factory 分支，既有 profile 路径不变。⚠（finding 3）
C3 授权边界 ≠ 能力边界：新增 emit_summary_response terminal tool 仅注册在 summary_workspace profile；所有工具经 buildRegistryWithUID 注入服务端 uid/session；能力面（Agent 可读频道）= 成员关系 ∩ space ∩ 请求 allowlist，模型无法自扩权。✔
C4 授权生命周期 / 容器-成员级联：参与者校验 space_member.status=1；团队范围校验 group_member.is_deleted=0；群组本身 g.status=1 在 discovery SQL 内。保存时点不重验成员资格（预览时已验），属既有 save-draft 语义，可接受。✔
C5 build/note 通过 ≠ 运行期正确：未在本环境运行 build/测试（skill 约束）；静态推演覆盖：migration 列顺序与 GORM tag 对齐、collation 改动对 SQL JOIN 的影响（未发现跨 collation JOIN）、lease/replay 时序、SSE/非 SSE 双响应路径。⚠ 需终审/CI 对 head 实跑确认。
C6 治理/策略文档自洽性：本 PR 未改 SECURITY.md/披露策略等治理文档。N/A。

附录 C — 跨轮 blocker 复检（R6）

N/A — 本 PR 首轮审查，无上轮未解决 blocker。head SHA 2c0dcf26255ee0468d67ef67ed1c950b96a5e9bc（与任务给定一致），merge-base eb9e0e3。

automated review建议（供终审参考）

[Octo-Q] verdict: APPROVE — 未发现 P0/P1：新增面全部处于 StrictAuth+StrictSpace 之后，会话/提案/保存/清理均按 (space_id, user_id, session_id) 隔离并带乐观版本与租约幂等；Agent 工具读面被成员关系 ∩ space ∩ allowlist 三重约束；四个 P2（DEFAULT_TIME_RANGE_DAYS 配置失效、worker space 过滤静默丢显式源、提案确认路径不可达且确认不校验提案参与者集合、session_id collation 变更的在线 DDL 风险）+ 一个注释 mojibake nit，均不满足 R1 阻塞条件（默认部署不受影响 / 无越权 / 无错误数据落库）。review 正文按 output-format 为 COMMENT（有 P2），按 R4 映射为 APPROVE。

yujiawei · 2026-09-01T08:25:11Z

Addendum to the review above — two more findings on the same code path (head `2c0dcf2`, unchanged)

Both concern the time-range and space-filter changes already discussed, so they belong with S2 and with the new WithSpaceID work rather than being separate topics. Neither changes the verdict; adding them so the fix list is complete.

A1 (P1) — `DEFAULT_TIME_RANGE_DAYS` silently becomes a no-op, and the new ceiling has no knob at all

cmd/ is untouched by this PR, so both binaries still wire the env var into the global at boot:

// cmd/summary-api/main.go:38-39   (identical at cmd/summary-worker/main.go:35-36)
if cfg.DefaultTimeRangeDays > 0 {
    pipeline.DefaultTimeRangeDays = cfg.DefaultTimeRangeDays   // from DEFAULT_TIME_RANGE_DAYS
}

At merge-base that global had three readers — internal/api/handler/task.go:339, internal/api/handler/bot_summary_create.go:272-273, internal/pipeline/fetch.go:861. This PR moves all three off it (to MaxTimeRangeDays, or to the new hardcoded legacySummaryDefaultTimeRangeDays = 31 at internal/service/summary_workflow.go:38). At HEAD, pipeline.DefaultTimeRangeDays is assigned but read by nothing in production — grep confirms the only remaining references are the declaration, the config plumbing, and its own doc comment.

Failure scenario: an operator running with DEFAULT_TIME_RANGE_DAYS=7 to bound fetch cost silently gets a 31-day default and a 90-day ceiling after this deploy. There is no error, no warning, and no log line — the assignment still succeeds, it just has no effect. Worse, the new MaxTimeRangeDays = 90 is a plain var with no config binding, so there is no longer any way to lower the ceiling from the environment. Combined with S2, an operator loses both the control and the thing it controlled in the same change.

Suggest: bind MaxTimeRangeDays to config (e.g. MAX_TIME_RANGE_DAYS, defaulting to 90) and either keep DefaultTimeRangeDays wired to a real consumer or delete the now-dead config field and its cmd/ assignments so the dead knob is not left looking live.

A2 (P2) — an explicitly selected source that fails the new space filter is silently dropped, with no failure path

ResolveAndFetchMessagesForPersonal now space-scopes discovery:

// internal/pipeline/fetch.go:938-943
channelQueryOpts := []ChannelQueryOption{WithSelectedThreads(selectedThreads)}
if channelScopeOpts != nil && strings.TrimSpace(channelScopeOpts.SpaceID) != "" {
    channelQueryOpts = append(channelQueryOpts, WithSpaceID(channelScopeOpts.SpaceID))
}
userChannels, err := GetUserChannels(ctx, creatorUID, imDB, channelQueryOpts...)

userChannels then feeds ApplySourceConstraints, which is a pure intersection with no error return:

// internal/pipeline/fetch.go:437-440
for _, ch := range userChannels {
    if specified[ch.ChannelID] && allowed[ch.ChannelID] {
        result = append(result, ch)

So a source the user explicitly picked whose group lives in another space — or a DM whose peer is not an active space_member — now vanishes from candidates (:1017). The only signal is an informational log line (%d → %d candidates). If every specified source is filtered out, candidates is empty, fetchMessagesByBackend returns zero messages, and the summary is produced from nothing.

This is a correct tightening (that filter is the point of the change) but the degradation is invisible. The user-visible symptom is "I selected three chats and got an empty summary." Suggest: compare the post-filter candidate set against specifiedSources and fail explicitly — or at minimum log at warn level naming the dropped source_ids — when an explicitly requested source is discarded for space reasons.

Jerry-Xin

Re-review at head dd32019655a60359c2381d96788c49c88ba5e3ea (one new commit dd32019 since the previous review 5075370881 at 2c0dcf26255e).

Prior blockers — resolution status

✅ FIXED — Team confirmation unreachable (prior 🔴 1). DeriveSummaryRoute now returns SummaryRouteTeamConfirmation for chat+generate requests with participants; SummaryRouteTeamWorkflow is only reachable through the confirm_workflow action after the proposal guards pass (internal/service/summary_route_policy.go:72,116). completeWorkspaceProposal persists the proposal, and ConfirmSummaryWorkspaceProposal (registered at POST /api/v1/agent/summary-sessions/:session/proposals/:version/confirm) re-validates the persisted scope and the proposal participant set before creating the workflow. Covered by the new route-policy tests (...require_confirmation, confirmed_current_team_proposal_creates_workflow, stale_team_proposal_cannot_create_workflow).
✅ FIXED — Worker dispatch can be lost (prior 🔴 2). Idempotent replay now re-emits a WorkerTrigger whenever the replayed task is non-terminal and the creator's personal result is still pending (internal/service/summary_workflow.go:614-633), and both workspace dispatch sites now send the trigger regardless of Replayed. triggerWorker now returns errors for network failures and non-2xx responses instead of swallowing them. The pre-existing scheduler sweep (scanStuckPersonalTasks, re-triggers accepted participants with a pending personal result older than 5 minutes) provides the durable backstop, so a dropped dispatch is recovered without client action. Test TestSummaryWorkflowIdempotencyReplayAndMismatch asserts the replay carries the trigger.

🔴 Blocking on this head

🔴 Critical — the new workspace cleanup can delete an actively renewed session (TOCTOU). cleanupExpiredSummaryWorkspaces selects expired sessions (expires_at <= now, or NULL expires_at with stale updated_at) outside the deletion transaction (internal/api/handler/agent_session_cleanup.go:206), and deleteWorkspaceSessionState then removes the session's runs, evidence artifacts, messages, turns and finally the session row itself without re-locking the row or re-checking expires_at / updated_at / active_turn_id. If BeginTurn renews the session between selection and deletion — e.g. a user returns to a 30-day-expired session exactly while the daily cleanup is running — the active session and its whole transcript are deleted mid-turn: the in-flight turn's CompleteTurn can no longer find the session row (500 to the client) and the user's workspace history is permanently lost. Fix: inside the deletion transaction, re-lock each session (SELECT ... FOR UPDATE), re-evaluate the expiration predicate (and skip sessions with a live turn lease), and add a concurrency regression test covering renewal between selection and deletion.

💬 Non-blocking

Legacy and bot surfaces are correctly restored to the 31-day ceiling (task.go now constructs the service with default=max=31; bot_summary_create.go validates against pipeline.DefaultTimeRangeDays); the 90-day ceiling now applies only to the new workspace entry and is configurable via MAX_TIME_RANGE_DAYS. The doc statement about the legacy default is now accurate. This resolves the scope-creep concern raised on the previous head.
The migration split (20260827-02/03/04) addresses the hot-table rebuild concern: no session_id collation change, ALGORITHM=INPLACE, LOCK=NONE, conditional application + IF NOT EXISTS for environments that already ran the original all-in-one 20260827-01. Note: upgraded environments keep the old idx_agent_summary_turn_session / idx_agent_summary_turn_lease indexes (the new file only adds idx_agent_summary_turn_owner_session) — harmless, worth a follow-up.
Summary-context strings are now capped (256/256/8192 runes) and the chat/confirm routes enforce a 512 KiB request-body limit — the previously uncapped-input concern is addressed.
Workspace confirm now derives the workflow idempotency key from the proposal token+version, so racing double-confirms replay one task instead of double-inviting participants. fetch_channel/peek_channel now enforce the workspace space scope, and discovery grants are unioned instead of replacing the scope — both peer concerns from the previous head.
Some dead code flagged on the previous head remains (newSummaryWorkspaceProposalToken, writeWorkspaceServiceError, MarkPreviewSaved, ValidateProposal, the leftover _ = fmt.Sprintf scaffolding) — cleanup recommended, not blocking.
Handler and worker test packages still cannot link on this host because libtokenizers is unavailable (as documented in the PR body); service, pipeline, config, and db suites pass, including the new confirmation-route and replay-trigger tests.

✅ Highlights

The idempotency model (replay-with-trigger + proposal-derived workflow key) is now coherent end to end.
Retention is real: expires_at is maintained as a sliding 30-day window on every session touch, and the reaper also retires orphaned idempotency tombstones.
time.Now() usage in the workspace store was consistently migrated to timezone.Now().

mochashanyao

[Octo-Q · automated review]

Verdict: Request changes — blocking findings below (data-flow traced).

Reviewer: Octo-Q (automated review)
PR: #234 — feat(summary): add unified summary workspace backend
head: dd32019655a60359c2381d96788c49c88ba5e3ea · merge-base: eb9e0e3adabd03208b7d3695290d7bb5c034fae4 (origin/main)
Diff: pr-base...HEAD · 103 files · +11395/−550 · 静态审查（未在本环境执行 build/tests）

[Octo-Q] verdict: REQUEST_CHANGES — 存在 1 个 P1 阻塞（新增的证据清理 SQL 在 MySQL 上必然报 1267 排序规则冲突，整条清理语句（含既有 legacy 部分）永久失效）。按 R4 映射：有 P1 → REQUEST_CHANGES。

1. 验证结论

⚠️ internal/api/handler/agent_session_cleanup.go:175 — 证据清理 DELETE 的 NOT EXISTS 子查询跨排序规则列比较（utf8mb4_0900_bin vs utf8mb4_unicode_ci），MySQL error 1267，语句整体失败（详见 P1）。
✅ 回合/租约幂等：BeginTurn/CompleteTurn/FailTurn（internal/api/handler/agent_workspace_store.go）session 行 FOR UPDATE + (space,user,session,request_id) 唯一键 + attempt/active_turn_id 双守卫，租约被接管后旧执行者完成写入必被拒（ErrWorkspaceTurnLeaseLost/ErrWorkspaceScopeConflict），无双重落库路径。
✅ 频道授权 parity（C1 逐对核验）：fetch_channel/peek_channel 在任何 DB 依赖解析前先过 ChannelAllowedByScope；search_messages/filter_relevant/summarize_chunk 的缓存 handle 绑定 (uid, sessionID) 双因子（internal/agent/cache.go），跨 session/跨用户取数返回 nil；发现类工具（list/narrow/find_shared）经 FilterChannelsForWorkspace+RestrictDiscoveredChannels，闭 UI scope 下 AuthorizeDiscoveredChannels 为 no-op，只有 openScopeAgent 可信发现可扩展白名单。
✅ 工作流幂等：binding insert + 锁回读判定 winner（不信 RowsAffected，规避 CLIENT_FOUND_ROWS）；replay 仅在 creator personal_result 仍 Pending 时补发 worker trigger；已删任务返回 40009 + recovery_action。
✅ 保存边界：workspace 保存不再走 legacy 的 (user, public session) 键控 origin 回查（该回查无法表达 space 边界），origin/sources/references 全部取自服务端持久化 scope；markWorkspacePreviewSaved 在同一事务内以乐观条件 + RowsAffected 双写 message/session 标记。
✅ 清理级联完整：30 天退休在单事务内删除 messages/turns/evidence/runs/specs/manifests/session，loadWorkspaceSnapshotTx 对 LatestPreviewMessageID 的硬依赖不会出现悬空引用；24h legacy 清理显式限定 space_id=''，不会误删 workspace 行。
✅ 路由/鉴权：新端点均在 StrictAuthMiddleware+StrictSpaceMiddleware 组内（internal/api/router/router.go:76-77,159-170）；capabilities 仅暴露非可操作元数据。

2. 发现问题（diff-scope 三问 + 定级）

P1 — 证据清理 DELETE 跨排序规则比较，MySQL 上每个 tick 必然失败（`internal/api/handler/agent_session_cleanup.go:175`）

三问：1) 本 PR 新引入（NOT EXISTS 守卫为本 PR 新增，原语句在 base 分支可正常工作）；2) 非"既有行为"——本 PR 直接改写了该路径；3) 不适用降级。
机制：agent_summary_session.agent_session_id（migrations/sql/20260827-03-agent-summary-session.sql:11，utf8mb4_0900_bin）与 agent_message_evidence.session_id（migrations/sql/20260717-01-add-agent-message-evidence.sql:16，表默认 utf8mb4_unicode_ci）均为列、隐式 coercibility 相同 → MySQL 1267 "Illegal mix of collations"，且在语句解析/执行期即报错，与表是否有数据无关。
后果：整条 DELETE 中止 = legacy 证据清理（原有可用路径）被本 PR 打断 + workspace 证据保留守卫同时失效 → agent_message_evidence 无限增长，#161 P2 修过的"复用 session_id 引用池膨胀"复发。确定性触发（每个清理周期），非边缘。
定级：R1"让本来能工作的路径在生产中变得不可用" → P1 阻塞（R4：有 P1 → REQUEST_CHANGES）。测试全绿仅因全部用内存 SQLite（agent_session_cleanup_test.go:18 等），SQLite 不做排序规则冲突检查——测试绿 ≠ 运行期正确（C5）。
修复：比较一侧显式 COLLATE（如 agent_summary_session.agent_session_id = agent_message_evidence.session_id COLLATE utf8mb4_unicode_ci），并在真实 MySQL 验证该语句。

P2 — scope_version 跳变静默丢弃在途 workflow 的会话指针（`internal/api/handler/agent_workspace_store.go:236`）

三问：新引入（本 PR 新存储语义）；非既有行为。
机制：BeginTurn 遇更高 scope_version 时清零全部折叠态（含 workflow_task_id/workflow_started_message_id），但底层 summary task 继续运行；handleSummaryWorkspaceHistory 仅在 WorkflowTaskID>0 时 reconcile → 用户改选择后，先前发起的总结完成时工作台不再出现完成卡片（任务本体仍在任务列表）。
定级：数据无损、任务可达，仅状态展示降级且需用户在执行中改 scope 才触发 → P2（若终审认为违反"完成后自动保存"的用户可见承诺，可按 R1 上调）。

P2 — MySQL 专属契约只被 SQLite 测试覆盖（`migrations/sql/20260827-03-agent-summary-session.sql:11`）

二进排序规则标识列、与 agent_message_evidence 的混合排序规则比较、ALGORITHM=INPLACE, LOCK=NONE 在线 DDL、代码中显式规避的 CLIENT_FOUND_ROWS 行为，SQLite 全部不可见——正是上面 P1 漏网的原因。建议至少为清理语句补一个 MySQL 集成校验。

3. 建议

P1 一行修：清理语句比较加显式 COLLATE；顺手在 CI/手工流程中对 4 条新迁移 + 清理语句跑一次真实 MySQL。
P2-a：scope 跳变前对未终结 workflow_task_id 先 reconcile（或跨 scope 保留 workflow 指针），避免完成卡片丢失。
P2-b：补 MySQL 侧最小集成测试（排序规则兼容 + 清理语句可执行）。

4. 额外发现（Human-verify，均非本 PR 阻塞）

worker 侧空间收窄现在无条件生效（internal/worker/processor.go:787、internal/worker/personal_processor.go:724 恒传 SpaceID: task.SpaceID）：legacy 任务若显式 source 指向其他空间的群、或 DM 对端已离开本空间，将从"静默抓取"变为 validateExplicitSourceCoverage 硬失败（"explicit summary source is unavailable in the current space"）。按安全收紧理解是有意为之，但请产品确认对存量 legacy 流的硬失败可接受。
buildSummaryWorkspaceGuidance（internal/api/handler/agent_summary_workspace.go:1572-1598）把上一版预览正文与页面上下文 JSON 注入 system prompt，已用 <current_preview>+"仅是数据"框定；预览内容源于聊天消息，属提示注入面，建议后续红队过一遍。

5. 数据流回溯

被消费数据	上游来源	是否真流到消费点
workspace scope（context JSON/hash）	客户端 `summary_context` → `normalizeSummaryWorkspaceContext`（长度/类型/时间范围上限）→ `canonicalizeSummaryWorkspaceContextForActor` → sha256 hash 落 `agent_summary_session`	✅ confirm 时从持久化行重读（不信客户端重放）；save 时从 session 行重建并二次 normalize+hydrate
preview 正文	模型 `emit_summary_response.preview.content` → 结构校验（DisallowUnknownFields + 形状约束）→ `CompleteTurn` 落 `response_payload_json`	✅ 保存路径 `loadWorkspacePreviewForSave` 重解码并校验 payload/message 一致性；`effective_scope` 字段服务端保留、模型提交即拒绝
messages_handle	`messageCache.Store(msgs, uid, sessionID)`（fetch/peek 时写入）	✅ 全部消费点（search/filter/summarize_chunk/citations/getSessionMessagePool）均带同一 `(uid, sessionID)` Retrieve，双因子不匹配返回 nil；workspace 使用派生 agent_session_id，legacy 使用公共 session_id，互不串用
worker trigger	`persist()` 事务提交后返回 → `go triggerWorker()`（5s 超时）	✅ 丢失有兜底：`scanStuckPersonalTasks` M3 对 accepted 且 personal_result Pending>5min 者补发；replay 仅 Pending 时补 trigger，无重复派发
证据行（agent_message_evidence）	fetch/peek 时 PersistEvidence（session=派生 agent_session_id）	⚠️ 消费/清理链本身即 P1：清理语句在 MySQL 必失败（见上）；退休级联删除路径（deleteWorkspaceSessionState）参数化比较、无排序规则问题
最近活跃频道（needsRecentFallback）	`findMostRecentAuthorizedChannel`：GetUserChannels→FilterChannelsForWorkspace→按 MessageTable 分桶聚合 `MAX(timestamp)`（表名 crc32 哈希生成，无注入）	✅ 仅模板/系统意图且无显式来源时触发；无候选返回友好澄清而非报错

6. 盲点 checklist（security_sensitive，全项）

C1 双路径 parity：clear。add/remove（scope 跳变重置映射在 BeginTurn 与 CompleteTurn 两处对称）；save 双写（message 行 + session 行，均校验 RowsAffected）；legacy 删除分支与 workspace 标记分支在 CreateAgentSummary 显式分流；注册表 Register/RegisterTerminal 互斥去重。
C2 control-flow ordering / 嵌套复用：clear。CreateTeamFromAgent 两处调用（工作台直发 + 提案确认）均带幂等键；normalize 被 legacy/agent 复用时，默认时间范围不进幂等指纹（hashInput.explicitTimeRange 处理）避免重试误判；幂等键先 TrimSpace 再正则 ^[A-Za-z0-9][A-Za-z0-9._:-]*$ + DB 二进排序规则（大小写/字节敏感），非规范形式（前导空白、大小写变体）已试穿。
C3 授权边界：clear。工具能力经包装器注入可信 uid/session；白名单闭包不可被模型扩展；确认端点在副作用前重验参与者/来源/引用/团队成员资格。
C4 授权生命周期级联：clear。群/子区按 space_id 等值过滤、g.status=1+gm.is_deleted=0；DM 对端须本空间 status=1 活跃成员；确认时刻二次校验；抓取时刻经 GetUserChannels 再次成员资格过滤（中途退群即收口）。
C5 build/note ≠ 运行期正确：命中 —— P1 即此类：SQLite 测试全绿但 MySQL 运行期必失败；已按 P1 上报，不以"测试通过"缓解。
C6 治理/策略文档自洽：clear。docs/unified-summary-safe-track.md 为设计边界说明，与安全披露/治理文档无冲突。

7. 跨轮 blocker 复检（R6）

N/A — 本 PR 首轮审查。

以下为按 skill output-format 渲染的正式 review 正文（供终审复核/转发）：

Code Review — PR #234 (octo-smart-summary)

Summary

This PR lands the unified summary workspace backend: a space-scoped session/turn store with lease-based idempotency (agent_summary_session / agent_summary_turn), a deterministic server-side routing policy, a terminal-tool Agent contract (emit_summary_response), request-scoped channel allowlists with trusted discovery, and workflow creation moved into SummaryWorkflowService with durable idempotency bindings. Legacy chat/save endpoints are namespace-isolated via space_id='' and keep their prior behavior; the worker pipeline gains space-scoped discovery. The architecture is careful and the security boundaries are mostly well constructed; one MySQL-specific defect in the cleanup job blocks this as-is.

Verification

Static analysis only at head dd320196; build and tests not executed in this environment.

✅ Turn/lease idempotency — BeginTurn/CompleteTurn/FailTurn use SELECT ... FOR UPDATE on the session plus the unique (space,user,session,request_id) turn key; attempt checks and active_turn_id guards prevent double-completion after lease takeover (internal/api/handler/agent_workspace_store.go).
✅ Channel scope enforcement parity — fetch_channel, peek_channel check ChannelAllowedByScope before any DB access; search_messages/filter_relevant/summarize_chunk consume cache handles bound to (uid, sessionID); discovery tools filter via FilterChannelsForWorkspace + RestrictDiscoveredChannels, and allowlist expansion is a no-op on closed UI scopes (internal/agent/types.go:146-260).
✅ Workflow idempotency — binding insert + lock read-back identifies the winner without trusting RowsAffected; replays never re-dispatch a worker unless the creator's personal result is still pending; deleted-task tombstones return 40009 with recovery actions (internal/service/summary_workflow.go).
✅ Save boundary — workspace saves skip the legacy session-keyed origin lookup, derive origin/sources/references from the persisted scope, and mark preview saved via optimistic RowsAffected checks in the same transaction as the formal summary (internal/api/handler/agent_summary.go:229-249, internal/api/handler/agent_summary_save.go).
✅ Cleanup cascade — workspace retirement deletes messages, turns, evidence, runs, manifests and the session row in one transaction, so no dangling latest_preview_message_id can break loadWorkspaceSnapshotTx.

Findings

One P1 blocker described below; two P2 items.

P1 — Evidence-cleanup DELETE mixes collations and will fail on MySQL every tick (`internal/api/handler/agent_session_cleanup.go:175`)

The new NOT EXISTS guard compares agent_summary_session.agent_session_id (utf8mb4_0900_bin, migrations/sql/20260827-03-agent-summary-session.sql:11) with agent_message_evidence.session_id (utf8mb4_unicode_ci table default, migrations/sql/20260717-01-add-agent-message-evidence.sql:16). Both are columns with equal coercibility, so MySQL raises error 1267 ("Illegal mix of collations") and aborts the entire statement — including the pre-existing legacy evidence cleanup that worked before this PR. The suite is green only because every test runs on SQLite, which does not enforce collation mixing. In production agent_message_evidence is then never cleaned again (unbounded growth plus the reused-session citation-pool inflation that #161 P2 fixed), and the workspace retention guard never runs either.

-- fix sketch: make one side explicit
AND agent_summary_session.agent_session_id =
    agent_message_evidence.session_id COLLATE utf8mb4_unicode_ci

Add an explicit COLLATE to the comparison (either side) and verify the statement against a real MySQL instance before merge.

P2 — Scope bump silently drops a running workflow's session pointers (`internal/api/handler/agent_workspace_store.go:236`)

When BeginTurn sees a higher scope_version it resets every folded artifact, including workflow_task_id / workflow_started_message_id (lines 236-240), while the underlying summary task keeps running. handleSummaryWorkspaceHistory only reconciles when WorkflowTaskID > 0, so after a mid-flight scope change the workspace never learns the earlier workflow completed: the user was told "已开始生成总结，完成后会自动保存" but never sees a completion card (the task itself survives in the task list). Either preserve workflow pointers across scope bumps or reconcile an outstanding workflow_task_id before resetting.

P2 — MySQL-only contracts are exercised only against SQLite (`migrations/sql/20260827-03-agent-summary-session.sql:11`)

The binary-collation identifier columns, the mixed-collation comparison they create with agent_message_evidence, the ALGORITHM=INPLACE, LOCK=NONE DDL, and the CLIENT_FOUND_ROWS behavior the Go code explicitly works around are all invisible to the SQLite-backed suite — which is exactly how the P1 above shipped. Worth adding at least one MySQL-backed integration check for the cleanup statements.

Human-verify

Space-scoped discovery is now unconditional in the worker (internal/worker/processor.go:787-790, internal/worker/personal_processor.go:724-727): legacy tasks whose explicit sources reference a group in another space, or a DM whose peer left the space, will now fail with "explicit summary source is unavailable in the current space" (internal/pipeline/fetch.go validateExplicitSourceCoverage) instead of silently fetching. This reads as the intended security tightening, but product should confirm that hard-failing these pre-existing legacy flows is acceptable. Not a merge blocker for this PR.
buildSummaryWorkspaceGuidance embeds prior preview content and page-context JSON into the system prompt wrapped in <current_preview>/"data only" framing (internal/api/handler/agent_summary_workspace.go:1572-1598). The framing matches the prompt policy, but adversarial chat content surviving into a preview is a prompt-injection surface worth a red-team pass. Not a merge blocker for this PR.

Things I checked that are fine

Turn replay after a later artifact advance replays the current authoritative message rather than mixing a historical result with today's state (turnFromSnapshot).
Terminal-tool hygiene: rejected/failed terminal attempts are never persisted; successful ones persist only the visible bubble; stripTerminalToolHistory + sanitizeToolProtocolHistory keep replayed transcripts protocol-valid.
Proposal confirmation re-validates participants, sources, references and team membership against the IM DB at confirm time, from the persisted scope (not client-repeated context), with proposal version + random token.
Fire-and-forget worker triggers are safe: scanStuckPersonalTasks (M3) re-triggers accepted participants whose personal result is pending >5 min.
Idempotency replay lookup runs before the workspace save preflight, so a retried save replays the binding instead of hitting the stale-preview conflict.
New endpoints sit behind StrictAuthMiddleware + StrictSpaceMiddleware; the capabilities endpoint exposes only non-actionable metadata.

Verdict: CHANGES_REQUESTED

The P1 (mixed-collation cleanup statement) is the only blocker and is a one-line fix plus MySQL verification. The two P2s are optional but recommended.

[Octo-Q] verdict: REQUEST_CHANGES — 1 个 P1（证据清理 SQL 跨排序规则比较在 MySQL 上必然失败，既有清理路径被本 PR 打断）；按 R4 有 P1 即 REQUEST_CHANGES。该建议供终审复核，非对外 verdict。

yujiawei

Code Review — PR #234 (octo-smart-summary)

Reviewed at head dd32019655a60359c2381d96788c49c88ba5e3ea against merge-base eb9e0e3adabd03208b7d3695290d7bb5c034fae4 (103 files, +11395/-550). This is a re-review after the hardening commit dd32019 landed on top of 2c0dcf2; the prior review at 2c0dcf2 raised 4 spec findings, 10 P1s, and ~35 P2s.

Verification performed locally at this SHA: go build ./... clean, go vet ./... clean. The commit dd32019 meaningfully fixes roughly two-thirds of the prior findings (see §3), but it also introduces real regressions and leaves several blocking issues in place.

Three important things to flag up front:

go test ./internal/api/handler is RED at this SHA. Two new cleanup tests and one pre-existing workspace-store test fail deterministically in any non-Asia/Shanghai TZ — and CI (ubuntu-latest, TZ unset → UTC) will hit this. Details at P1-1.
ReconcileWorkflow idempotency is broken for error/cancelled terminal states. The second history poll for a failed workflow 500s instead of replaying the already-written terminal message. Details at P1-2.
Several prior P1s are fixed (space-boundary bypass, AuthorizeDiscoveredChannels replace-vs-union, proposal/confirmation reachability, deleted-summary 500, unbounded snapshot read, request-body cap, migration split, retention reaper). They are not re-listed except where the fix itself has a defect.

1. Spec compliance verdict

Spec: ❌

Measured against the self-declared contract surface (code + docs/unified-summary-safe-track.md) and PR title ("unified summary workspace backend"):

漏建 — Incomplete / incorrect fixes

Team-workflow time-range ceiling still widened to 90 days on the worker fetch path. internal/pipeline/fetch.go:940 still caps at MaxTimeRangeDays (= 90) for the ResolveAndFetchMessagesForPersonal pipeline, and both internal/worker/processor.go:793 (team processor — participant-bearing tasks) and internal/worker/personal_processor.go:730 (personal processor) invoke it. The legacy HTTP ceiling was correctly restored to 31 (internal/api/handler/task.go:58, internal/api/handler/bot_summary_create.go:272, now using DefaultTimeRangeDays=31), and service-layer ValidatePersonalWorkflow is now fed s.maxTimeRangeDays, but the workspace-coordinator constructs the workflow with NewSummaryWorkflowService(db, imDB, DefaultTimeRangeDays, MaxTimeRangeDays) at internal/api/handler/agent_summary_workspace.go:120, so workspace-originated team/personal tasks still allow a 90-day span. docs/unified-summary-safe-track.md:31 ("legacy endpoint still uses DefaultTimeRangeDays=31") does not actually claim a 90-day ceiling for the workspace, so this is not strictly a doc lie — but please confirm 90 days was the intended workspace product decision, because the prior review's S2 framed the widening as unintended. As shipped it touches team tasks created from the workspace.

超建 — Out-of-scope surface

The workspace path still ships ~200 lines of dead/production-unreachable code: AgentWorkspaceStore.ValidateProposal (internal/api/handler/agent_workspace_store.go:783), AgentWorkspaceStore.MarkPreviewSaved (not referenced outside tests; the production save path uses markWorkspacePreviewSaved in agent_summary_save.go:249), newSummaryWorkspaceProposalToken (agent_summary_workspace.go:1674 — the proposal flow uses workspaceStoreToken at agent_workspace_store.go:457), writeWorkspaceServiceError (agent_summary_workspace.go:1905), hasExplicitWorkspaceRunIntent (:1115) and summaryWorkspaceRequirement (:1277) (only referenced by summary_workspace_contract_test.go), and the scaffolding no-op block at agent_summary_workspace.go:2180-2184. The handler also hard-rejects any action != "chat" at agent_summary_workspace.go:143, which makes SummaryRouteAgentPreviewSave (internal/service/summary_route_policy.go:36, 75) a route that can be returned but is never reachable and has no case arm in the consumer switch — it silently falls into the default clarification reply. Either wire or delete.
cmd/summary-api/main.go:40-42 and cmd/summary-worker/main.go:40-42 now read and apply cfg.MaxTimeRangeDays at boot. Combined with a new MAX_TIME_RANGE_DAYS env var (internal/config/config.go:267, default 90), this turns the previously hard-coded 90 into an operator-configurable knob. That is fine, but please document it (env var, units, blast radius) before shipping — right now it is invisible.

偏离

Migration split for environments that already applied the original all-in-one file. migrations/sql/20260827-01-agent-summary-workspace.sql was rewritten in-place from a heavy ALTER + two CREATE TABLEs into a compatibility stub (SELECT 1;). That is a rewrite of an already-numbered migration, not a new forward migration. For any database that already ran the original 20260827-01 (i.e., any staging/dev box that was pointed at this PR before the split), migration state will say "01 applied" but the new tables will already exist from the old 01 and the stub 02/03/04 IF NOT EXISTS path will silently run. For any fresh database, 01 is a no-op and 02/03/04 create everything. That works, but there is a subtle risk: the Down of 01 was also rewritten from "DROP + MODIFY COLLATE back" to SELECT 1;, so if anyone ever runs Down against a database that got the new all-at-once 01 applied before this rewrite, they get nothing — the destructive DROP that was the previous Down is gone. Worth a loud comment explaining the compatibility intent and noting that Down is intentionally a no-op on both paths (the 02/03/04 Downs are still destructive and note that they are unused in production, which is fine).
Timezone API misuse in tests will break CI. The new tests added in dd32019 construct time.Now() fixtures and compare against timezone.Now() (Asia/Shanghai). On a UTC host the fixtures are written in UTC while runOnce cuts off on timezone.Now(), so any fixture seeded within an 8-hour window of the boundary is classified incorrectly. TestRunOnce_borderline23_9hUntouched and TestRunOnce_activeWorkspaceSessionPreserved both fail in UTC. CI runs on ubuntu-latest with TZ unset → UTC → red. This is a deviation from the existing test pattern (most older tests seed with time.Now() but compare against time.Now()); the new code introduced timezone.Now() in production and the tests did not follow suit.

2. Code Quality verdict

Quality: Changes-Requested

P1-1 — New cleanup tests (and one pre-existing store test) fail in UTC; CI will be red

internal/api/handler/agent_session_cleanup_test.go:136 (TestRunOnce_borderline23_9hUntouched) and :476-482 (TestRunOnce_activeWorkspaceSessionPreserved) seed created_at = time.Now().Add(-23h55m) / time.Now().Add(-48h) (UTC) while runOnce now uses cutoff := timezone.Now().Add(-cleanupAge) (Asia/Shanghai). On a UTC host the seeded times are ~8 hours older in Shanghai wall-clock terms, so the 23h55m-old session falls just past the 24h boundary and is incorrectly cleaned, and the "-48h, expires_at=now+1h" active session test compares updated_at = time.Now()-30d-1h (UTC) against legacyCutoff = tz.Now()-30d and finds it expired. Reproduced locally (TZ=UTC go test -run 'TestRunOnce_borderline23_9hUntouched|TestRunOnce_activeWorkspaceSessionPreserved' → FAIL; TZ=Asia/Shanghai → PASS).
internal/api/handler/agent_workspace_store_test.go:881 (TestAgentWorkspaceStoreReconcileWorkflowErrorOnce) also fails — even in Shanghai TZ. It replays a ClearWorkflow: true error terminalization, and the new code at internal/api/handler/agent_workspace_store.go:818 ("skip re-insert if session.WorkflowTerminalMessageID > 0 && !in.ClearWorkflow") does not also skip the WorkflowTaskID != in.TaskID || WorkflowScopeVersion != in.ScopeVersion guard that runs immediately after (:820). On replay session.workflow_task_id = 0 (first reconcile cleared it) while in.TaskID = 99, so it returns ErrWorkspaceScopeConflict. The previous SHA passed this test.
Fix: in tests, seed timestamps through timezone.Now() instead of time.Now(). In ReconcileWorkflow, move the in.ClearWorkflow short-circuit above the scope-version mismatch check, or clear in.TaskID/in.ScopeVersion along with workflow_task_id on the ClearWorkflow update so subsequent reconciles don't re-check a stale task.

P1-2 — `ReconcileWorkflow` no longer idempotent on error/cancelled terminal states

The logic at internal/api/handler/agent_workspace_store.go:818 is:

if session.WorkflowTerminalMessageID > 0 && !in.ClearWorkflow {
    snapshot, err = loadWorkspaceSnapshotTx(tx, in.Key)
    return err   // short-circuit: success path, terminal already written
}
if session.WorkflowTaskID != in.TaskID || session.WorkflowScopeVersion != in.ScopeVersion {
    return ErrWorkspaceScopeConflict
}

For the successful (StatusCompleted, ClearWorkflow=false) path the short-circuit fires and replays load a fresh snapshot. For StatusFailed/StatusCancelled/deleted-task paths (ClearWorkflow=true, workflow_task_id=0 after the first reconcile), the short-circuit condition is false, execution falls through to the scope-check, session.WorkflowTaskID (0) != in.TaskID (99) → ErrWorkspaceScopeConflict.

handleSummaryWorkspaceHistory is called on every GET /api/v1/agent/chat/history poll (internal/api/handler/agent_summary_workspace.go:991-1024). The first poll after failure writes the error terminal message and clears workflow_task_id; every subsequent poll returns 500. This is exactly the class of bug the P1-4 fix in dd32019 was meant to resolve for the deleted-task case, but the fix was incomplete: it handles the ClearWorkflow=false (success) branch but not the ClearWorkflow=true (error/cancelled/deleted) branch.

P1-3 — Replayed idempotent workflow dispatch only re-triggers the creator, not other participants

internal/service/summary_workflow.go:611-646 (new in dd32019) was added to fix the dropped-dispatch-on-replay problem (prior P1-6). It constructs a WorkerTrigger{Type:"personal_summary", TaskID, ParticipantRefID: creator.ID} if personal.WorkerStatus == PersonalStatusPending. But for team workflows there is a summary_participant row and a personal_result row per invited participant (see createFromAgent, :491-532); a dispatch that crashed after dispatching the creator but before dispatching the other participants leaves those other personal_result rows at WorkerStatus=Pending and they are never retried. The current replay dispatches at most one trigger (the creator). Concretely, a crashed POST after 1 of N participants' dispatches leaves N-1 participants permanently stuck at pending with no sweeper.

The fix also still relies on at least one client retry arriving within the idempotency-key reuse window. There is still no background reconciler/sweeper for PersonalStatus == Pending && created_at < now-5m. P1-6 (dropped-dispatch recovery) is therefore partially fixed, not fully — the single-participant case works, the multi-participant case does not.

P1-4 — The `MAX_TIME_RANGE_DAYS` env knob controls three surfaces with divergent default semantics and no validation

internal/pipeline/MaxTimeRangeDays is now overwritten from env at both API and worker boot (cmd/summary-api/main.go:40, cmd/summary-worker/main.go:40). That single constant is used as:

Workspace contract ceiling (internal/api/handler/summary_workspace_contract.go:237) — 90-day workspace time ranges.
Workspace service ceiling (agent_summary_workspace.go:120 feeds MaxTimeRangeDays as the service's maxTimeRangeDays, passed to ValidatePersonalWorkflow at summary_workflow.go:411).
Worker pipeline hard cap (internal/pipeline/fetch.go:940) for both personal and team tasks — for the legacy HTTP paths the handler now caps at 31 before dispatch, so a 90-day env value does not widen them (good — S2 is partially fixed); but workspace-originated tasks bypass that handler cap and hit the pipeline cap at 90 directly.

There is no validation that MaxTimeRangeDays >= DefaultTimeRangeDays. An operator setting MAX_TIME_RANGE_DAYS=7 while leaving DEFAULT_TIME_RANGE_DAYS=31 will produce a service whose default time range (31d) fails its own validation ceiling (7d) on every request with an implicit range. Add a startup sanity check.

P1-5 — Proposal confirmation still has no version-bump under lock; two concurrent confirms can double-create team tasks

Prior finding P2 ("Proposal-confirm has a lock gap") is not fixed. BeginProposalConfirmation (internal/api/handler/agent_workspace_store.go:660-780) validates pending_proposal_status='pending' and pending_proposal_version = in.ProposalVersion under SELECT ... FOR UPDATE but does not flip status or bump version in that transaction — it only sets active_turn_id. A second confirmation arriving within the 6-minute summaryWorkspaceTurnLease will find active_turn_id held by the first (still running) and return WorkspaceTurnInProgress; but once the first turn commits CompleteTurn and resets active_turn_id = 0 (at :491, after writing pending_proposal_status = "confirmed" and pending_proposal_task_id = T), the second confirm — blocked until then on its own lock wait — proceeds to re-validate pending_proposal_status='pending' … which now says "confirmed", so the guard at :699 (session.PendingProposalStatus != "pending") rejects it with ErrWorkspaceProposalStale. So that case is mostly closed by the transition to confirmed. However: the CompleteTurn update at :440-465 sets pending_proposal_status = "confirmed" only inside if in.Workflow != nil { ... in.Workflow.Scope == "team" { updates["pending_proposal_status"] = "confirmed" ... } }, which fires for completeTeamWorkspaceWorkflow; but before that, the turn body runs (the createTask + participant invitations + worker trigger), and there is a window between the turn lease expiring and CompleteTurn writing confirmed during which a second confirmation with a new Idempotency-Key can pass BeginProposalConfirmation (lease expired, active_turn_id reset) and re-run completeTeamWorkspaceWorkflow with the same proposal — creating a second task. The guard in ConfirmSummaryWorkspaceProposal that re-validates participants (agent_summary_workspace.go:960) uses the request's participants vs the proposal's participants (not the committed state), so it does not detect a second confirmation racing the first. Remedy: inside the BeginProposalConfirmation transaction, atomically set pending_proposal_status = 'confirming' (or bump a version) so only one waiter can proceed past the proposal-valid step regardless of lease expiry.

P2 (non-blocking)

Tests / correctness

TestAgentWorkspaceStoreReconcileWorkflowErrorOnce — fails for the reason in P1-2; the test was not updated when ReconcileWorkflow changed. It is a real regression signal, not a stale test.
testSeed functions use time.Now() not timezone.Now() — seedWorkspaceSession (agent_session_cleanup_test.go:321) and seedMsgInSpace (:47) build rows without explicit TZ. Combined with the production code now reading timezone.Now(), any time-boundary test near the 24h/30d cutoff is TZ-dependent. TestRunOnce_borderline23_9hUntouched is the first victim; future tests will hit the same trap.
TestRunOnce_expiredWorkspaceSessionAndEvidenceCleaned seeds expires_at = time.Now().Add(-time.Hour) and updated_at = now - retention - 1h, so it exercises both expiry predicates but does not test the exclusive-OR cases (a session with non-NULL expires_at in the future but updated_at older than retention should be preserved; a NULL-expires_at session younger than retention should be preserved).
Migration 20260827-02 uses dynamic SQL (PREPARE/EXECUTE) to conditionally run the ALTER. MySQL prepared statements inside a migration executed by sql-migrate work, but note that DDL statements in MySQL implicitly commit (autocommit=1 for DDL regardless of transaction), so the whole-file "transaction wrapper" is a no-op just like before. That was already true of the prior all-in-one file; calling it out because the new comments say "so a failure cannot leave one migration half-applied" — MySQL DDL still commits per-statement, so a failure during the ALTER (e.g. halfway through adding indexes) can leave space_id added but not idx_agent_message_turn. The SELECT 1-or-ALTER conditional itself is a one-shot choice, so replay after a partial failure will attempt the ALTER again and likely hit Duplicate column name 'space_id'. Same class of risk as P1-9 before (admittedly reduced because the slow MODIFY session_id COLLATE was dropped); worth noting.
Migration 20260827-04 conditionally adds the idx_agent_summary_turn_owner_session index with ALGORITHM=INPLACE, LOCK=NONE (migrations/sql/20260827-04-agent-summary-turn.sql:122). For a brand-new table on a fresh install the CREATE TABLE IF NOT EXISTS already includes the index (line 23), so the ALTER ADD KEY is a no-op; for upgrades from the old all-in-one 20260827-01, it adds the composite tenant index. But it does not drop the now-redundant idx_agent_summary_turn_lease that the old schema created. LeaseExpiresAt is still in the model (it is used for lease enforcement, the index is just no longer declared on the struct at internal/model/agent_summary_workspace.go:56) — so on upgraded databases the (status, lease_expires_at) index is an orphan (no GORM model tag, no code query, leftover write amplification), and on fresh installs it never exists. Divergent schemas between environments. Either add a matching conditional DROP INDEX or keep the index tag on the model.
The idempotency-key namespace is still shared across entry points (prior P2). A key reused between POST /summaries (31-day default) and the workspace turn (90-day ceiling) replays whichever was created first. Same collision class, unchanged.
json.Marshal errors are still swallowed on request-hash fingerprints — summary_workspace_contract.go:268 (payload, _ := json.Marshal(...)) and agent_workspace_store.go's payload marshal (payload, _ := json.Marshal(payloadValue), line ~840). Same observation as before.
ScopeJSON is still a bare string against a nullable JSON column (internal/model/agent_summary_workspace.go:19). Same observation as before — currently unreachable because writes go through ScopeJSON: string(in.ScopeJSON) after validation, but a careless Save(&session) will emit scope_json='' and fail on MySQL.
Smart-quote mojibake in space-gate test comments is unchanged (members_auth_test.go, list_space_gate_test.go, etc.). Comment-only, no runtime impact.

Handler / routing

ConfirmSummaryWorkspaceProposal reads c.ShouldBindJSON after MaxBytesReader at internal/api/handler/agent_summary_workspace.go:802-804, which is correct, but note that the body-size cap is applied after parseWorkspaceProposalVersion at :795-800. That only reads the URL path param, so no body has been consumed yet; ordering is fine. Good.
The participantIDs set in sameSummaryWorkspaceParticipants (agent_summary_workspace.go:978-992) is built from left only and checks membership of right, so a duplicate user_id in left + a missing user_id in right passes (because both right IDs are in the set built from left), but the real mismatch — a participant added between proposal and confirm that is not in the proposal — is caught. However, if the client drops a participant and adds another (lengths equal, sets differ), the second loop walks right looking up ids in the left-built set and correctly rejects; but if the client deduplicates by accident (lengths differ) it is caught. Minor: consider sort.Strings + reflect.DeepEqual or build a symmetric diff for clarity. Not blocking.
triggerWorker now returns an error and logs HTTP status (agent_summary_workspace.go:1876-1902) but the call sites (:429-435, :509-515) only log via the goroutine wrapper; the error never reaches the client. Combined with the partial retry fix (P1-3), a failed worker trigger after commit is logged but not surfaced. That is acceptable for a fire-and-forget, but there is still no retry/sweeper.
scheduler.go response status is now logged but errors are still swallowed (internal/worker/scheduler.go:553-555) — same class as the workspace trigger; acceptable for this PR but please add a metric.

Agent loop

Gemini advisory noted: empty WithAllowedSummaryResultTypes slice is fail-closed (rejects all). Confirmed at internal/agent/tool_emit_summary_response.go:175; currently the workspace always passes a non-empty slice (agent_summary_workspace.go:650-658), so this is latent but real for any future caller.
Gemini advisory noted: sanitizeToolProtocolHistory (internal/agent/history.go:106) swallows orphan tool results after a malformed assistant turn. I did not independently verify this but it is consistent with the loop shape (i = j advances past both the assistant turn and subsequent tool results without checking they belong to it). Flag for author attention; severity depends on how often tool results get interleaved in production, which I cannot verify statically.

3. Status of prior-review findings (for reference)

Prior	Status at `dd32019`
S1 (proposal gate unreachable)	Fixed. `summary_route_policy.go:116` now returns `SummaryRouteTeamConfirmation`; `completeWorkspaceProposal` is reached; `ConfirmSummaryWorkspaceProposal` now re-loads + re-validates persisted scope (`agent_summary_workspace.go:870-885`).
S2 (31→90 widening on legacy endpoints)	Fixed for legacy HTTP + bot, not for workspace. See §1 漏建.
S3 (dead subsystem)	Partially fixed. Proposal flow now wired. Unreachable items listed in §1 超建 remain. Leftover scaffolding block at `agent_summary_workspace.go:2180-2184` remains.
S4 (doc stale)	Not re-verified; `docs/unified-summary-safe-track.md` still says deferred items land later but several now ship in this PR. Low priority, doc-only.
P1-1 (team task from any chat sentence, no confirmation)	Fixed by routing to `SummaryRouteTeamConfirmation` instead of `SummaryRouteTeamWorkflow`.
P1-2 (space-boundary bypass)	Fixed. Explanation now returns after source validation (`summary_route_policy.go:87-93`); `fetch_channel`/`peek_channel` pass `pipeline.WithSpaceID(spaceID)` (`tool_fetch_channel.go:135-137`, `tool_peek_channel.go:111-113`); allowlist is built after validation.
P1-3 (AuthorizeDiscoveredChannels replace-vs-union)	Fixed. `replace()` removed; `scope.add(grants, uid)` used at `types.go:240`; union test added (`channel_scope_test.go:38-48`).
P1-4 (deleted completed summary → 500)	Partially fixed. Handle now uses `Unscoped()` lookup + a `MessageID` parameter; but the `ClearWorkflow=true` path still fails the idempotency guard on replay (see P1-2).
P1-5 (unbounded transcript read in tx)	Fixed. `loadWorkspaceSnapshotTx` does `Order("id DESC").Limit(maxHistoryRows)` then reverse (`agent_workspace_store.go:588-592`).
P1-6 (idempotent replay can't recover dropped dispatch)	Partially fixed. Replay re-issues a WorkerTrigger for the creator participant; multi-participant redispatches still missing (P1-3).
P1-7 (no length cap / no body limit)	Fixed on the workspace path. `maxAgentChatRequestBodySize = 512KiB` applied to both chat endpoints and confirm (`agent_chat.go:80, 591, 889`, `agent_summary_workspace.go:802`); per-field rune caps added (`summary_workspace_contract.go:35-37, 180, 203, 220-222, 234`); validation error no longer echoes `chat_type` (`:183`).
P1-8 (no retention reaper; expires_at written by nothing)	Fixed. `expires_at` is written on every session create/turn/complete/fail/reconcile/save (`agent_workspace_store.go` passim); `cleanupExpiredSummaryWorkspaces` added (`agent_session_cleanup.go:235-281`); idempotency tombstone GC added. One correctness caveat: test seeds don't match the new clock source (see P1-1).
P1-9 (migration ALTER TABLE agent_message COPY rebuild)	Fixed. The `MODIFY COLUMN session_id COLLATE utf8mb4_0900_bin` (the clause that forced COPY rebuild) has been removed from `20260827-02`; the new migration uses `ALGORITHM=INPLACE, LOCK=NONE`; the big ALTER is split into 02 (columns+indexes), 03 (session table, `CREATE TABLE IF NOT EXISTS`), 04 (turn table, `CREATE TABLE IF NOT EXISTS` + conditional index). See remaining caveats in §2 P2 (migration `20260827-01` Down compatibility, orphan `idx_agent_summary_turn_lease`).
P1-10 (max 31→90 widening)	Same status as S2 (see above).
P2 — time.Now() vs timezone.Now()	Mostly fixed. Bulk conversion across `agent_workspace_store.go`, `agent_summary_workspace.go`, `agent_summary_save.go`, `agent_session_cleanup.go`. One straggler to double-check: test seed helpers still use `time.Now()`.
P2 — validateAgentWorkflowParticipants empty UserID	Fixed. Added at `summary_workflow.go:260-267`.
P2 — empty participant IDs	Fixed. See previous.
P2 — dead condition in route (HasValidSource always true)	Not fixed — `summary_route_policy.go:118` (now at :118 after edits) still checks `in.HasValidSource` in the personal-workflow branch after the guard at :88 returned for the invalid case. Harmless.
P2 — proposal lock gap	Not fixed. See P1-5.

4. What I would like a human to verify before merge

Was a 90-day workspace time-range ceiling an approved product decision? It now applies to both workspace-originated personal and team summaries (via the worker pipeline cap at internal/pipeline/fetch.go:940). The legacy HTTP/bot surfaces are back to 31 (fixed correctly); the workspace surfaces are at 90.
Migration compatibility for databases that already applied the original all-in-one 20260827-01. I read the IF NOT EXISTS/information_schema.COLUMNS guards and they look correct for both "already upgraded" and "fresh install" paths, but this is exactly the kind of thing that needs one staging rollout before prod. Please confirm the rollout plan: is there any deployed environment that already ran the original 01, and will 20260827-02 correctly skip the ALTER on those?
Orphan idx_agent_summary_turn_lease on upgraded databases (see §2 P2). Either drop it conditionally in 20260827-04 or keep the GORM index tag so all environments converge.
ReconcileWorkflow error-path idempotency (P1-2). This is an actual bug introduced by this hardening commit and is the easiest thing to miss because the success path was tested but the error path was not.

5. Additional observations

The space-boundary hardening is the strongest part of the fix commit. Combining (a) moving the Explain intent after the source/participant/reference guards, (b) threading WithSpaceID into both content-reading tools, and (c) building the agent-channel allowlist from the post-materialization, post-validation contextValue closes the prior P1-2 end-to-end. The defensive addition of validateExplicitSourceCoverage in the worker pipeline (internal/pipeline/fetch.go:446-468) is defense-in-depth the original PR did not have and is good work.
Worker trigger error propagation is improved: non-2xx responses are now logged both from the workspace coordinator (agent_summary_workspace.go:1898) and the scheduler (scheduler.go:554). Still fire-and-forget, but now observable.
The retention reaper is well-batched (workspaceCleanupBatchSize = 200, agent_session_cleanup.go:183) and deletes in dependency order (citation manifests → evidence artifacts → specs → runs → evidence → messages → turns → session) inside a single transaction per batch. Good.

Coverage note

No MySQL instance was available. Migration ALGORITHM/LOCK claims in 20260827-02 and 20260827-04 are derived from MySQL 8.0 docs and the absence of MODIFY COLLATE; they were not measured against a real server. The orphan idx_agent_summary_turn_lease observation depends on the old 20260827-01 CREATE TABLE having been applied in production — if no such deployment exists, this is moot.
Tests were run with CGO enabled (internal/agent's //go:build cgo tests pass). Three tests fail (P1-1, P1-2); all other packages pass.
octo-deployment was not consulted. Whether migrations run as a pre-deploy Job (isolating the migration-lock crash-loop risk) determines whether the 30-second GET_LOCK + Fatalf is still a rolling-deploy hazard even after the COPY-rebuild fix.
go test -race was not run.
No end-to-end server run. P1-2, P1-3, and P1-5 were traced statically through handler → store → worker and confirmed with test runs; none were reproduced against a live stack.

wanyiwang06 added 3 commits August 27, 2026 17:28

feat(summary): add unified workflow safe-track foundation

05d39b8

feat(summary): complete unified workspace backend

97d6624

feat(summary): complete unified agent routing and scope handling

2c0dcf2

wanyiwang06 requested a review from a team as a code owner September 1, 2026 07:42

yujiawei added the needs-human-review label Sep 1, 2026

github-actions Bot added the size/XL PR size: XL label Sep 1, 2026

Jerry-Xin requested changes Sep 1, 2026

View reviewed changes

yujiawei requested changes Sep 1, 2026

View reviewed changes

mochashanyao previously approved these changes Sep 1, 2026

View reviewed changes

fix(summary): harden unified workspace integration

dd32019

wanyiwang06 dismissed mochashanyao’s stale review via dd32019 September 1, 2026 12:33

Jerry-Xin requested changes Sep 1, 2026

View reviewed changes

mochashanyao requested changes Sep 1, 2026

View reviewed changes

yujiawei requested changes Sep 1, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat(summary): add unified summary workspace backend - #234

feat(summary): add unified summary workspace backend#234
wanyiwang06 wants to merge 4 commits into
Mininglamp-OSS:mainfrom
wanyiwang06:feat/unified-summary-local-integration

wanyiwang06 commented Sep 1, 2026

Uh oh!

Jerry-Xin left a comment

Uh oh!

yujiawei left a comment

Uh oh!

mochashanyao left a comment

Uh oh!

yujiawei commented Sep 1, 2026

Uh oh!

Jerry-Xin left a comment

Uh oh!

mochashanyao left a comment

Uh oh!

yujiawei left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

wanyiwang06 commented Sep 1, 2026

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

🔴 Blocking

💬 Non-blocking

✅ Highlights

Uh oh!

yujiawei left a comment

Choose a reason for hiding this comment

Code Review — PR #234 (octo-smart-summary)

1. Scope / specification compliance

S1 — Missing: the mandated team-creation confirmation gate is unreachable

S2 — Over-build: the maximum time range was tripled on three endpoints this PR does not claim to touch

S3 — Over-build: a substantial unreachable subsystem ships as if it works

S4 — Divergence: the doc added by this PR is already stale at HEAD

2. Code quality

P1-1 — A team summary task is created, and other users are invited, from an ordinary chat sentence

P1-2 — The explanation route bypasses source validation, and the two content-reading tools are space-blind

P1-3 — AuthorizeDiscoveredChannels replaces the allowlist instead of extending it, contradicting its own doc comment

P1-4 — Deleting a completed summary permanently 500s the workspace session

P1-5 — The whole session transcript is re-read, unbounded, inside every locked transaction

P1-6 — An idempotent replay cannot recover a dropped worker dispatch

P1-7 — No length cap on any summary_context string, and no request-body limit on the route

P1-8 — Workspace state has no retention path, and the new evidence guard depends on a retirement step that does not exist

P1-9 — Migration 20260827-01 is a full-table COPY rebuild of agent_message, unannotated

P1-10 — Max time range 31 → 90 on three out-of-scope surfaces

P2 (non-blocking)

Findings raised during review and rejected after verification

3. What I would like a human to verify before merge

4. Additional observations outside the review scope

Coverage note — what this review did not verify

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

Code Review — MR !15 (octo-card-forge)

Summary

Verification

Findings

P1 — 两个作业直推同一生产分支，可见 diff 内无串行化或 rebase-retry，引入新的 push 冲突失败模式（.gitlab-ci.yml:281、:312）

P2 — argocd_sync 移除同步前唯一机器校验，人工闸门从此无交叉核对（.gitlab-ci.yml:331，旧 346–350 行被删）

P2 — image: $CURL_IMAGE 的定义不在 diff 内，需确认（.gitlab-ci.yml:323）

Human-verify

数据流回溯（逐被消费数据）

盲点 checklist（C1–C6）

跨轮 blocker 复检（R6）

Things I checked that are fine

Verdict: CHANGES_REQUESTED

附录 A — 数据流回溯

附录 B — 盲点 checklist（security_sensitive 全项）

附录 C — 跨轮 blocker 复检（R6）

automated review建议（供终审参考）

Uh oh!

yujiawei commented Sep 1, 2026

Addendum to the review above — two more findings on the same code path (head 2c0dcf2, unchanged)

A1 (P1) — DEFAULT_TIME_RANGE_DAYS silently becomes a no-op, and the new ceiling has no knob at all

A2 (P2) — an explicitly selected source that fails the new space filter is silently dropped, with no failure path

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

Prior blockers — resolution status

🔴 Blocking on this head

💬 Non-blocking

✅ Highlights

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

1. 验证结论

2. 发现问题（diff-scope 三问 + 定级）

P1 — 证据清理 DELETE 跨排序规则比较，MySQL 上每个 tick 必然失败（internal/api/handler/agent_session_cleanup.go:175）

P2 — scope_version 跳变静默丢弃在途 workflow 的会话指针（internal/api/handler/agent_workspace_store.go:236）

P2 — MySQL 专属契约只被 SQLite 测试覆盖（migrations/sql/20260827-03-agent-summary-session.sql:11）

3. 建议

4. 额外发现（Human-verify，均非本 PR 阻塞）

5. 数据流回溯

6. 盲点 checklist（security_sensitive，全项）

7. 跨轮 blocker 复检（R6）

Code Review — PR #234 (octo-smart-summary)

P1-2 — The `explanation` route bypasses source validation, and the two content-reading tools are space-blind

P1-3 — `AuthorizeDiscoveredChannels` replaces the allowlist instead of extending it, contradicting its own doc comment

P1-7 — No length cap on any `summary_context` string, and no request-body limit on the route

P1-9 — Migration `20260827-01` is a full-table COPY rebuild of `agent_message`, unannotated

P1 — 两个作业直推同一生产分支，可见 diff 内无串行化或 rebase-retry，引入新的 push 冲突失败模式（`.gitlab-ci.yml:281`、`:312`）

P2 — argocd_sync 移除同步前唯一机器校验，人工闸门从此无交叉核对（`.gitlab-ci.yml:331`，旧 346–350 行被删）

P2 — `image: $CURL_IMAGE` 的定义不在 diff 内，需确认（`.gitlab-ci.yml:323`）

Addendum to the review above — two more findings on the same code path (head `2c0dcf2`, unchanged)

A1 (P1) — `DEFAULT_TIME_RANGE_DAYS` silently becomes a no-op, and the new ceiling has no knob at all

P1 — 证据清理 DELETE 跨排序规则比较，MySQL 上每个 tick 必然失败（`internal/api/handler/agent_session_cleanup.go:175`）

P2 — scope_version 跳变静默丢弃在途 workflow 的会话指针（`internal/api/handler/agent_workspace_store.go:236`）

P2 — MySQL 专属契约只被 SQLite 测试覆盖（`migrations/sql/20260827-03-agent-summary-session.sql:11`）

P1 — Evidence-cleanup DELETE mixes collations and will fail on MySQL every tick (`internal/api/handler/agent_session_cleanup.go:175`)

P2 — Scope bump silently drops a running workflow's session pointers (`internal/api/handler/agent_workspace_store.go:236`)

P2 — MySQL-only contracts are exercised only against SQLite (`migrations/sql/20260827-03-agent-summary-session.sql:11`)

P1-2 — `ReconcileWorkflow` no longer idempotent on error/cancelled terminal states

P1-4 — The `MAX_TIME_RANGE_DAYS` env knob controls three surfaces with divergent default semantics and no validation