* 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>
* feat: notify user when a kandev update is detected
Adds a background-poller-driven update notification with a saved
preference for delivery channel (desktop / in-app banner / both) and
an enable/disable switch on the Updates settings page.
Backend:
- internal/system/updates: NotifySettings + NotifyStore persist the
enable/channel preference on the existing install-wide settings
table; notifyIfNewUpdate broadcasts a WS event the first time a
newer release is observed, deduped durably via kandev_meta so a
restart or repeat poll doesn't re-announce the same version.
- New GET/PUT /api/v1/system/updates/notification-settings endpoints.
- pkg/websocket: new system.update_available action.
Desktop (Tauri):
- native_notifications.rs: task_id is now optional (Option<String>,
required only for session-scoped events) and the event allowlist
accepts system.update_available: alongside the existing session
events, so the update notification can use the native bridge.
Frontend:
- New UpdateNotificationsCard on the Updates settings page (Switch +
channel Select), wired through the shared settings-save-provider
floating Save/Discard action.
- useUpdateAvailableToast bridges the WS event to an in-app toast
and/or the native/browser desktop notification, per the saved
channel, deduped per version.
* docs: document the update notification channel setting
* fix: hydrate update notification settings and broadcast delivery ack
- Only mark a release notification as delivered when the broadcaster
reports at least one connected client, closing a race where an
update could be silently dropped before the first client connects.
- Hydrate updateNotificationSettings through the settings boot payload
(loadSettingsInitialState -> hydrateState) instead of a component-
level fetch, per the boot-payload -> store -> component data flow.
Widen the hydration types (HydrationState) so callers can hydrate a
partial system slice without fabricating the other system fields.
UpdateNotificationsCard now only reads/writes the subscribed store
state.
- Fix module-scope settings-fetch cache leaking across tests in
use-update-available-toast.test.ts so the fetch-failure test
actually exercises the catch/fallback path.
- Add API-client contract tests for fetchUpdateNotificationSettings
and saveUpdateNotificationSettings (GET cache policy, PUT method
and body).
* test: cover delayed hydration and boot-payload mapping for update notifications
* fix(scripts): launch dev-isolated backend via __backend subcommand
The kandev binary became a bundle launcher with the restart supervisor
(ADR 0019) and exits without KANDEV_BUNDLE_DIR when invoked bare; the
isolated dev instance must run the backend entrypoint directly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(mcp): session spawning, sibling messaging, and renameable tabs
- spawn_session_kandev spawns an additional agent session on an existing
task (default: the caller's own) via LaunchSession(IntentStart), with
optional agent_profile_id (defaults to the spawner's profile) and a
session name; the initial prompt carries spawner attribution and reply
instructions.
- message_task_kandev accepts an optional session_id to target a specific
session, enabling same-task sibling messaging. Explicit targets are
pinned through dispatch so the idle-path primary-session re-resolution
cannot reroute a sibling message back to its sender (regression-tested).
- task_sessions gains a name column (migration ordered after the
agent_execution_id-drop rebuild), a session.rename WS action, and inline
tab rename UI; custom names take precedence over derived agent/model
labels across the tab, agents dropdown, mobile list, chat header, and
dockview panel titles.
- Inter-session messages stamp sender_session_name into metadata so the
sender badge shows which session sent the message.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(mcp): address review findings on session spawn/rename/messaging
- include name in task-session DTOs (list + full, no omitempty) so
persisted tab names survive list hydration and clears propagate
- sanitize sender task/session ids embedded in <kandev-system> blocks
- reject cross-workspace spawn targets; require sender_task_id
- truncate session names by runes, not bytes
- distinguish not-found from internal errors in message target lookup
- guard TabRenameInput blur-commit after Enter/Escape resolution
- cap optimistic rename length client-side and toast on rename failure
- add tests: spawn launch-failure and cross-workspace paths, renameSession
API client
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(mcp): round-two review fixes for spawn/rename lint and hardening
- extract spawn validation/authorization helpers (cyclop/nestif limits)
- make stripSystemTag replace until stable to block nested tag evasion
- load session before rename so a durable rename can never skip its
broadcast; surface lookup failures to the caller instead
- drop redundant sessionLauncher nil check (guarded registration)
- add maxLength to the rename input matching the server cap
- add tests: nested sanitizer evasion, spawn rename-failure tolerance
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* feat(workflow): explicit step-completion signal for auto-advance
Adds `step_complete_kandev` MCP tool + `WorkflowStep.auto_advance_requires_signal`
column so workflow steps can gate their `on_turn_complete` transitions on an
agent-emitted completion signal instead of inferring "done" from any turn-end.
See docs/decisions/0015-explicit-completion-signal-for-auto-advance.md for the
motivation, alternatives considered, and trade-offs.
* fix(workflow): address PR review feedback for explicit completion signal
- Dedup `LoadPendingStepSignal` into `internal/task/models` so the MCP
handler (write site) and the orchestrator (read site) share one decoder.
- Gate the out-of-band subscriber on `WorkflowStep.AutoAdvanceRequiresSignal`
so `step_complete_kandev` is not an unintended manual-advance path for
signalless steps.
- Fail closed on step-load errors in `processOnTurnCompleteViaEngine`: a
loader hiccup must block the transition rather than silently bypass the
gate.
- Reject `step_complete_kandev` for terminal sessions
(Completed / Failed / Cancelled) so the agent gets a clear error instead
of `accepted: true` followed by a no-op.
- Surface bus-publish failures back to the agent instead of swallowing
them — the idempotency guard makes the retry safe and prevents the
session from getting stuck.
- New `TestOnStepCompletionSignaled` covers the subscriber paths
(RUNNING / WAITING+match / stale step / not signal-gated).
- ADR doc fixes: payload field list now matches the wire shape, writer
ownership is documented, the unsubstantiated 250ms-debounce claim is
removed, and the late-signal re-close race is recorded as a deferred
open question.
- Drop the new toggle's HelpTip in favour of an inline description — the
shadcn tooltip portal was intercepting clicks on the agent-profile
dropdown options in the workflow settings E2E.
* fix(workflow): round-2 review fixes for explicit completion signal
- Split `handleStepComplete` into `resolveStepCompleteTarget` +
`publishStepCompletionEvent` helpers so the entrypoint clears funlen
(88 → under 80 lines) and the two halves are independently testable.
- Classify session/task load errors instead of returning `NotFound` for
every failure: use the exported `taskrepo.ErrTaskNotFound` sentinel for
task lookups, substring-match for the session repo (no sentinel today),
and surface anything else as `InternalError` with the raw error logged
but not leaked to the agent — so a transient DB hiccup triggers a retry
rather than session abandonment.
- Pin `step_complete_kandev` explicitly in the mode-specific tool-count
tests: `Contains` in `ModeTask`, `NotContains` in `ModeConfig` and
`ModeOffice`, so accidental dereg or cross-mode registration is caught
by the diff instead of by a still-equal count.
* fix(workflow): round-3 review fixes + defer pipeline-editor toggle
- Defer the "Wait for agent completion signal" sub-toggle to a follow-up
PR. Adding it to the step config card shifts the layout enough that the
agent profile select's Radix popper opens into a sibling tooltip's hover
path, breaking workflow-agent-profile.spec.ts on shard 7. The backend
column ships in this PR — workflows can opt in via the YAML import or
the MCP `update_workflow_step_kandev` tool — and the UI affordance gets
its own iteration where the test interference can be investigated
without blocking the rest of the change.
- Re-publish the bus event on the `already_signaled` short-circuit when
the session is WAITING_FOR_INPUT. Without this, an agent's retry after
the first call's publish failed would short-circuit on the persisted
bag, never fire the subscriber, and leave the session stuck.
- handleStepComplete docstring: drop the non-existent feature-flag claim
and the "audit entry" wording — gating is purely
`WorkflowStep.AutoAdvanceRequiresSignal` and the bag is cleared on the
next turn start with no separate audit trail.
- `step_complete_kandev` tool description: same fix — clarify that the
call succeeds for non-gated steps but the signal is discarded on the
next turn start (no audit history to query later).
* test(workflow): cover handleStepComplete + reword defer-comment
- Add `TestHandleStepComplete_*` (6 subtests, all passing) covering the
uncovered handler paths flagged by Claude/cubic on round 3:
missing-fields validation, session-does-not-belong-to-task ownership
guard, terminal-session rejection (Completed/Failed/Cancelled),
first-call happy path (bag persisted + bus payload shape pinned), dedup
while RUNNING (no re-publish), and dedup while WAITING_FOR_INPUT (the
retry-after-publish-failure path — must re-publish so the subscriber
fires).
- Reword the deferred-toggle comment to drop the YAML-export claim:
the workflow export model doesn't yet serialize
`auto_advance_requires_signal`, so MCP `update_workflow_step_kandev`
is the only configuration path that round-trips today. Note the
follow-up needs to extend the export schema alongside the UI toggle.
* test(workflow): pin signaled_at + drop redundant GetTaskSession on bag-clear
- Assert `signaled_at` is present and parses as RFC3339Nano in
`TestHandleStepComplete_FirstCallAccepted`. The field is part of the
handler's documented response contract; pinning it catches accidental
drop/rename regressions.
- Add `clearPendingStepSignalByID(ctx, sessionID)` as a thin DB-only
variant of `clearPendingStepSignal` and use it from
`executeStepTransition`, where the session struct isn't read after the
clear. Avoids an extra `GetTaskSession` round-trip per transition.
`clearPendingStepSignal(session)` still exists for the
in-memory-mutation case (subscriber's stale-step path).
* fix(workflow): subscriber stale-clear guard + ADR/comment drift fixes
- onStepCompletionSignaled's stale-step path now reads the bag and only
clears it when its current StepID matches the (stale) event's stepID.
Previously the unconditional clear could destroy a valid signal that
the new step's agent had just written when sessions are reused across
steps without auto_start_agent. (Greptile P1.)
- Drop the "feature flag" phrase from WorkflowStep.AutoAdvanceRequiresSignal
docstring — leftover from an earlier draft. The gating is purely the
boolean field; the global flag was removed two rounds ago.
- Merge the two adjacent `if triggerOnEnter` blocks in
executeStepTransition so the full triggerOnEnter=true path reads top-
to-bottom in one place. (CodeRabbit + cubic.)
- ADR 0015 Mechanism section: re-label the SessionStepHistory.Metadata
audit annotation, the `step_complete_signaled` system message, and the
expvar telemetry counters as deferred. They're planned shapes, not
current behaviour — calling them "Audit" / "Chat visibility" /
"Telemetry" in present tense was misleading for future readers.
* fix(workflow): drop subscriber complexity + reload session on stale-clear
- Extract `parseStepCompletionEvent` and `stepIsSignalGated` helpers from
`onStepCompletionSignaled`. Cyclomatic complexity goes 17 → under 15
(cyclop on new code, CI lint regression after the round-6
stale-clear guard).
- The stale-step branch now re-loads the session from the DB before
deciding whether to clear the bag. The previous round-6 fix checked
the local `session.Metadata` snapshot, which can be older than a
concurrent write from the new step's agent — that check could see a
stale signal that had since been overwritten with a valid one and
erase the new bag entry. The narrower load-on-clear is cheap and
closes the cubic P1 hole. (The broader race in
`processOnTurnCompleteViaEngine` reading session.Metadata is the
monotonic-marker open question in ADR 0015; out of scope here.)
* test(workflow): pin stale-event guard's preserve-current-bag side + dedup StringFromAny
- Add `TestOnStepCompletionSignaled` subtest "stale event → valid bag
for CURRENT step is preserved" — fires a stale step-A event while
the bag holds a valid step-B signal, asserts the bag still contains
StepID=step_b after the subscriber runs. Pins the negative side of
the round-6 `existing.StepID == stepID` guard, so a regression that
drops or inverts the guard (and would silently leave signal-gated
steps stuck) fails the test instead of slipping through.
- Promote `pendingSignalString` to exported `models.StringFromAny`
and reuse it from the orchestrator subscriber's
`parseStepCompletionEvent`. Eliminates the byte-for-byte duplicate
helper across the two packages; the bag-shape decoder and the bus-
event decoder now share one source of truth for "missing string".
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
* feat(diff): use stored base_branch for diff stats + per-task compare picker
Task-card additions/deletions and the Changes panel were computed against a
hardcoded origin/main → master priority list inside agentctl. For tasks
branched off a release/staging branch this inflated the counts by every
commit since the actual fork point, and the Changes panel grouped unrelated
files alongside the task's real work.
This wires the per-repo task_repositories.base_branch end-to-end into the
agentctl WorkspaceTracker so each repo's diff stats resolve against the
recorded base instead of the integration-branch fallback. A picker on the
Changes panel branch hover card lets the user change the base mid-task; the
new value is persisted via PATCH /tasks/:id/repositories/:repo_id, pushed
live to the running agentctl instance, and the session's cached
base_commit_sha is cleared so the commits panel + cumulative diff refetch
against the new base without a session restart.
Falls back to the existing origin/main → master list for legacy tasks
(empty base_branch) and to the branch tip when no merge-base exists with
HEAD (unrelated histories — local rebase backups, etc.) so the stats and
commits panels stay anchored on the same SHA in every failure mode.
* fix(agentctl): sanitize user-controlled base_branch refs before git exec
CodeQL flagged the picker's user-controlled base_branch payload reaching
exec.Command("git", ..., baseBranch) inside workspace_git_cmd.go as a
command-built-from-user-controlled-source vulnerability. A malicious caller
could smuggle a leading-"-" argument like "-upload-pack=..." that git
itself reinterprets as a flag, even though exec.Command keeps args
separate from the shell.
Adds an IsSafeGitRef sanitizer at every boundary the value crosses on its
way to git: the service write path, the agentctl HTTP handlers
(/workspace/base-branches, /git/log, /git/cumulative-diff), and
WorkspaceTracker.SetBaseBranch itself. Unsafe values are rejected at the
service tier and silently downgraded to the no-override fallback inside
agentctl so a misbehaving caller can't half-break the tracker.
* fix(diff): address PR review blockers and CI lint
- Split UpdateRepositoryBaseBranch into validate/load/apply helpers to drop
cyclomatic complexity below the cyclop limit that gated Run Backend Tests.
- Replace direct m.cfg.BaseBranches reads/writes with getBaseBranches /
setBaseBranches under a dedicated RWMutex. UpdateBaseBranches used to
write under repoTrackersMu while rescan + lazy-subpath callers read under
workspaceTrackersMu, leaving an unsynchronized data race on the map.
- Drop the false 'PR target is auto-wired' claim from the MCP tool
description — GitOperator.CreatePR takes baseBranch as an explicit arg
and never reads the tracker's value.
- Picker no-op guard now compares against currentBase instead of storedBase
so selecting the displayed fallback (legacy task before any picker
change) is treated as no-op instead of triggering a spurious save.
- HTTP handler now maps non-validation service errors to 500 instead of
400, classifying via the existing isValidationError helper plus the new
unsafe-git-ref message.
* fix(diff): regex-based ref sanitiser at every git subprocess sink
CodeQL kept flagging workspace_git_cmd.go:43 even after SetBaseBranch
rejected unsafe values — the taint tracker doesn't follow sanitised stores
through struct fields, so it still saw user input flowing into
exec.Command("git", ..., baseBranch).
Switches IsSafeGitRef to a single regex check (a pattern CodeQL recognises
as a sanitiser barrier) and adds SanitizeGitRef calls inline at every
agentctl call site that splices a user-controlled ref into a git arg:
- workspace_git_status.resolveStoredRef (before the rev-parse probes)
- workspace_git_status.computeBaseCommit (before merge-base/rev-parse)
- workspace_git_status.getAheadBehindCounts (before rev-list)
- agentctl/server/api/git.go runGitLogForRepo + handleGitCumulativeDiff
(before computeMergeBase/GetRevParse on the HTTP TargetBranch)
Behaviour is unchanged for valid refs — only narrows the allowlist (now
[A-Za-z0-9_./-]) so the sanitiser barrier is recognisable to static
analysis. SetBaseBranch's field-store sanitiser stays as defence in depth.
* fix(diff): transform sanitiser at every map read for CodeQL taint flow
CodeQL still flagged workspace_git_cmd.go:43 because:
- handleSetBaseBranches used an IsSafeGitRef bool guard before a verbatim
map[k] = v copy. CodeQL recognises regex-transforming sanitisers but not
bool-guarded passthroughs.
- lookupBaseBranch returned m.cfg.BaseBranches[k] verbatim. Even though
the map was sanitised on write, taint flow across map stores + field
stores loses the sanitised state in the analyser.
Switches both sites to the transforming SanitizeGitRef helper so every
read produces a freshly-sanitised value before it flows to a downstream
git argument. Behaviour is unchanged for valid refs.
* fix(diff): regex transformer sanitiser + guard live push from empty map
CodeQL still flagged the previous SanitizeGitRef because it returned the
input string unchanged on success — an identity passthrough that the taint
tracker doesn't recognise as a sanitiser barrier.
Switches to safeGitRefPattern.FindString so the output comes from the
regex match (not the parameter). IsSafeGitRef now wraps SanitizeGitRef so
the predicate and the transformer share one allowlist.
Also addresses two Claude review blockers:
- collectTaskBaseBranches returns a nil/empty map when the task has no
recorded base_branches; pushing that to agentctl wipes every tracker's
override. Now skipped — the DB row is the source of truth for the next
session launch.
- applyBaseBranchSideEffects now runs on context.WithoutCancel so an HTTP
/ WS request cancel after the task_repositories UPDATE commits can't
leave session.base_commit_sha stale + the live agentctl tracker map
un-pushed, drifting from the persisted row.
* fix(diff): share securityutil.IsValidBranchName + harden error classification
CodeQL still flagged the picker's regex sanitiser as taint passthrough.
Refactors SanitizeGitRef / isSafeBaseBranchRef to delegate to the existing
securityutil.IsValidBranchName helper that agentctl's Rebase / Merge /
RenameBranch already use without being flagged. The shared allowlist also
fixes a Claude review blocker: 'feature/@2024' used to pass the service
validator but get dropped by the agentctl-side check at push time,
silently wiping every tracker's override.
Plus the two error-classification blockers Claude raised on the previous
HEAD:
- WS handler (task_ws_handlers.go wsUpdateTaskRepository) now distinguishes
validation errors (surfaced verbatim) from internal faults (opaque
500-style message). HTTP path already did this via the existing
isValidationError helper.
- MCP handler (handlers.handleUpdateRepositoryBaseBranch) gets the same
treatment via a new isValidationError local to the mcp/handlers
package so raw service error strings stop leaking across the MCP
boundary.
* fix(diff): inline securityutil.IsValidBranchName at every git subprocess sink
CodeQL's interprocedural taint tracking wasn't recognising SanitizeGitRef
as a sanitiser even after it delegated to securityutil.IsValidBranchName
— the wrapper helper + map/field hops between source and sink kept the
flow tainted in the analyser.
Inlines the IsValidBranchName allowlist check (already used inline by
Rebase/Merge in git.go without being flagged) at every call site that
splices a user-controlled ref into a git subprocess argument:
- workspace_git_status.resolveStoredRef (before rev-parse --verify)
- workspace_git_status.computeBaseCommit (before merge-base / rev-parse)
- workspace_git_status.getAheadBehindCounts (before rev-list)
- agentctl/server/api/git.go runGitLogForRepo + handleGitCumulativeDiff
- agentctl/server/api/workspace_base_branches.go handleSetBaseBranches
Behaviour unchanged: same allowlist as Rebase / Merge / RenameBranch
already use. SanitizeGitRef / IsSafeGitRef survive as defence-in-depth
helpers for callers that want a one-line check; they delegate to the same
securityutil helper so the rules stay aligned.
* fix(diff): inline regexp.MatchString at every git subprocess sink
CodeQL still flagged workspace_git_cmd.go:43 even with the inline
securityutil.IsValidBranchName helper — the taint tracker apparently does
not follow into helper functions for the go/command-injection rule on
this path.
Hoists the regex to package-scope vars (safeBranchRefPattern in both
process and api packages) and calls regexp.MatchString directly at each
sink site. Behaviour is identical to securityutil.IsValidBranchName; only
the call form changes.
* fix(diff): SHA-shape allowlist on BaseCommit before downstream git diff
CodeQL still flagged workspace_git_cmd.go:43 because update.BaseCommit
is the OUTPUT of an earlier merge-base / rev-parse call whose argument
list contained a user-controlled branch ref — the analyser treats that
command's output as still tainted even when the input ref was sanitised.
Adds an inline 40-hex SHA-shape allowlist check in enrichWithBranchDiff
right before `git diff --numstat <update.BaseCommit>` so the sanitiser
barrier sits in the same function as the next subprocess call. Behaviour
is unchanged: merge-base / rev-parse already produce SHA-shaped output.
* fix(diff): async tracker refresh + tighten init spread order
Two CodeRabbit/Claude review fixes:
- UpdateBaseBranches stamps each tracker's new baseBranch synchronously
then runs RefreshGitStatus on a background goroutine instead of inside
the picker-save HTTP handler. Per-tracker git subprocesses can each
take ~3-5 calls; serialising them on the request goroutine made the
PATCH response wait for every refresh to finish. The picker UI re-fetches
via the existing WS stream once Refresh emits, so the handler doesn't
need to block.
- updateTaskRepositoryBaseBranch now spreads options.init BEFORE the
required method/body fields so an upstream caller's init overrides
can't replace the PATCH method or the base_branch payload. Matches
the convention deleteTask / moveTask already follow.
* fix(diff): narrow mcp isValidationError + opaque 500 + post-sanitize log count
Three final Claude review threads on the latest commit:
- mcp/handlers isValidationError drops the broad 'invalid' substring so
DB / IO errors whose messages happen to contain 'invalid' don't
misclassify as ErrorCodeValidation.
- task_http_handlers httpUpdateTaskRepository now logs the raw service
error server-side and returns an opaque 'failed to update task
repository' on the 500 path. Stops potential DB / IO detail leakage
through the HTTP boundary.
- workspace_base_branches debug log reports accepted + rejected counts
(post-sanitisation) instead of the pre-filter total so the metric
actually reflects what hit UpdateBaseBranches.
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
* feat(backend): full GitLab parity — store, service, poller, WS, presets
Add the watcher/poll/write-action surface on top of the existing GitLab
client + service skeleton:
* 5 new SQLite tables (mr_watches, review_watches, review_mr_tasks,
issue_watches, issue_watch_tasks, action_presets) + CRUD
* Client + Service: MergeMR, GetProjectMergeMethods, GetProtectedBranch,
ListUserProjects, SearchProjects, SetMRLabels, SetMRAssignees
* Service: review/issue/MR watch CRUD + Check/Trigger, GetStats,
CleanupAllReview/IssueTasks (cleanup-policy aware), TriggerMRSync,
action-presets CRUD with defaults
* Background poller (MR / review / issue loops) wired from main.go
* ~35 GitLab WS action constants + handlers.go registrations
* Mock controller for E2E seeding (MRs, issues, pipelines, discussions,
approvals, branches)
* eventBus + taskDeleter + taskSessionChecker plumbed through Service
* Event types: GitLabMRFeedback / NewReviewMR / NewIssue / TaskMRUpdated
* feat(web): GitLab frontend parity — types, API, store slice, domain hooks
Extend the frontend GitLab integration on top of the new backend surface:
* HTTP API client expansion (gitlab-api.ts): full CRUD for review/issue/MR
watches, MR write actions (merge/approve/labels/assignees), MR
files/commits/feedback, action presets, projects autocomplete, stats
* Store slice expansion: review/issue/MR watches lists, action presets
by workspace, stats + status with loading flags. Properties prefixed
with gitlab to avoid collision with GitHub slice
* Domain hooks: use-gitlab-status, use-gitlab-stats,
use-gitlab-review-watches, use-gitlab-issue-watches,
use-gitlab-action-presets — mirror github hook shapes
* Type model: ReviewWatch, IssueWatch, MRWatch, ActionPresets, Stats,
ProjectMergeMethods, MRApproval, Pipeline, MRFeedback, etc.
* Backend controller HTTP routes for watches/presets/write-actions/
projects/stats so the frontend HTTP API client has endpoints to hit
All passes: backend (build + vet + lint + tests), web (typecheck + lint +
2400 tests).
* fix(gitlab): wire orchestrator event handlers, fix label-filter drop, add tests
Address Claude review blockers on the parity PR:
1. Wire orchestrator subscriptions for GitLab review/issue watch events.
Add event_handlers_gitlab.go with handleGitLabNewReviewMR /
handleGitLabNewIssue, dedup reservation handshake, and task-creator
interfaces (mirrors event_handlers_github.go). main.go now calls
orchestratorSvc.SetGitLabService so the dedup APIs are usable
2. Fix silent label-filter drop: when a custom_query is set on an issue
watch, labels were being appended to the unused filter arg. Fold
labels into customQuery (or default filter) so they actually reach
GitLab; extracted appendLabelsToQuery helper with explicit handling
of pre-existing labels= clauses
3. Switch event payloads to pointer types (mirrors GitHub) so
handlers can type-assert pointer events
4. Backend tests: store_watches_test.go (CRUD + reserve/assign for all
4 new tables + presets), service_watches_test.go (label-filter
helper, cleanup policy, project normalisation), action_presets_test.go
5. Frontend slice tests: review/issue watch CRUD round-trips, action
presets + stats reducer coverage
All green: backend (build+vet+lint+tests), web (typecheck+lint+2403 tests).
* fix(gitlab): address remaining blockers — cleanup wiring, safe defaults, nil guards
Address the second round of Claude review blockers:
1. main.go: wire SetTaskDeleter + SetTaskSessionChecker on services.GitLab
(mirrors GitHub). Without these, manual cleanup sweeps would always
error with "task deleter not configured"
2. service_cleanup.go: default cleanup policy on transient DB error is now
CleanupPolicyNever (preserve tasks) instead of CleanupPolicyAuto
(silently delete). Genuine "watch was deleted" path also falls under
the safe-side default; user can manually delete the orphan tasks
3. service_watches.go: add nil guards on requireStore() so the 6 MR-watch
list/get/delete methods return an errStoreUnavailable error instead
of panicking when the SQLite store failed to construct at boot
4. use-gitlab-status.ts + use-gitlab-stats.ts: add per-mount attemptedRef
so an unreachable GitLab doesn't trigger an infinite re-fetch loop —
the previous useEffect re-ran every render because the failure path
left status null, satisfying the !status guard
* fix(gitlab): propagate session-check errors, clamp poll interval on update
Address remaining Claude blockers:
* service_cleanup.go: HasUserAuthoredMessage transient errors are now
preserved (return false → skip delete) instead of silently ignored;
the alternative was occasionally deleting tasks a user had touched
* service_watches.go: extract clampPollInterval helper applying the same
bounds (0 → default, <60 → 60) as the create path; both
applyReviewWatchPatch and applyIssueWatchPatch use it so user-supplied
zero or tiny values via UpdateXxxWatchRequest no longer hammer GitLab
* fix(gitlab): clear dupl lint, fix preset retry loop + per-workspace watch cache
* service_watches.go: applyReviewWatchPatch / applyIssueWatchPatch get
nolint:dupl markers — they share shape but per-domain field validation
lives in the create paths, so deduplicating via generics would obscure
the contract. Restores backend lint to 0 issues
* use-gitlab-action-presets.ts: per-workspace attemptedRef set so a
failing preset fetch doesn't retry on every render
* use-gitlab-review-watches.ts + use-gitlab-issue-watches.ts: track
lastFetchedRef per consumer so a workspace switch triggers a refetch.
The slice-level loaded flag is shared across instances and can't
double as a per-workspace cache key
* fix(gitlab): add nil-store guards across review/issue watch + cleanup paths
Round-3 Claude finding: ReviewWatch / IssueWatch / preset / reservation
methods still called s.requireStore().X() directly. If NewStore fails at
boot (table migration error), the service struct's store is nil and every
list/get/update/create/reserve method panics on first request.
Apply the same store-nil → errStoreUnavailable pattern from f95a929 to
every remaining method:
* CreateReviewWatch / CreateIssueWatch
* GetReviewWatch / ListReviewWatches / ListAllReviewWatches /
UpdateReviewWatch / TriggerReviewWatchAll
* GetIssueWatch / ListIssueWatches / ListAllIssueWatches /
UpdateIssueWatch / TriggerIssueWatchAll
* ReserveReviewMRTask / AssignReviewMRTaskID / ReleaseReviewMRTask
* ReserveIssueWatchTask / AssignIssueWatchTaskID / ReleaseIssueWatchTask
* lookupReviewPolicy / lookupIssuePolicy (return CleanupPolicyNever)
* fix(gitlab): nil-store guards on CheckXxxWatch + DeleteXxxWatch + cleanup paths
Round-4 Claude finding: 4 remaining panic sites the previous nil-guard
pass missed — these run from the background poller (every 5 min) and the
manual delete/cleanup flows, so a boot-time NewStore failure would
crash the orchestrator process on first tick.
* service_watches.go: add store-nil guards to DeleteReviewWatch /
DeleteIssueWatch / CheckReviewWatch / CheckIssueWatch
* service_cleanup.go: guard CleanupAllReviewTasks / CleanupAllIssueTasks
entry points; wrap the trailing DeleteReviewMRTask / DeleteIssueWatchTask
calls in `if store := s.requireStore(); store != nil` blocks so the
cleanup succeeds even when the dedup-row delete is unreachable
* refactor(gitlab): split service_watches.go to stay under 800-line revive limit
CI's --new-from-rev lint caught service_watches.go at 808 lines (limit
800). Split into three focused files:
* service_events.go (94 LOC) — publish helpers for MR feedback /
new review MR / new issue / watch lifecycle events
* service_issue_watches.go (302 LOC) — Issue watch CRUD + Check +
Trigger + fetch + helpers
* service_reservations.go (59 LOC) — Reserve/Assign/Release dedup
handles used by the orchestrator event handlers
service_watches.go is now 505 lines and contains MR watch + Review
watch only. No behavioral change.
* fix(gitlab): address inline code-review feedback
Address actionable inline comments from Claude/CodeRabbit/cubic/Greptile:
* appendLabelsToQuery: use url.ParseQuery for exact key match instead
of strings.Contains; previously false-matched keys like mylabels=
and silently dropped the watch's labels (test added)
* fetchReviewMRs: drop dead `filter = watch.CustomQuery` assignment —
SearchMRs's buildMRSearchQuery returns customQuery verbatim and
ignores filter when customQuery is non-empty
* store.go: add workspace_id indexes on gitlab_review_watches and
gitlab_issue_watches (the 5-min poller and HTTP list endpoints did
full table scans)
* mock_client.SearchProjects: switch from == to case-insensitive
Contains so partial-query autocomplete returns the seeded project,
matching the doc comment
* Poller: add sync.Mutex to guard `started` field; Start/Stop are
now safe to call concurrently (go test -race would have flagged
the previous read/write race)
* Add ErrWatchNotFound sentinel; controller_watches.go maps it to
HTTP 404 via httpRespondError helper. Update/Trigger handlers
previously returned 500 for missing watches, hiding the distinction
from real server faults
* service_events.go: log Publish errors on watch lifecycle events
instead of swallowing them silently
* handlers.go: reject malformed JSON payloads in list-style WS
handlers rather than silently falling through to broader
list/search behavior
* event_handlers_gitlab.go: remove dead gitlabRepoSlug helper +
blank-identifier suppression
* ci: re-trigger to clear flaky E2E shard 3
* fix(gitlab): address final review round — nil guards, store merge, logs, body validation
- CheckMRWatch, UpdateReviewWatch, UpdateIssueWatch: nil guards for store/req
- handlers: wsNewDiscussionNote rejects empty body
- action_presets: Update reads raw stored row (no default freezing);
Reset validates workspace_id
- service_search.GetStats: log Warn per sub-call failure before zero
- event_handlers_gitlab: log Release* dedup-row errors with context
- web/store.ts: post-slice merge restores every GitLab sub-state field;
extract buildStateOverrides helper
- web/default-state.ts: add GitLab fields to defaultState +
mergeInitialState via mergeGitLabFields helper
- web/gitlab-api.ts: UpdateXxxWatchRequest now Omit workspace_id
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
* feat(tasks): multi-branch tasks — N (repo, branch) pairs per task
Extends multi-repo into multi-branch so a single task can hold multiple
worktrees on the same repo (one per branch). Sibling worktrees layout at
<task>/<repo>/ + <task>/<repo>-<slug>/, dedicated PRWatch per branch,
live agentctl tracker rescan + frontend store updates so the UI splits
PRs / files / commits per branch without a session restart. MCP
add_branch_to_task_kandev tool is shielded to the worktree executor.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(web): unify multi-branch repo label as <repo> · <branch> across panels
PR Changes and Commits panels rendered repo labels differently for multi-branch
tasks — Commits showed "kandev · feature/x" while PR Changes leaked the raw
"kandev-feature-x" agentctl tracker tag. Reformat tracker-tagged repo names via
formatRepoLabel using the longest known-repo prefix so kandev-cli-* doesn't
collapse to kandev · cli-*.
* fix(tasks): address multi-branch PR review — data races, slug collisions, MCP error codes
Backend
- agentctl process.Manager: add repoTrackersMu RWMutex guarding repoTrackers
+ workspaceTracker. Reads snapshot under RLock; RescanRepositories builds
trackers first, takes Lock for swap/append. Validates newWorkDir via
os.Stat before publishing cfg.WorkDir.
- worktree.Manager: reject invalid explicit BranchSlug up-front; cache key
uses sanitized wt.BranchSlug so read/write paths are symmetric.
- worktree.store: narrow isDuplicateColumnError to "duplicate column name".
- task service AddBranchToTask: slug-collision guard rejects feat/a vs feat-a.
- orchestrator resolvePushRepo: two-pass exact-name match before subdir
prefix fallback so "backend" doesn't swallow pushes for "backend-admin".
- executor buildRepoSpecs: duplicate repos always get a non-empty BranchSlug
(falls back to branch-N when both branch fields sanitize to empty).
- MCP handler: classifyAddBranchError maps service errors to NOT_FOUND /
CONFLICT / VALIDATION_ERROR; add ErrorCodeConflict constant.
- MCP server: extract registerAddBranchToTaskTool, register in task mode
only (external mode contract: config tools + create_task_kandev).
- workspace_rescan handler: reject malformed JSON with 400 instead of
silently rescanning the current root.
- branch_materializer: split MaterializeBranch into prepare + finalize
helpers to satisfy the 80-line cap.
Frontend
- existingPrUrl returns undefined when a task has >1 PR so per-repo View PR
buttons take over (no silent linking to a sibling).
- formatRepoLabel checks exact known-repo match before prefix splitting so
"kandev-cli" isn't mislabeled as "kandev · cli".
- Repo chip key now includes base_branch so siblings differing only on
base render as distinct chips.
Docs
- multi-branch spec uniqueness wording aligned with the canonical 4-column
key (task_id, repository_id, base_branch, checkout_branch).
- worktree.CreateRequest.BranchSlug doc describes the sibling layout
(<repo>-<slug>) instead of the obsolete nested form.
* fix(tasks): scope slug-collision check to repo only + use errors.Is for ErrTaskNotFound
- service_branches: drop base_branch from the slug-collision check. Worktree
path is (repo, slug); two rows on the same repo cut from different bases
still collide on disk.
- service_branches: prepareBranchAdd wraps taskrepo.ErrTaskNotFound via
fmt.Errorf with %w so classifiers don't have to substring-match the
formatted task UUID.
- mcp handlers: classifyAddBranchError uses errors.Is(err, ErrTaskNotFound).
* fix(agentctl): sanitize rescan work_dir before os.Stat to clear CodeQL path-injection
CodeQL flagged the rescan endpoint as a path-injection sink (the
HTTP-supplied work_dir flowed straight into os.Stat and downstream
scanRepositorySubdirs). Add validateRescanPath which:
- requires the input to be a clean absolute path with no ".." segments
- rejects anything that isn't equal to or an ancestor of the current
cfg.WorkDir — the only legitimate use case is the branch materializer
promoting <task>/<repo>/ to <task>/
Defense in depth: agentctl binds to localhost, but any colocated process
could otherwise direct it at /etc or the user's home by POSTing
/api/v1/workspace/rescan.
* fix(agentctl): tighten validateRescanPath, close TOCTOU on existingTrackers, add tests
- Drop the strings.Contains(clean, "..") guard. filepath.Clean already
resolves traversal; the substring check rejects legitimate names like
/home/u/..hidden without adding defense (the ancestor check via
filepath.Rel is the real boundary).
- Drop the rel == "." branch in the ancestor check — early return for
clean == currentClean already covers identity.
- Read existingTrackers inside the write-lock section (not under RLock
then re-Lock) so two concurrent rescans can't both see 0 trackers and
double-bootstrap the multi-repo set.
- Add table-driven TestValidateRescanPath covering rejection paths
(relative, descendant, sibling, traversal-cleaned, empty input),
legitimate ancestor promotion, exact match, no-anchor first-launch,
and a /home/u/..hidden dotted-segment case.
* fix(agentctl): break taint flow into os.Stat — derive rescan path from cfg, not HTTP body
CodeQL's go/path-injection trace still flagged manager_rescan.go after the
prior sanitizer because the user-supplied newWorkDir flowed (after
validation) into os.Stat and then into cfg.WorkDir → tracker paths →
workspace_monitor os.Stat sinks.
Replace validateRescanPath with resolveRescanPath: it returns a path
DERIVED from currentWorkDir (no-op = currentClean, promotion =
filepath.Dir(currentClean)) and uses newPath only for comparison, never
as a source for the returned string. os.Stat now receives a path whose
provenance is trusted manager config.
Functionally the legitimate caller (branch materializer promoting
<task>/<repo>/ to <task>/) is unchanged.
* test(agentctl): make TestResolveRescanPath windows-portable
The Linux Backend Tests job passed but the Backend (windows) job rejected
every "absolute" case because filepath.IsAbs("/task/repo") is false on
Windows (no volume letter).
Build inputs via filepath.Join against absRoot() (filepath.Abs of "/" /
"\\") so the same table covers POSIX (`/...`) and Windows (`C:\...`)
without per-OS branches. Keep the traversal-after-Clean case POSIX-only
since filepath.Clean is the shared primitive and that input form needs a
volume to be considered absolute on Windows anyway.
* fix(agentctl): serialize RescanRepositories + re-check trackers under write lock
Two concurrent RescanRepositories calls could both observe an empty
tracker set (between the snapshot read and the bootstrap branch) and both
enter transitionToMultiRepoMode, leaking a duplicate bare-root tracker
and duplicate per-repo trackers. Same race in appendNewRepoTrackers would
double-append the same new child across both callers.
Fix:
- Add rescanMu sync.Mutex on Manager. Acquired for the full body of
RescanRepositories so the snapshot → branch → mutate sequence is
serialized. repoTrackersMu still guards individual field reads from
unrelated goroutines (subscribe/unsubscribe/poll-mode).
- appendNewRepoTrackers re-checks membership inside the write-lock
before appending and drops already-present trackers (Stop + detach
outside the lock). Defense in depth.
- Test: add `dotted segment is legal as promotion` case so the
..hidden-segment promotion path stays exercised.
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Carlos Florêncio