Chat Create Prompt No Op#669
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds ChangesChat read/send actions and CLI chat read command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
arul28
force-pushed
the
ade/ok-asked-agent-ade-new-9bca8ce0
branch
from
09d816a to
f36e8bb
Compare
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ade-cli/src/cli.test.ts (1)
1051-1484: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd transport-specific coverage for these chat flows
apps/ade-cli/src/cli.test.ts:1051-1484only exercises planning/formatting and a headless stub; add one headless and one desktop-socket-backed test forchat create --promptandchat readso dispatch/envelope mismatches in either mode are caught.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/cli.test.ts` around lines 1051 - 1484, Add transport-specific coverage for the chat flows by extending cli.test.ts with one headless and one desktop-socket-backed test for chat create --prompt and chat read. Reuse buildCliPlan, expectExecutePlan, expectStaticPlan, summarizeExecution, and the chat create/read assertions already in this block, but run them through both transport modes so dispatch and envelope differences are verified in executePlan and summarizeExecution. Ensure the new tests cover the createSession/sendMessage chaining and readTranscript envelope in both headless and desktop socket paths.Source: Coding guidelines
🧹 Nitpick comments (1)
apps/ade-cli/src/cli.test.ts (1)
1250-1263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the other kickoff aliases in this rejection test.
buildChatPlan(...)rejects--no-kickoffwith--prompt,--kickoff, and--kickoff-prompt, but this assertion only locks down the--promptspelling. A small table-driven test here would catch alias regressions too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/cli.test.ts` around lines 1250 - 1263, The rejection test for conflicting kickoff flags only covers the --prompt case, so it does not protect the other aliases handled by buildChatPlan. Update the test around buildCliPlan/buildChatPlan to be table-driven and verify that --no-kickoff is rejected with --prompt, --kickoff, and --kickoff-prompt, using the same conflict assertion pattern for each variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 3606-3621: The dry-run preview for the Linear attach step is
missing the attachment flags that the real flow passes through. Update the
`preview`/`afterCreate` construction in `cli.ts` so the
`lane.attachLinearIssueToSession` entry includes the same `attachmentFlags` data
produced by `readLinearAttachmentFlags(args)` and spread in the live path,
keeping the `compactPreviewObject` shape aligned with the actual `afterCreate`
payload.
- Around line 5889-5891: The `chat create` validation only blocks `--no-kickoff`
for plain chat create, but the same no-op prompt path can happen when
`--from-linear-issue` is used with explicit prompt aliases. Update the guard
around `explicitKickoff` in `cli.ts` so `--no-kickoff` is rejected whenever any
explicit prompt is provided, including Linear issue chat create, and keep the
error handling centralized in the same `CliUsageError` branch.
- Around line 15230-15231: The mapping in cli.ts is incorrectly renaming the
attach response from values.result to kickoff, which breaks the no-kickoff
Linear issue flow. Update the object assembly around unwrapActionEnvelope so the
attach step stays keyed as attach even when kickoff is skipped, or derive the
key from the action type before assigning it. Use the existing values.attach and
values.result branches to ensure the summary preserves the correct action name.
- Around line 5741-5762: The chat action path handled in cli.ts for sub values
like read/messages/transcript needs test coverage in both execution modes. Add
or extend tests around the CLI action builder so that chat.readTranscript and
the chained chat.sendMessage flow are exercised once through the headless
runtime and once through the desktop socket-backed RPC path, using the existing
actionStep and chat read/sendMessage symbols to locate the logic.
In `@apps/desktop/src/main/services/adeActions/registry.test.ts`:
- Around line 83-85: The ADE registry test coverage is missing the allowlist
check for the new chat send action. Update the assertions in registry.test.ts
near isAllowedAdeAction/isCtoOnlyAdeAction to include a direct expectation that
isAllowedAdeAction("chat", "sendMessage") is true, so the
ADE_ACTION_ALLOWLIST.chat.sendMessage path is verified alongside the existing
transcript checks.
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 1106-1116: `chat.sendMessage` is forwarding the parsed payload
without enforcing the required `text` contract at the action boundary. In
`sendMessage`, after `readObjectActionArg` and `requireNonEmptyString` for
`sessionId`, also validate `record.text` with `requireNonEmptyString` before
calling `agentChatService.sendMessage`. Keep the validation in the same action
handler so whitespace-only or missing text is rejected early, and continue
passing the normalized `sessionId` and validated `text` into
`agentChatService.sendMessage`.
---
Outside diff comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 1051-1484: Add transport-specific coverage for the chat flows by
extending cli.test.ts with one headless and one desktop-socket-backed test for
chat create --prompt and chat read. Reuse buildCliPlan, expectExecutePlan,
expectStaticPlan, summarizeExecution, and the chat create/read assertions
already in this block, but run them through both transport modes so dispatch and
envelope differences are verified in executePlan and summarizeExecution. Ensure
the new tests cover the createSession/sendMessage chaining and readTranscript
envelope in both headless and desktop socket paths.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 1250-1263: The rejection test for conflicting kickoff flags only
covers the --prompt case, so it does not protect the other aliases handled by
buildChatPlan. Update the test around buildCliPlan/buildChatPlan to be
table-driven and verify that --no-kickoff is rejected with --prompt, --kickoff,
and --kickoff-prompt, using the same conflict assertion pattern for each
variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 052b7999-e558-4b00-8e4a-7adf69ef6aa8
⛔ Files ignored due to path filters (3)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**
📒 Files selected for processing (6)
apps/ade-cli/README.mdapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f36e8bb375
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const since = typeof record.since === "string" && record.since.trim() | ||
| ? record.since.trim() | ||
| : undefined; | ||
| return agentChatService.readTranscript(sessionId, limit, since); |
There was a problem hiding this comment.
When the CLI falls back to its in-process headless runtime, agentChatService does not implement readTranscript; it exposes getChatTranscript instead (apps/ade-cli/src/headlessLinearServices.ts:1686). Because this wrapper is always added and calls agentChatService.readTranscript(...) unconditionally, ade chat read ... / chat.readTranscript now fail with a TypeError whenever no desktop/runtime socket is available, even though the new command is not marked socket-only. Guard this call or map to the headless transcript API.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
--limitoption.Bug Fixes
Documentation
Greptile Summary
This PR adds
--promptkickoff chaining toade chat create(creates the session then immediately sends the prompt as the first message), a newade chat read <session>subcommand, and fixes two issues from the previous review: the--no-kickoff + --promptconflict guard now covers the--from-linear-issuepath as well, andchat.sendMessagenow validates thetextfield before dispatching.cli.ts:buildChatPlangrows achat readbranch that callschat.readTranscript, a two-step plan forchat create --prompt(session + send), and thebuildChatCreateConfigPreviewdry-run now renders the fullafterCreatesequence (attach + send) so users can verify both steps before launching.registry.ts: explicitreadTranscriptandsendMessagehandlers added tobuildChatDomainService, with agetChatTranscript-based fallback for older runtimes that includes client-sidesincefiltering.Confidence Score: 5/5
Safe to merge — the two previously reported gaps are both addressed, the new code paths are well-tested, and no regressions were found on the existing chat create or linear-issue workflows.
All new code paths have corresponding unit tests, including edge cases (blank text, no-kickoff with linear issue, dry-run preview). The fix to always key the linear-issue attach step as "attach" (instead of conditionally using "result") is correctly reflected in the summarizeExecution handler and verified by test. No untested branches or missing validations were found in the changed files.
No files require special attention. The only comment is on the fallback path in registry.ts where stale pagination metadata may be returned after client-side since-filtering, which is a minor quality improvement rather than a correctness blocker.
Important Files Changed
chat readsubcommand,--promptkickoff chaining forchat create, conflict guard for--no-kickoff + --prompt,formatChatReadtext formatter, andsummarizeExecutionbranches for the new chat shapes. The attach-step key-change (alwaysattach) fixes a pre-existing labeling bug when--no-kickoffwas combined with--from-linear-issue.readTranscriptandsendMessageas explicit action handlers on the chat domain service, with input validation, agetChatTranscriptfallback, and client-sidesincefiltering in the fallback path.sendMessagenow validates thetextfield, fixing a prior gap.afterCreate,--no-kickoffconflict guard on both plain and linear-issue paths,chat readplan construction, transcript formatting, andsummarizeExecutionoutput shapes.readTranscript(including fallbackgetChatTranscriptpath with since-filter),sendMessagevalidation, and allowlist membership for both new actions.--promptkickoff semantics,ade chat read, and theade shell start-clireasoning-tier path.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant CLI as ade CLI participant ADE as ADE Runtime Note over User,ADE: ade chat create --prompt "fix tests" User->>CLI: chat create --lane L --provider claude --prompt "fix tests" CLI->>ADE: chat.createSession(laneId, provider, ...) ADE-->>CLI: "{ sessionId: "chat-new" }" CLI->>ADE: "chat.sendMessage({ sessionId: "chat-new", text: "fix tests" })" ADE-->>CLI: "{ ok: true, accepted: true }" CLI-->>User: "{ session: {...}, kickoff: {...} }" Note over User,ADE: ade chat read session-id User->>CLI: chat read session-id --limit 20 --since ISO CLI->>ADE: "chat.readTranscript({ sessionId, limit, since })" ADE-->>CLI: "{ entries: [...] }" CLI-->>User: formatted transcript Note over User,ADE: ade chat create --from-linear-issue ENG-431 User->>CLI: chat create --from-linear-issue ENG-431 CLI->>ADE: chat.createSession(...) ADE-->>CLI: "{ sessionId: "chat-new" }" CLI->>ADE: "lane.attachLinearIssueToSession({ chatSessionId, issues })" ADE-->>CLI: "{ linked: true }" CLI->>ADE: "chat.sendMessage({ sessionId, text: derivedKickoff })" ADE-->>CLI: "{ ok: true, accepted: true }" CLI-->>User: "{ session, attach, kickoff }"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant CLI as ade CLI participant ADE as ADE Runtime Note over User,ADE: ade chat create --prompt "fix tests" User->>CLI: chat create --lane L --provider claude --prompt "fix tests" CLI->>ADE: chat.createSession(laneId, provider, ...) ADE-->>CLI: "{ sessionId: "chat-new" }" CLI->>ADE: "chat.sendMessage({ sessionId: "chat-new", text: "fix tests" })" ADE-->>CLI: "{ ok: true, accepted: true }" CLI-->>User: "{ session: {...}, kickoff: {...} }" Note over User,ADE: ade chat read session-id User->>CLI: chat read session-id --limit 20 --since ISO CLI->>ADE: "chat.readTranscript({ sessionId, limit, since })" ADE-->>CLI: "{ entries: [...] }" CLI-->>User: formatted transcript Note over User,ADE: ade chat create --from-linear-issue ENG-431 User->>CLI: chat create --from-linear-issue ENG-431 CLI->>ADE: chat.createSession(...) ADE-->>CLI: "{ sessionId: "chat-new" }" CLI->>ADE: "lane.attachLinearIssueToSession({ chatSessionId, issues })" ADE-->>CLI: "{ linked: true }" CLI->>ADE: "chat.sendMessage({ sessionId, text: derivedKickoff })" ADE-->>CLI: "{ ok: true, accepted: true }" CLI-->>User: "{ session, attach, kickoff }"Reviews (2): Last reviewed commit: "Address chat CLI review feedback" | Re-trigger Greptile