Skip to content

docs(agentctl): document the ACP session-transition lock so new session paths do not bypass it - #3396

Open
nova28 wants to merge 1 commit into
kdlbs:mainfrom
nova28:feature/serialize-acp-sessio-i7y
Open

docs(agentctl): document the ACP session-transition lock so new session paths do not bypass it#3396
nova28 wants to merge 1 commit into
kdlbs:mainfrom
nova28:feature/serialize-acp-sessio-i7y

Conversation

@nova28

@nova28 nova28 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Tip

PR walkthrough: Open the visual walkthrough

Contributors reading apps/backend/internal/agentctl/AGENTS.md had no written record that ACP session-transition RPCs (NewSession, LoadSession, ResetSession) must run serialized behind Adapter.sessionTransitionMu — the invariant that PR #3338 introduced to stop a concurrent transition from closing a session that had already become live again. Anyone touching this code path had to reconstruct that constraint by reading the lock sites themselves.

Today: the serialization invariant PR #3338 added exists only in code; nothing documents it, so the next contributor adding a session-transition path has no written warning that it must hold the mutex for the whole transition, not just the write.
After this: AGENTS.md states the invariant directly, naming the three serialized operations and the reason (agentctl dispatches WS requests to the adapter without serialization).
Who hits this: anyone extending ACP session lifecycle handling in apps/backend/internal/agentctl/server/adapter/transport/acp/.
Scope: standalone documentation follow-up from PR #3338's review (three independent reviewers flagged the missing invariant).
Not here: no production code changes — the fix already shipped in main via PR #3338's sessionTransitionMu.

Validation

This is a documentation-only change (one sentence appended to apps/backend/internal/agentctl/AGENTS.md). Verification confirmed the documented invariant against the live code and ran the full check gauntlet:

  • Verified every lock site (adapter_session.go: NewSession, LoadSession, ResetSession, closeSupersededSessionLocked) against the sentence added; confirmed ResetSession holds the mutex across newSession and closeSupersededSessionLocked end to end, and that the only production writers of a.sessionID (newSession, LoadSession) both run under the mutex.
  • make fmt
  • make typecheck (backend + web)
  • make lint (backend, web, harness, specs, architecture)
  • make lint-format
  • pnpm run i18n:ratchet — clean, no UI source touched
  • go test -tags fts5 ./... (backend, full suite): 13 packages failed. Reproduced the same 13 packages against a scratch worktree checked out at this branch's merge-base (ddcc4fc6a, pre-dating this change) — 12 failed identically (pre-existing sandbox filesystem restriction unrelated to any code, e.g. open npm cache root: not a directory), and the 13th (internal/orchestrator) failed only once, from a test-timeout caused by unrelated concurrent test runs on the shared build machine; it passed cleanly in isolation. None of these packages are touched by this diff (a single markdown file).

E2E was not required: the sole changed path is apps/backend/internal/agentctl/AGENTS.md, not under apps/web/ where Playwright specs live.

Possible Improvements

None — this documents an invariant that already shipped; no behavior changes.

Checklist

  • If I do not have repository write access and this is a large architectural change, I discussed the direction in a linked issue before opening this PR.
  • This PR contains one logical change; unrelated work is split into separate PRs.
  • I have performed a self-review of my code.
  • I have manually tested my changes and they work as expected.
  • My changes have tests that cover the new functionality and edge cases.
  • If my change touches UI files (apps/web/), I have added or updated Playwright e2e tests in apps/web/e2e/ and verified them with make test-e2e.
  • I checked whether this affects public docs in docs/public/** and updated them or noted why no docs change is needed.

Review in cubic

Preview Environment

URL https://kandev-pr-3396-bwo7.sprites.app
Commit aab19a0
Agent Mock agent

Updates automatically on each push. Destroyed when the PR is closed.

PR kdlbs#3338 added Adapter.sessionTransitionMu to serialize NewSession,
LoadSession, and ResetSession, but the invariant was never written
down where a future change to adapter_session.go would find it.
@nova28
nova28 temporarily deployed to opencode-review-trusted September 5, 2026 05:11 — with GitHub Actions Inactive
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nova28's task in 0s —— View job


I'll analyze this and get back to you.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Team

Run ID: 2f2d67e9-b073-485b-9ca8-c2ad51b015ea

📥 Commits

Reviewing files that changed from the base of the PR and between ddcc4fc and aab19a0.

📒 Files selected for processing (1)
  • apps/backend/internal/agentctl/AGENTS.md

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Documentation
    • Updated internal adapter guidance to improve coordination of session transitions and cleanup.

Walkthrough

The ACP adapter guidance now requires serialized handling of session lifecycle transitions, superseded-session cleanup, and all writes to a.sessionID using Adapter.sessionTransitionMu.

Changes

ACP session lifecycle guidance

Layer / File(s) Summary
Session transition locking
apps/backend/internal/agentctl/AGENTS.md
The guidance requires Adapter.sessionTransitionMu for the full NewSession, LoadSession, and ResetSession transitions, including session/new, superseded-session cleanup, and writes to a.sessionID.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to aab19

This change documents the existing ACP session-transition locking invariant without changing runtime behavior. No current merge-readiness risk remains.

Suggested reviewers: carlosflorencio

Poem

A rabbit guards the session gate
With locks held firm from start to state
New paths, old paths, cleanup too
Keep every session change in view
The ACP trail stays safe and straight

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the documentation goal, scope, validation performed, known test limitations, and checklist status. It includes minor template deviations, such as the generated Cubic a…
Title check ✅ Passed The title is concise, specific, and accurately describes documenting the ACP session-transition lock to prevent bypasses.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 5, 2026

Copy link
Copy Markdown

Greptile Summary

The PR adds concise, implementation-accurate guidance to the scoped agentctl engineering documentation.

  • Documents that ACP session transitions are serialized through Adapter.sessionTransitionMu.
  • Identifies the existing NewSession, LoadSession, and ResetSession paths.
  • Explains that the mutex must cover the whole transition rather than only the a.sessionID write.

Confidence Score: 5/5

The documentation-only change appears safe to merge.

The added guidance agrees with the current lock scopes, cleanup behavior, and production session-ID writes, with no actionable defects identified.

Important Files Changed

Filename Overview
apps/backend/internal/agentctl/AGENTS.md Adds accurate documentation of the ACP session-transition locking invariant and its concurrency rationale.

Reviews (1): Last reviewed commit: "docs(agentctl): document ACP session-tra..." | Re-trigger Greptile

@carlosflorencio
carlosflorencio self-requested a review September 9, 2026 19:05
@carlosflorencio

Copy link
Copy Markdown
Member

Thanks for the contribution. The added guidance records the existing session-transition mutex invariant for NewSession, LoadSession, ResetSession, and superseded-session cleanup. I found no additional code fix or unresolved review thread to apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants