Skip to content

feat(issue-5): align legacy wrapper commands with session contract - #14

Merged
nnennandukwe merged 2 commits into
mainfrom
codex/issue-5-legacy-wrapper-compatibility
Mar 19, 2026
Merged

nnennandukwe merged 2 commits into
mainfrom
codex/issue-5-legacy-wrapper-compatibility

Conversation

@nnennandukwe

Copy link
Copy Markdown
Owner

Summary

  • route legacy root wrapper commands through the explicit session command handlers
  • add optional --session <id> and --json support to the legacy wrapper surface where deterministic targeting is needed
  • add integration coverage and docs parity for safe single-session auto-resolution and ambiguous multi-session failures

Testing

  • coast lookup
  • coast exec dev-1 -- sh -c "cd /workspace && npm test -- tests/integration/cli.test.ts tests/unit/session-service.test.ts"
  • coast exec dev-1 -- sh -c "cd /workspace && npm run build"

Closes #5

@nnennandukwe

Copy link
Copy Markdown
Owner Author

/agentic_review

@qodo-code-review

qodo-code-review Bot commented Mar 19, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Grey Divider


Action required

View findings (1)
1. status succeeds with no session ☑ 📎 Requirement gap ✓ Correctness
Description
The legacy status wrapper returns a successful result with No active session. when zero sessions
match, instead of failing with SESSION_REQUIRED. This violates the requirement that legacy
convenience commands fail safely with clear guidance when no session exists.
Code

src/commands/session-status.ts[R15-18]

+  const result = await getStatus(context.cwd, sessionId ? { sessionId } : { allowLegacySingleActive: true });

 if (!result.active) {
   writeCommandSuccess(context, {
Evidence
PR Compliance ID 3 requires legacy convenience commands to fail with clear SESSION_REQUIRED
guidance when zero sessions match. The legacy status wrapper enables legacy auto-resolution and
then treats the zero-session case as a success (writeCommandSuccess) rather than raising
SESSION_REQUIRED.

Fail safely with clear guidance when zero sessions match
src/commands/status.ts[5-6]
src/services/session-service.ts[212-216]
src/commands/session-status.ts[15-22]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Legacy `threadloop status` currently returns success with `No active session.` when there are zero active sessions, but compliance requires failing safely with a clear `SESSION_REQUIRED` error.
## Issue Context
- The legacy wrapper calls `sessionStatusCommand(..., true)` (legacy single-active behavior enabled).
- `getStatus()` returns `active: null` when `allowLegacySingleActive` is true and there are zero active sessions.
- `sessionStatusCommand` converts that into a successful response via `writeCommandSuccess`.
## Fix Focus Areas
- src/commands/session-status.ts[15-22]
- src/services/session-service.ts[212-216]
- src/commands/status.ts[5-6]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

View findings (1)
2. Docs misstate status behavior ☑ 🐞 Bug ⚙ Maintainability
Description
README/docs state legacy threadloop status fails with SESSION_REQUIRED when there are zero
active sessions, but the implementation returns a successful "No active session." response instead.
This documentation mismatch can mislead users and any automation expecting an error code for the
no-session case.
Code

docs/cli.md[R68-73]

+Compatibility rules:
+- `start` preserves the legacy single-active-session behavior and refuses to open a second legacy root session in the same repo
+- `capture`, `status`, `artifact generate`, and `finish` auto-resolve only when exactly one active session exists
+- when zero sessions match, they fail with `SESSION_REQUIRED`
+- when multiple sessions match, they fail with `SESSION_AMBIGUOUS`
+- pass `--session <id>` or use the `threadloop session ...` forms for deterministic targeting
Evidence
The docs explicitly include status in the legacy commands that should fail with SESSION_REQUIRED
when zero sessions match, but getStatus() short-circuits to active: null when there are no
active sessions, and sessionStatusCommand() converts that into a success response; the integration
test asserts this behavior.

docs/cli.md[58-73]
README.md[25-37]
src/services/session-service.ts[212-216]
src/commands/session-status.ts[15-22]
tests/integration/cli.test.ts[378-383]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The docs/README state that legacy `status` fails with `SESSION_REQUIRED` when there are zero active sessions, but the CLI currently succeeds and prints/returns &amp;quot;No active session.&amp;quot; (including JSON success envelopes). This mismatch can confuse users and break automations written to the documented error behavior.
## Issue Context
Current implementation intentionally returns a friendly success response for `status` when no session is active.
## Fix Focus Areas
Update documentation (preferred, since tests codify current behavior) to exclude `status` from the “zero sessions =&amp;gt; SESSION_REQUIRED” rule, or explicitly call out that `status` returns a successful empty state.
- docs/cli.md[58-73]
- README.md[25-37]
(Optionally add/adjust a test to document the JSON no-session `status --json` behavior.)
- tests/integration/cli.test.ts[378-383]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

…active session

- Add explicit SESSION_REQUIRED check in sessionStatusCommand for legacy status
- Update documentation to correctly document status behavior
- Update test to expect SESSION_REQUIRED error instead of success message
@nnennandukwe

Copy link
Copy Markdown
Owner Author

Qodo Issues Fixed

Both Qodo issues have been addressed in this commit (de59880):

Issue 1: succeeds with no session (📎 Requirement gap)

Status: Fixed

Updated to throw error when legacy command is called with no active session. The fix adds an explicit check before the success response:

if (!result.active && !sessionId && allowLegacySingleActive) {
  throw new ThreadloopError('SESSION_REQUIRED', 'No active session.', {
    details: { hint: 'Start a session with threadloop session start.' },
  });
}

Issue 2: Docs misstate status behavior (🐞 Bug)

Status: Fixed

Updated documentation to correctly document that now fails with when zero sessions match:

  • : Updated compatibility rules section
  • : Updated compatibility rules section
  • : Updated test to expect error

All 29 tests pass, including the updated test case.

@nnennandukwe

Copy link
Copy Markdown
Owner Author

Fixed both Qodo issues in commit de59880. Issue 1: status command now throws SESSION_REQUIRED when no active session. Issue 2: Docs updated to reflect correct status behavior. All 29 tests pass.

@nnennandukwe
nnennandukwe merged commit 1453c69 into main Mar 19, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legacy command compatibility and ambiguity handling

1 participant