* fix(mcp): honor explicit task_id in plan/walkthrough/review tools
resolveTaskID silently discarded the caller-supplied task_id whenever the
MCP server was session-bound (every agent session) and substituted the
caller's own task. A parent reading a child's plan got the parent's empty
plan back ("No plan exists"), and a cross-task create/update would have
written to the wrong task — all with no error, despite the tool schemas
advertising task_id as a usable parameter.
Honor an explicitly provided task_id and fall back to the session-bound
task only when the argument is absent, matching how message_task_kandev /
stop_task_kandev already address other tasks. Cross-task access stays safe:
internal/mcp/scope attaches the stream owner's identity (from the agent's
own execution, never the payload), and the plan/walkthrough/review services
gate the target task_id through the same workspace-ownership check every
other cross-task surface uses — an out-of-reach task is rejected, not
served. The review service gains the SetTaskAuthorizer wiring the plan and
walkthrough services already had, so publish_review_findings is covered too.
Tool descriptions now document the default-to-current / cross-task-within-
reach behavior. Tests cover the resolution precedence (explicit wins /
fallback to bound / neither errors), a session-bound-to-A reads/creates a
plan for B regression, and the review authorizer allow/deny paths.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(mcp): drop mcp.Required() from task_id on session-defaulting tools
The plan, walkthrough, and publish-review tools default task_id to the
session-bound task when it is omitted, so advertising it as required
contradicted that documented behavior (flagged by Codex and CodeRabbit on
the PR). Mark task_id optional on all eight; handler behavior is unchanged.
The genuinely cross-task tools (update/move/delete/archive_task,
message_task, get_task_conversation) keep task_id required.
Adds a schema test asserting task_id is present-but-optional on every
session-defaulting tool.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(mcp): cover review scoping + walkthrough/review tool-desc reach
Addresses two non-blocking review findings:
- Extend TestPlanAndWalkthroughScoping to exercise ReviewService through the
assembled AuthorizeTaskAccess → SetTaskAuthorizer → PublishFindings path, so
all three task-scoped services are covered uniformly (foreign owner denied
with ErrTaskNotFound; real owner passes and the finding is stored).
- Document the default-to-current / cross-task-within-reach semantics in the
top-level descriptions of the walkthrough and publish-review tools (was only
in the task_id parameter description), since LLMs read the tool description
first. Broaden the description test to assert all eight session-defaulting
tools carry it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(mcp): assert review finding persists through the auth gate
Follow-up to review: query repo.ListTaskReviewFindings after the owner
publish in TestPlanAndWalkthroughScoping and assert exactly one finding was
stored, so the test verifies the write landed in the repository rather than
only the service's returned slice.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(plugins): implement Host data API write RPCs (CreateTask/UpdateTask/CreateComment)
The api_write manifest capability was a stored no-op. Wire the deferred
phase-2 write RPCs (ADR 0043) so closed-loop plugins (Sentry->task,
Slack slash-command task creation, refactor fan-out) can create and
update tasks and post comments.
- SDK: Host.Tasks() gains Create/Update; new Host.Comments() writer;
CreateTaskInput/UpdateTaskInput/Comment DTOs (pkg/pluginsdk).
- Gating: api_write:tasks gates Create/UpdateTask, api_write:comments
gates CreateComment; undeclared -> gRPC PermissionDenied, per-method
and independent of the read caps on the same resource.
- Writes route through the first-party service layer (task service for
tasks, office service for comments) via backendapp adapters, never a
repository -- so task.*/comment-created events fire and WS updates.
Server stamps source="plugin:<id>"; CreateTask resolves workspace/
workflow defaults and best-effort honors start_agent; UpdateTask uses
a conservative title/description/state/workflow_step_id mask.
- Fixture plugins exercise a CreateTask+CreateComment round-trip;
in-process, real-broker wire, and subprocess tests cover the round-trip
and per-capability denial.
- Docs: spec, GRPC-CONTRACT, HOST-DATA-API.proto, proto comments moved
writes deferred -> implemented; ADR 0043 amended with the concrete
service-layer routing decision.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(plugins): send a message to a task session via Host SendMessage (replaces CreateComment)
The office TaskComment write was the wrong primitive — it's an office/
dashboard construct, not a way to message the agent working a task, and
it only did anything under office reactivity. Replace the CreateComment
write RPC with SendMessage, which delivers a prompt to a task session
through the orchestrator's real delivery path (the same one message_task
uses).
- Proto: swap CreateComment/Comment for SendMessage(SendMessageRequest)
returns SendMessageResponse{session_id, status}; regenerated stubs.
- SDK: Host.Comments() removed; Host.Messages().Send(taskID, sessionID,
text) added (pairs with Messages().List), returning MessageDispatch.
- Gating: api_write:comments dropped; SendMessage gated api_write:messages
(independent of api_read:messages on the same accessor, like Tasks()).
- Delivery: a new pluginsTaskMessengerAdapter (backendapp) resolves the
target session (explicit id verified against the task, or the primary
session), records a user message stamped source="plugin:<id>", then
dispatches by state — running → queue; idle/completed → prompt (resume
if the agent process is gone); never-started → launch; failed dispatch
deletes the recorded message. Wired via SetWriteDeps (live-read).
- Tests: adapter state-machine unit tests (queue/start/prompt/resume/
delete-on-failure/belongs-to-task/no-primary/terminal); SDK, in-process
gating, wire, and subprocess round-trips updated to SendMessage.
- Docs: spec, GRPC-CONTRACT, HOST-DATA-API.proto, proto comments, and the
ADR 0043 amendment updated from comment-create to message-send.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(plugins): address PR review — nil guards, detached contexts, state validation
Follow-up fixes from the #1970 review (Claude + CodeRabbit bots):
- grpcHostServer.CreateTask/UpdateTask/SendMessage: nil-guard the returned
value before toProto() so a custom/test Host returning (nil, nil) yields a
gRPC Internal error, not a server panic (matches GetTask's pattern).
- host.go: fix stale server doc comment (Comments()/CreateComment →
Messages()/SendMessage).
- startTaskBestEffort and the post-resume wait+retry now run on
context.WithoutCancel(ctx): the task/resume is already committed, so a
plugin's incoming deadline must not silently abort the launch/resume.
- pluginsTaskWriterAdapter.UpdateTask validates the state string against the
known task-state enum (excluding orchestrator-owned SCHEDULING) and returns
InvalidArgument, so a plugin typo can't forward a bogus state to the service.
- Tests: add the resume-wait-timeout path (message deleted so a retry can't
duplicate), and a dedicated pluginsTaskWriterAdapter test covering
source→metadata mapping and state validation (both adapters now depend on
narrow interfaces for testability).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(plugins): launch start_agent asynchronously so CreateTask doesn't block
Review nit follow-up: startTaskBestEffort called StartTask synchronously, so
with the detached context the plugin's CreateTask RPC blocked on the
orchestrator's full inline launch path (office lookup, state update, workflow
step move, task fetch) with no bound — and the "asynchronous" comment was wrong.
Move the fire-and-forget into pluginsTaskStarterAdapter (backendapp, no goleak),
mirroring MCP's launchAutoStartTask: the launch runs in a goroutine on a
context.WithTimeout(context.WithoutCancel(ctx), AgentLaunchTimeout), and
StartTask returns immediately. The plugin host stays a simple synchronous call
(its taskStarter contract now documents "returns promptly, launches async").
Adds adapters_plugin_starter_test.go: launch invoked, returns before the launch
completes, detaches from a cancelled caller context, and swallows launch errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(plugins): document write validation + SendMessage rollback; harden starter test cleanup
Addresses the final PR #1970 review nits:
- adapters_plugin_starter_test.go: register the block-channel release via
t.Cleanup (sync.Once-guarded) before the goroutine can block, per AGENTS.md.
- GRPC-CONTRACT.md: document CreateTask/UpdateTask validation + error rules
(title required, id required, NotFound on missing task, invalid-state reject).
- spec.md: document that a failed SendMessage dispatch deletes the recorded
message, plus the required-field/NotFound/terminal-session rules.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(automations): reliable cron scheduling and webhook-driven push/CI triggers
Automations were "wounded": cron schedules mishandled timezones and some
never fired, and the GitHub push/CI trigger types were coming-soon stubs.
Cron (fix):
- The scheduler reduced every cron expression to a fixed interval derived
only from a */N step in the minute/hour field, so any pinned schedule
(0 9 * * *, 30 14 * * *, weekday ranges, day-of-month) never fired, and
the configured timezone was parsed but never used.
- Replace the interval approximation with robfig/cron/v3: shouldFire now
computes the true next fire time in the configured timezone (DST-aware,
defaulting to UTC for host-independence), anchored to last-evaluated or
creation time. Dead interval-parsing helpers and shorthand constants
removed.
GitHub push + CI triggers (feat):
- Implement push and CI-failure triggers end to end via the existing
per-workspace GitHub App webhook plumbing (#1810): the HMAC-verified
webhook service now parses push and completed check_run events, resolves
the installation to its workspaces, and publishes normalized bus events.
- A new automation subscriber matches enabled github_push / github_ci
triggers by workspace + repo + branch/conclusion/check-name filters and
fires them (deduped by commit SHA / check-run id). The old poll-based
stub evaluators are removed; the PR poll path is unchanged.
- Subscribe the App manifest to push/check_run and enable both trigger
types in the registry (CI defaults to the failure conclusion).
Docs:
- Update docs/public/feature-status.md and automation-and-mcp.md so the
cron and push/CI rows reflect shipped reality.
Tests: regression tests pin the previously-never-firing cron shapes,
timezone/DST behavior, and host-independence; backend tests cover the
webhook parse->publish and subscriber match/fire/dedup flows for both
push and CI-failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(automations): populate push.message and ci.url placeholders in fired trigger data
Address PR review blockers: the default push and CI automation prompts
reference {{push.message}} and {{ci.url}}, which the interpolator maps to
data["message"] / data["html_url"] — but the webhook subscriber never
populated those fields, so both always rendered empty.
- Parse head_commit.message (push) and check_run.html_url (CI) in the
GitHub webhook service and carry them on the published bus payloads.
- Include "message" / "html_url" in the fired trigger data so the
placeholders resolve; extend the webhook and subscriber tests to assert
both flow through end to end.
Non-blocking review cleanups:
- Replace the misleading reuse of matchesAuthors for CI conclusions/check
names with a clearly-named matchesFilterValue helper.
- Drop dead context.Background() line and its unused import from the cron
regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(automations): scope webhook triggers to active installs and per-branch dedup
Address CodeRabbit review on the push/CI webhook triggers:
- Security: resolveWorkspaceIDs now only returns connections whose status is
active, so a suspended or revoked GitHub App installation can no longer fire
push/CI automations.
- Correctness: include the branch in the push dedup key
(push:owner/name@branch@sha) so the same commit pushed to two matching
branches fires once per branch instead of being suppressed.
- Correctness: add an optional head-branch filter to github_ci triggers and
gate CI matching on it, matching the push trigger's branch-scoping.
- Stability: make the webhook subscriber's Start retryable — a subscription
failure rolls back partial subscriptions and leaves the subscriber
un-started so a later Start re-attempts, instead of permanently disabling
push/CI processing.
- Tests: cover multi-connection workspace resolution (dedup + inactive
filtering), per-branch push dedup, and CI branch filtering.
- Docs: note the CI branch filter and per-branch push dedup.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(automations): cover webhook subscriber Start retry-after-failure path
Add TestGitHubWebhookSubscriberStartRetryableAfterFailure with a minimal
flaky bus whose second Subscribe fails, pinning the retry contract: a
subscription failure rolls back the partial subscription and leaves the
subscriber un-started, and a later Start re-attempts and succeeds.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(worktree): export executor-profile env into repo setup script
Executor-profile environment variables (with secrets revealed) were merged
into the launch request env and reached the agent subprocess, but the
repository setup script — run inside worktree.Manager.Create, where
`npm install` typically runs — was built with only the install-managed env
(GOCACHE). The profile env was never injected, so tokens like an npm auth
token were unavailable during install. This affected both single-repo and
multi-repo tasks, since both run their setup script through Create.
Thread the resolved profile env into the setup script at the point both
paths converge: add CreateRequest.ScriptEnv (transient, never persisted so
secrets stay out of the DB), set it from req.Env in buildWorktreeCreateRequest
(used by single-repo and per-repo in multi-repo), and merge it over the
managed env in runWorktreeSetupScript. Managed GOCACHE keeps precedence.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(worktree): avoid overflow-flagged alloc size, clarify env precedence
Address PR review: drop the len(a)+len(b) capacity hint in mergeScriptEnv
(CodeQL flagged the addition as a possible allocation-size overflow) and
reword the ScriptEnv doc comment to state that install-managed values such
as GOCACHE take precedence over profile values on key collisions.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(worktree): round out mergeScriptEnv edge cases
Add profile-only and managed-only assertions to TestMergeScriptEnv per PR
review, making the table exhaustive alongside the existing (nil,nil) and
(profile+managed) cases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(plugins): add auth capability for plugin-provided OIDC/SAML login
Plugins can now authenticate a visitor against an external IdP (OIDC/SAML)
and contribute a login button to the pre-auth screen:
- new `auth` manifest capability + a capability-gated X-Kandev-Auth-Login
webhook-response directive; the host mints the kandev_session (ADR 0050),
the plugin never receives the raw token. Plugin-set Set-Cookie is dropped.
- auth.Service.AuthenticateExternal: existing-identity / link-by-email / JIT
member provisioning; refuses to auto-link to an admin; verified-email is
the plugin's contract.
- auth_providers manifest block + plugins.SSOProviders() populate the
anonymous boot payload (auth.ssoProviders); the login page renders a
"Continue with <provider>" button per provider.
- ADR 0050 + plugin-manifest / plugin-authoring / auth spec docs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: exclude publish_review_findings_kandev from public-docs coverage
The native code-review agent (#1957) added the publish_review_findings_kandev
MCP tool but did not update docs/public/coverage.json; that PR did not touch
docs/public so the path-filtered validator never ran. It's an internal review
reporting tool, not a user-facing feature, so record it as an exclusion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(plugins,auth): address review on SSO directive and JIT provisioning
- takeAuthLoginDirective strips every case variant of X-Kandev-Auth-Login and
rejects (403) when a plugin sends more than one, so no variant leaks to the
browser and login never uses a nondeterministically-picked assertion.
- provisionExternalUser rolls back the just-created user when the identity
insert fails (transient error or a lost UNIQUE(provider,subject) race) and
resolves to the winning identity, so no account is left without a login and
no email is reserved by an unusable row. Adds userstore.DeleteUser.
- auth spec: mark the login-page SSO buttons + provider discovery as landed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: drop redundant coverage exclusion; main now covers publish_review_findings_kandev
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(review): navigate to code-review findings from the Changes panel
The findings count in the review top bar was a passive badge, so a user
had to scroll the diff to hunt for each inline finding. Turn it into a
navigator: it opens a popover listing every open finding grouped by file
(most-severe first), and clicking one jumps to the finding's card in the
diff, scrolls it into view, and briefly flashes it.
- lib/review/navigation.ts: navigateToFinding selects the finding's file,
emits an event, then polls across frames (files render lazily) for the
card and scrolls + flashes it. Reuses the composite <repo>\x00<path>
key so cross-repo same-named files stay distinct.
- review-findings-overview + review-findings-button: the grouped list and
its click-to-open popover trigger, mirroring the FixCommentsButton /
ReviewCommentsOverview pattern. The badge moves out of ReviewRunButton
into the top bar, keeping the review-open-count testid and copy.
- inline-review-finding: stamps data-review-finding-id + scroll margin so
navigation can target the card.
- unanchored-findings-banner: auto-expands when a navigation targets one
of its stale findings so that card renders and can be scrolled to.
- globals.css: review-finding-flash highlight, respecting reduced motion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(review): harden finding navigation from code-review feedback
Address automated review of the findings navigator:
- navigation.ts: scope the finding lookup to the open review dialog so a
duplicate card rendered by the Changes panel behind it is not scrolled
to instead; re-emit the navigate event each poll frame so a lazily
mounted stale-findings banner still expands (a one-shot event fired
before the section rendered was missed); honor prefers-reduced-motion
for the scroll, matching the flash animation.
- review-dialog-surface: clear the sidebar file filter before selecting a
finding's file, so a finding in a filtered-out file still mounts.
- review-findings-button: surface a warning when the card can't be reached
instead of discarding the promise silently.
- review-findings-overview: compute the file directory once per row.
- Tests: unit coverage for the banner auto-expand and the findings button
(null render + navigate), plus a mobile-* e2e that taps the navigator
and verifies the target card is reached.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Delete unreachable/unused backend symbols confirmed by staticcheck U1000
and golang.org/x/tools/deadcode, plus deprecated back-compat aliases kept
alive only by their own tests.
Removed (unused):
- parseGitHubRepoURL, syncTaskStateForBulkWorkflowMove (task/service)
- resolvePrimaryRepoInfo (orchestrator/executor)
- resolveWorkflowAgentProfile, workflowDefaultAgentProfile wrappers (mcp/handlers)
- cleanupExitedContainer (runtime/lifecycle) + orphaned fmt import
- VscodeManager.setEnv (agentctl/server/process)
- repoRootDir const (cmd/preview)
Removed (deprecated shims):
- Canceller.CancelSessionAndNotify alias -> callers use DetachSessionAndNotify
- ScriptPlaceholder type alias (live scriptPlaceholders var kept)
- Placeholder{QueueRun,ClearDecisions,QueueRunForEachParticipant}Callback
no-op structs and their self-referential test
No behavior change; build, vet, and affected-package tests pass.
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(auth): scope integration config/watch routes to workspace owner
The third-party integration route group is guarded by its own
query-only middleware (integrationWorkspaceScopeMiddleware), which
authorizes only the workspace_id query param and falls through when it
is absent. Two shapes bypassed it entirely under enforced auth:
- POST /{jira,linear,slack}/config/copy takes the copy TARGET from the
request body (targetWorkspaceId), which no layer checked. A caller
could copy their own connection settings + credential into another
user's workspace by naming its ID, after which that workspace's
identity-less poller would run against attacker-controlled config
(Jira/Linear issue watchers auto-creating tasks from attacker issues;
Slack bot token/webhook). CopyConfigToWorkspace now authorizes BOTH
source and target up front, before the pre-write UpdateAuthHealth
side effect. Mirrors github/copy.go.
- GET /{jira,linear}/watches/issue with workspace_id omitted returned
ListAllIssueWatches across every workspace (JQL/filters, repo, agent
and executor profile IDs, spawn prompt). It now filters to the
caller's workspaces; an identity-less internal caller still sees all,
preserving poller use.
- The config Get/Set/Delete entry points resolved an unowned global
"default" workspace when workspace_id was omitted; they now
resolve+authorize via resolveWorkspaceID.
Each service gains SetWorkspaceAuthorizer(taskSvc.AuthorizeWorkspaceAccess),
wired in backendapp; nil (unit tests, auth disabled) stays unscoped.
Denials surface repoerrors.ErrWorkspaceNotFound, mapped to 404 by the
handlers (no existence leak). Recorded the query-only-middleware rule in
integrations/AGENTS.md, including that Sentry and GitLab's
ListAllIssueWatches still need the same filter.
Regression tests added per package and mutation-verified: dropping only
the copy up-front guard fails on the victim health-flip witness;
dropping both guards fails on the config/secret witness.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(auth): scope integration watch-write and data-plane paths too
Review of the prior commit (Codex + Claude, converging) found the same
body/query workspace-bypass class in surfaces the first pass missed:
- CreateIssueWatch / UpdateIssueWatch (Jira, Linear) never authorized the
workspace. CreateIssueWatch takes WorkspaceID from the request body, so
a caller could plant a persistent watch in another user's workspace that
repeatedly spawns tasks with that user's credentials and an
attacker-supplied filter/prompt. Now authorize req.WorkspaceID (create)
and the loaded row's workspace (update). The by-ID mutation/trigger/reset
handlers were already guarded by assertWatchInWorkspace + the
query-authorizing middleware; the service-layer guard adds defense in
depth and covers non-HTTP callers.
- TestConnectionForWorkspace (Jira, Linear, Slack) resolved the default
workspace with no auth, so an omitted workspace_id + attacker-supplied
siteUrl could exfiltrate that workspace's stored token. Now authorizes
after normalize and returns a hard error (404), not a swallowed
{OK:false}.
- Data-plane reads funnel through clientFor, which now resolves+authorizes
the workspace — scoping search/projects/statuses in one chokepoint.
- filterIssueWatchesByAccess treated ANY authorizer error as a denial,
silently dropping watches on a transient failure and returning a
truncated 200. It now drops only on ErrWorkspaceNotFound and propagates
everything else.
Denials map to 404 via the shared writeClientError / writeIssueWatchError
sinks and the two list/test handlers. Renamed the test helper
denyExcept -> denyOnly (it denies exactly the named workspaces). Added
per-package regression tests for create/update/list denial, authorizer
error propagation, connection-test denial, and data-plane denial;
mutation-verified the CreateIssueWatch guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(review): persist and publish native code-review findings
Adds the backend core for reviewing a task's working diff before a PR
exists: task-scoped review runs and anchored, advisory findings that the
Changes/Review panel can render as inline comments.
- task_review_runs / task_review_findings schema, models, and repository,
with supersede-on-re-review and a restart sweep that cancels in-flight
runs rather than leaving them looking alive.
- ReviewService as the single write path, publishing bus events that the
WS gateway forwards to the client.
- internal/review: reviewer-identity resolution (agent profile > code-review
utility agent > user default, failing closed on passthrough-only profiles),
change-set collection, prompt batching, response parsing, anchor
extraction, and run orchestration.
- publish_review_findings_kandev MCP tool plus the task.review.* WS actions.
- Go djb2 that matches apps/web/lib/utils/hash.ts, asserted from shared
fixtures on both sides so stale detection agrees across the wire.
Spec: docs/specs/native-code-review/spec.md
Plan: docs/plans/native-code-review/plan.md
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(review): add run_code_review workflow step and wire the review runner
Makes a review pass triggerable both ways and actually runnable end to end.
- run_code_review on_enter action across models, engine compile, and an
orchestrator callback that logs rather than propagates failures, so a task
never gets stuck in a step because no reviewer was configured.
- The step's optional agent_profile_id is what lets a step review with a
different model than it implemented with; export/import swap it for a
portable agent name/model/mode so it survives a cross-workspace round trip
instead of carrying a dangling id.
- Concrete adapters for reviewer identity, changed-file collection, prompt
rendering from the code-review utility agent's editable prompt, and
inference (session-bound agentctl, falling back to the host instance so a
review works on a task whose agent is not running).
- Boot sweep cancels review runs interrupted by a restart; the E2E reset
clears review state before deleting the tasks that own it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(web): add review findings state, helpers, and live updates
Frontend plumbing for native code review, ahead of the UI that renders it.
- Review types, WS message map, and an API client that preserves the backend's
machine-readable failure code so the surface can show an actionable message.
- A review store slice that merges findings by id (a re-publish refreshes an
issue rather than listing it twice) and keeps a bounded run history.
- Findings helpers: composite file keys matching reviewFileKey, staleness via
the same per-file diff hash that drives review marks, and best-effort
re-anchoring from the stored anchor text so a moved finding follows the code
it was written about instead of pointing at whatever now occupies its line.
- Hooks for backfill, resolve/dismiss/reopen with optimistic rollback, and
sending a finding to the agent as follow-up context without changing its
status — handing a finding to an agent is not the human accepting it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(web): render review findings inline in the Changes diff
Findings now appear where the code is, in the same annotation surface as
anchored review comments, with the run control that produces them.
- ReviewFindingCard: severity chip, category, Markdown body, and the three
advisory actions. A suggestion is labelled "not applied automatically"
because it is display-only in this iteration. Resolved and dismissed
findings collapse to one muted line with an Undo, so a reviewed diff does
not stay cluttered while the human can still change their mind.
- A review-finding annotation type threaded through the diff viewer, anchored
from the same per-file diff hash the backend stamped. A finding whose diff
moved is re-anchored from its stored text when that text survives, and
otherwise reported as unanchored rather than placed against unrelated code.
- Review changes control with idle / running / cancel / failed states, and an
inline Settings link for the one failure the user can actually fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(web): make the review control reachable from the Review toolbar
Mounts the Review changes control next to the walkthrough action and lets an
inline finding wire its own actions.
The finding card container now reads the active task and session from the store
instead of taking four callbacks threaded through FileDiffViewer and DiffViewer;
an inline finding annotation only exists when the active task has findings, so
the store is always present and every other diff consumer is spared the
plumbing. Staleness likewise hashes the diff the viewer already holds, which is
already normalized, so it matches the hash the backend stamped without passing
it down.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(web): surface stale review findings above their file's diff
A finding whose diff moved and whose anchor text is gone had nowhere to render,
so it was silently dropped. It now appears in a collapsed per-file banner above
the diff: the finding may still be valid, and placing it at its stored line
would point at whatever code now occupies that line instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(review): mark the landed native code-review plan tasks done
Tasks 01-09 (backend persistence through inline diff rendering) are
implemented, tested, and committed. Tasks 10-13 — findings overview and step
editor, mobile parity, E2E, and public docs — remain pending.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(review): cover the on-demand review path end to end
Adds the mock-agent review responder and a Playwright spec, so both the happy
path and the no-reviewer path are verified against a real backend.
The mock agent answers any prompt carrying the review sentinel with fenced JSON,
deriving its anchors from the prompt's own diff rather than hardcoding them, so
findings land on files and lines the fixture really changed. It includes one
deliberately malformed entry to exercise the rejected-entry path.
Writing it caught an off-by-one shared by three diff walkers: splitting on "\n"
leaves an empty element after a trailing newline, which carries no diff marker
and was being counted as a context line, pushing new-side numbering one past the
end of a hunk. Interior blank lines still count, since a whitespace-stripped
blank context line legitimately appears as "".
Also adds a listInferenceAgents helper: default_utility_agent_id needs the
registered agent-type id, and an agent row UUID is rejected as not
inference-capable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(review): close review-run races and stale client findings
Addresses the three blockers and three suggestions from the PR #1957 automated
reviews. Two independent reviewers found each blocker separately.
- Launch claimed the task slot only after inserting the run row, so two
concurrent launches both created a run and the loser returned a pending row
with no goroutine behind it — a review that never finished until the next
restart. The slot is now claimed before any DB write, and freed on every
early return.
- Cancelling only marked the row. The goroutine ran on the shared background
context, finished inference, and its completion overwrote the cancelled
status and published findings the user had declined. Runs now carry a cancel
handle, the WS action routes through the runner so the live context is
cancelled, CompleteRun/FailRun refuse to resurrect a terminal run, and a
cancelled pass neither publishes nor re-reports itself as failed.
- The backend deletes a superseded finding and inserts a replacement with a new
id, but the publish event carried only the new one, so every open browser
showed both at the same anchor until a reload. The event now names the
superseded ids and the slice drops them.
- Run() waited on the shared WaitGroup, blocking on every other task's review;
it now waits on its own pass only.
- createReviewSlice is typed over the `set` it actually uses, so call sites no
longer pass superfluous arguments (CodeQL js/superfluous-trailing-arguments).
- A GetAgentProfile fault no longer collapses into "profile not found", which
would have told the user to configure an agent they already have.
The cancel bug exposed a hole in the original tests: they asserted cancel could
not override a terminal run, but never the reverse. Both directions are covered
now, along with the concurrent-launch race, per-run waiting, and the supersede
path on both sides of the wire. Test fakes now mirror the service's terminal
guard rather than hiding it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope session-keyed reads and mutations to the workspace owner
Session-keyed service entry points did not apply per-user scoping, so
knowing another user's session or task ID was enough to reach it:
- GetTaskSession, ListTaskSessions, GetPrimarySession returned rows for
any ID. These feed GET /task-sessions/:id and GET /tasks/:id/sessions,
whose DTO carries session Metadata, the host worktree path, the branch,
and container/executor IDs.
- DismissLastAgentError mutated any session.
- ApproveSession approved a workflow review step on any session, which
advances the task's workflow — the highest-impact of the set.
Authorize via authorizeTaskID off the row already read, rather than
AuthorizeSessionAccess, which would re-read the session on what is a hot
path. Internal identity-less callers (pollers, event bus, office
schedulers, office agent JWTs) and the synthetic identity used while auth
is disabled stay unscoped, and unowned pre-auth rows stay visible.
Batch helpers that take ID lists are deliberately left alone: their
callers derive IDs from an already-authorized task list, so per-ID checks
would add N queries to list views for no gain. Recorded in AGENTS.md.
Also route httpApproveSession through handleNotFound so a denial answers
404 like every other session route instead of a 500 that would confirm
the session exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope the session-keyed paths that bypass the task service
An adversarial sweep of the previous commit found session-keyed surfaces
reachable with a caller-supplied ID and no ownership check. They bypass
the task service, so scoping its methods did not cover them:
- POST /task-sessions/:id/{set-mode,set-model,set-config-option,
authenticate} and .../processes/:processId/stop resolve their execution
with a bare in-memory lookup, which skips the GetOrEnsure* chokepoint
where the lifecycle per-user check runs. A non-owner could switch
another user's model or mode, re-authenticate their agent, or kill
their process. Guarded by a shared denySessionAccess helper.
- The orchestrator reads sessions through its own repo handle, so
GetTaskSessionStatus and CheckSessionPR inherited nothing from the task
service. CheckSessionPR also installs a PR watch as a side effect.
Added a SetSessionAccessChecker hook mirroring the lifecycle manager's,
wired to taskSvc.AuthorizeSessionAccess. The check runs before
CheckSessionPR's unconfigured-GitHub early return so it applies
regardless of setup, and denials reuse the "session not found" shape so
they cannot be told apart from a missing session.
- GetMessage had no scoping while ListMessages beside it did, so a
caller holding another user's (session_id, message_id) pair could read
their shell output.
This does NOT close the whole surface. A second sweep found the
orchestrator's session-mutating WS actions (session.stop/delete/rename/
set_primary/set_plan_mode/reset_context/recover/launch/ensure,
agent.cancel, permission.respond) and agent.stdin / agent.resize also
unscoped — agent.stdin writes arbitrary input into another user's agent
process. Those touch live session lifecycle and need their own change;
recorded in AGENTS.md so the gap is not mistaken for covered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope every session-keyed WS action, plus a gateway backstop
Completes the sweep. Dispatched WS actions were left to authorize
themselves at the service layer, and a dozen never did — a caller with a
guessed session ID could stop, delete, rename, relaunch or reset another
user's live agent session, cancel their in-flight turn, answer their
permission prompts, or write arbitrary bytes into their agent process.
Two layers, deliberately both:
1. Gateway backstop. Client.authorizeAction checks any dispatched payload
carrying task_id or session_id against the client's identity before
dispatch. Per-method opt-in across ~74 actions in four packages had
been missed repeatedly; this makes a newly added action scoped by
default. Verified safe to apply generically: session_id means a task
session everywhere, and user-shell/terminal actions key off task_id /
task_environment_id, so nothing is misread.
2. Explicit guards, for defense in depth and for non-WS callers:
- orchestrator: StopSession, DeleteSession, SetPrimarySession,
RenameSession, ResetAgentContext, CancelAgent, RespondToPermission,
SetSessionPlanModeByID, RecoverSession, LaunchSession, EnsureSession,
CancelTransientRetry. Added SetTaskAccessChecker for the task-keyed
ones. LaunchSession guards every intent at the single funnel;
SessionID is empty on create, so the task check carries that case.
- agent.stdin / agent.resize now call lifecycleMgr.CheckSessionAccess,
the helper already documented for bare-lookup handlers.
Guards go first in each method, before any repo/executor use.
TestSessionKeyedEntryPointsGuardBeforeDependencies runs all twelve with
nil dependencies and fails on a panic if one is placed too late.
Denials are indistinguishable from a missing resource: WS returns
not-found, CancelTransientRetry reports "nothing cancelled", and
GetTaskSessionStatus reuses its "session not found" shape.
Unwired checkers stay permissive throughout, so identity-less internal
callers (schedulers, event bus, workflow engine) and pre-auth single-user
instances are unaffected — pinned per entry point.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope environment-keyed user-shell actions
Third sweep finding, in surface the previous commit's own comment claimed
was covered. The user-shell actions key off task_environment_id and treat
task_id as optional, so a task_id-only backstop missed them:
user_shell.stop with an empty task_id and a foreign task_environment_id
fell through to the legacy runner path and tore down another user's
terminal PTY. That path never reaches
GetOrEnsureExecutionForEnvironment, where the environment check normally
runs, so neither layer caught it.
- Gateway backstop now also reads task_environment_id, gated by a new
AuthPolicy.ActionEnvironment hook wired to
taskSvc.AuthorizeEnvironmentAccess. This covers all three
environment-keyed actions generically, not just the reported one.
- wsUserShellStop guards explicitly via a new
lifecycleMgr.CheckEnvironmentAccess, mirroring CheckSessionAccess, so
the fallback is safe independent of the gateway.
Corrected the comment that wrongly asserted task_environment_id was
already covered, and recorded in AGENTS.md both the exact field names the
backstop reads and why a permissive type on one of them would silently
opt out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): authorize both IDs on task+session entry points
Review finding (P1). Entry points taking both a task and a session ID
only checked the session, so a caller could satisfy that check with one
of their own sessions while pointing taskID at another user's task:
- CheckSessionPR then used the unchecked taskID for GetTaskPR,
resolveTaskRepo, resolvePrimaryTaskRepositoryID and EnsurePRWatch —
cross-workspace PR disclosure plus a PR-watch write.
- CancelTransientRetry handed it to handleRecoverableFailure, which
writes against that task.
Both now use a new authorizeTaskSessionPair, which authorizes each ID and
additionally requires the pair to be consistent, so the two arguments
cannot describe different tasks. GetTaskSessionStatus already validated
the pair after loading the session and is unchanged.
The consistency half is skipped when the session cannot be loaded: both
IDs are authorized by then, so a mismatch is a caller bug rather than a
cross-user leak, and failing there would change behavior for
identity-less internal callers.
Recorded the rule in AGENTS.md — a two-ID entry point must never use
authorizeSession alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope AttachWorkspaceSources
Found while rebasing onto main. POST /tasks/:id/workspace-sources and the
add_workspace_sources_kandev MCP tool landed on main during this PR's
review with no ownership check, so a caller could attach repositories and
folders to another user's task and have them materialized into that
task's workspace.
Same class as the rest of this PR, so fixed here rather than left for a
follow-up. The WS dispatch backstop added earlier already covered the
browser path (the payload carries task_id); this closes the HTTP route
and the MCP tool, which do not go through it.
ErrTaskNotFound already maps to 404 in workspaceSourceHTTPStatus, so the
denial surfaces correctly without touching the handler.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): map denied shell-output reads to 404
Review finding. GetMessage became per-user scoped earlier in this PR, so
a denial now arrives as ErrTaskNotFound. httpGetShellOutput only mapped
sql.ErrNoRows to 404, sending everything else to the 500 branch — so a
foreign message answered 500 while a missing one answered 404, which is
an existence signal and inconsistent with the 404-everywhere rule the
other session routes follow.
Kept the sql.ErrNoRows branch explicit rather than replacing it outright:
isNotFound classifies typed sentinels and the "not found" substring, and
"sql: no rows in result set" matches neither, so routing that case
through handleNotFound alone regressed the missing-message response to
500 (caught by TestHTTPShellOutputSnapshot).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(auth): make the shell-output scoping test actually exercise the guard
Both reviewers caught that TestHTTPGetShellOutputDeniesForeignMessageWith404
was vacuous. httpGetShellOutput has a second 404 gate after the service
call — `if !ok || message.TaskSessionID != c.Param("id")` — and
ExtractShellExecOutput returns ok=false for nil metadata. The fixture had
none, so the route answered 404 through that gate whether or not scoping
denied the read.
Confirmed by mutation rather than inspection: with the authorization
removed from GetMessage the test still passed. With valid shell-output
metadata in the fixture it returns 200 and the victim's stdout, so the
test fails as intended; restoring the guard makes it pass again.
Also mutation-tested the sibling denial tests, since this is a defect
class rather than a one-off:
- TestHTTPGetTaskSessionDeniesForeignSessionWith404 — sound, returns 200
with the session DTO when the guard is removed.
- TestProcessRoutesDenyForeignSession — catches removal, though via the
documented nil-lifecycleMgr panic rather than a status assertion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(auth): add an in-test witness to the shell-output scoping test
Review suggestion. The fixture fix made the test exercise the guard, but
nothing in the test proved the 404 came from authorization rather than the
route's post-service `!ok` gate — which is exactly how it went vacuous in
the first place. The evidence lived in a commit message describing a
manual mutation run, not in the test.
Now asserts both directions, matching the approve-route test: the
non-owner gets 404, and the owner does NOT. If the assertion ever stops
proving authorization, the owner case fails and says so.
Re-verified by mutation: removing the guard from GetMessage fails the test
with 200 and the victim's stdout; restoring it passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore: revert agent subscription usage in settings and chat tooltip
Reverts #1697 (55c8b461b). The subscription usage plugin now covers this,
so the built-in Settings > Agents section and the context-doughnut tooltip
rows are no longer needed.
Conflicts resolved to preserve later work on the same files:
- token-usage-display.tsx/.test.tsx: kept the pinnable tooltip (#1869),
the rebuilt context-window layout (#1741), and the ACP/API source row
(#1752); removed only SessionUsageRows and its imports.
- settings/agents/page.tsx: #1741 moved <AgentUsageSection /> above the
pre-existing <Separator />, so the revert missed the JSX; removed it
and kept the separator.
- agent-usage-section.tsx/.test.tsx: deleted (later changes were cosmetic
tweaks to the section itself).
Office-side subscription surfaces (dashboard quota card, agent detail
QuotaCard, internal/agent/usage clients) predate #1697 and are untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(usage): keep #1697 client fixes when reverting its UI surface
#1697 bundled two things: the Settings > Agents section plus doughnut
tooltip rows, and a set of genuine bug fixes to the underlying provider
usage clients. Reverting it wholesale threw away the second group, which
is still live code behind the office dashboard quota card and the agent
detail QuotaCard.
Restores the client layer to its post-#1697 state (billing.go, cache.go,
client_claude.go, client_codex.go, fileutil.go, types.go and the two
client tests), fixing the defects CodeRabbit flagged on #1940:
- billing type is recomputed per call again instead of being frozen for
the process lifetime by sync.OnceValue, so login/logout is observed
without a backend restart
- usage cache regains fetch coalescing and short negative caching, so a
provider outage no longer means one outbound call per request
- Claude usage parses float utilization and the richer limits[] array
instead of failing to decode
- Claude token refresh patches only claudeAiOauth in a decoded map and
writes atomically via temp+rename, preserving unknown keys in the
credentials file the real CLI shares
- Codex reads tokens.access_token rather than a nonexistent flat token
field, and its doc comment matches the ~/.codex/auth.json path
The removed UI/API surface stays reverted: GET /api/v1/agents/usage, the
host usage lister, the settings section, and the tooltip rows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Carlos Florêncio