(feat/agent) Agent threads - #217
Open
nickscamara wants to merge 12 commits into
Open
Conversation
Threads let a follow-up continue an earlier agent run, so the CLI needs to know which thread the last run belonged to. Store it next to the other config-dir files, keyed by a hash of the API key so switching keys never crosses threads. Co-authored-by: Cursor <cursoragent@cursor.com>
A run can now continue a conversation (--thread/--continue/--new), pick a mode and effort, and pass Exchange options through to the API. Chat replies print before the JSON result, with follow-ups and any pending approval rendered from whatever the API returns. The pinned SDK drops unknown request keys and does not type the new response fields, so starts that use them go over raw HTTP and status reads widen the type; both can go once the SDK ships them. Co-authored-by: Cursor <cursoragent@cursor.com>
Pins the behaviour a follow-up depends on: which thread a run continues, that thread memory is per API key and dropped when the server no longer has the thread, that a chat reply leads the output, and that resolving an approval sends its control prompt. Resolving an approval now fails early when no thread is in play, since an approval only exists inside one. Co-authored-by: Cursor <cursoragent@cursor.com>
The new flags are only discoverable through --help otherwise, and follow-ups are the reason most people will reach for them. Co-authored-by: Cursor <cursoragent@cursor.com>
Locks in the precedence between --thread, --continue, --new and an approval being resolved. Co-authored-by: Cursor <cursoragent@cursor.com>
A follow-up inherits the previous turn's URLs and schema, and the only way to say "stop using those" is an empty list or an explicit null. Neither was reachable: both keys were dropped from the body when empty. --no-urls and --no-schema now send them. They only mean something inside a thread, and silently picking a winner when --urls and --no-urls are both passed would be worse than refusing, so both cases fail with a clear message. Runs without them post the same body as before. Co-authored-by: Cursor <cursoragent@cursor.com>
Member
Author
|
@cubic review |
@nickscamara I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/utils/agent-threads.ts">
<violation number="1" location="src/utils/agent-threads.ts:34">
P1: When the API key comes from environment, stored credentials, or top-level `--api-key`, callers pass `undefined` here, selecting `NO_API_KEY` instead of the effective key. Resolve and pass the effective key before calling these helpers, or resolve it in this module, so changing accounts cannot reuse the old thread.</violation>
</file>
<file name="src/commands/agent.ts">
<violation number="1" location="src/commands/agent.ts:397">
P1: When a run becomes approval-pending before completion, `--wait` keeps polling until timeout instead of showing the approval prompt. Return the pending status as soon as `agentStatus.pendingApproval` is present so the user can approve or decline it.</violation>
</file>
| * Short, stable hash of the API key. The key itself is never written to disk by | ||
| * this file; credentials.json already owns that. | ||
| */ | ||
| export function apiKeyFingerprint(apiKey?: string): string { |
There was a problem hiding this comment.
P1: When the API key comes from environment, stored credentials, or top-level --api-key, callers pass undefined here, selecting NO_API_KEY instead of the effective key. Resolve and pass the effective key before calling these helpers, or resolve it in this module, so changing accounts cannot reuse the old thread.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/agent-threads.ts, line 34:
<comment>When the API key comes from environment, stored credentials, or top-level `--api-key`, callers pass `undefined` here, selecting `NO_API_KEY` instead of the effective key. Resolve and pass the effective key before calling these helpers, or resolve it in this module, so changing accounts cannot reuse the old thread.</comment>
<file context>
@@ -0,0 +1,105 @@
+ * Short, stable hash of the API key. The key itself is never written to disk by
+ * this file; credentials.json already owns that.
+ */
+export function apiKeyFingerprint(apiKey?: string): string {
+ const key = apiKey?.trim();
+ if (!key) return NO_API_KEY;
</file context>
| data: agentStatus.data, | ||
| creditsUsed: agentStatus.creditsUsed, | ||
| expiresAt: agentStatus.expiresAt, | ||
| ...threadFields(agentStatus), |
There was a problem hiding this comment.
P1: When a run becomes approval-pending before completion, --wait keeps polling until timeout instead of showing the approval prompt. Return the pending status as soon as agentStatus.pendingApproval is present so the user can approve or decline it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/agent.ts, line 397:
<comment>When a run becomes approval-pending before completion, `--wait` keeps polling until timeout instead of showing the approval prompt. Return the pending status as soon as `agentStatus.pendingApproval` is present so the user can approve or decline it.</comment>
<file context>
@@ -144,6 +394,7 @@ async function checkAgentStatus(
data: agentStatus.data,
creditsUsed: agentStatus.creditsUsed,
expiresAt: agentStatus.expiresAt,
+ ...threadFields(agentStatus),
},
};
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Both cases spawned the built CLI with the ambient environment. On a machine with a key that lands in the agent command and the check runs; on CI there is no key, so the CLI stops at its login prompt, finds no stdin to answer, and exits 0. The assertions were reading that. They now run against a throwaway home with a key in the environment, so a remembered thread or a stored key on the machine cannot change the answer either. The key is never spent: both cases are rejected before a request. Co-authored-by: Cursor <cursoragent@cursor.com>
…uggestions Thread memory was keyed on the --api-key flag alone. The key almost always comes from the environment or stored credentials instead, so nearly every run landed in the one keyless bucket and changing accounts reused the previous account's thread. It is keyed on the server and the effective key now, which also stops two keyless self-hosted servers being handed each other's thread IDs and 404ing them away. Remembering a thread read the store and wrote it back, so two runs finishing together lost one of the entries and the loser silently started a new thread on its next --continue. The update takes a lock, writes through a rename, and still goes ahead if the lock cannot be taken: memory is a convenience and is not worth hanging a command over. agent thread <id> against a keyless --api-url was rejected, because the key requirement was read off global configuration rather than the URL the request resolved to. Suggestions were rendered into double quotes, which leaves $(...) and backticks live in something printed for a person to copy. They are single-quoted now. Restoring the spies in afterEach, so one failed assertion stops swallowing the output of every test after it. Co-authored-by: Cursor <cursoragent@cursor.com>
Commander gives a negated flag a true default only when no positive option already shares its name. --urls and --schema are declared ahead of their negations, which is the only reason an ordinary run leaves both unset; reorder either pair and every run without that flag reaches options.urls.split(true) and dies before asking for anything. Nothing at the call site shows that, and it has now been read as a live bug twice. A run against a closed port pins it: swapping the two lines turns the refused connection into "options.urls.split is not a function". Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Both were re-reported after the fix, so they are executable now rather than argued. The identity case fails against the previous behaviour: keyed off the --api-key flag, a run whose key comes from stored credentials remembers nothing, and the next account continues its thread. The approval case pins what a paid call actually does. An approval is written with the run's result, so it only reaches the client with a terminal status; --wait returns on the poll carrying it and prints the approve and decline commands. A timeout shorter than the poll budget makes a spinning loop fail rather than pass slowly. Co-authored-by: Cursor <cursoragent@cursor.com>
--urls and --no-urls share one Commander attribute, as do the schema pair, and that attribute carries three different answers: a string, false when cleared, or nothing. The parsing read it directly, which was correct only because each positive option happens to be declared before its negation; Commander defaults a negated flag to true when it is not, and that true would reach the URL split. Reading each intent by type makes the parsing say what it wants and stops it depending on the order two adjacent option calls appear in. Reordering them is now a no-op, so the comment claiming otherwise is gone and the test that covered the order covers the behaviour instead. Co-authored-by: Cursor <cursoragent@cursor.com>
…ud URL by value The lock lives in the config directory, so on a machine that had never run the CLI it could not be created: the first write spent all twenty attempts failing to make one, 249ms, and then wrote unlocked, which is exactly when two runs are both likely to be first. The directory is made before the lock now, and that write takes 5ms. isCustomApiUrl compared the URL as a string, so any cased or trailing slashed spelling of the cloud API read as self-hosted. That waived the key requirement and sent the request to Firecrawl with no Authorization header at all. It compares through a shared normalizer now, which agent thread memory uses for the same question and no longer defines for itself. The lock test claimed to cover the stale branch while creating a lock with a current mtime, which only ever exercised the fallback. It says what it does, and the stale path has its own case with an aged mtime. The argv spawns take a timeout: none of them should reach a round trip, and asserting on a refused connection made the suite depend on nothing listening on that port. It asserts on the failure the CLI reports instead. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/agent.ts">
<violation number="1" location="src/commands/agent.ts:97">
P1: When a custom URL is stored globally but `agent thread` receives a differently cased cloud `--api-url`, this branch still permits a keyless cloud request. `validateConfig(apiKey)` checks the stale global URL instead of `baseUrl`; validate against the resolved URL or check the resolved key directly.</violation>
</file>
<file name="src/__tests__/cli-argv.test.ts">
<violation number="1" location="src/__tests__/cli-argv.test.ts:30">
P3: The new comment says "None of these cases should reach a network round trip," but the 'leaves URLs and schema unset' test in this same file deliberately passes `--api-url http://127.0.0.1:9` to reach a failed request. Drop the claim or reword to "no remote call that would block," since the point of the timeout is to fail a hang, and the localhost refusal is the very round trip the assertion relies on.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // points at a self-hosted server, and those need no key. Compared through | ||
| // the normalizer, so a differently cased cloud URL is still the cloud and | ||
| // cannot slip through unauthenticated. | ||
| if (isDefaultApiUrl(baseUrl)) { |
There was a problem hiding this comment.
P1: When a custom URL is stored globally but agent thread receives a differently cased cloud --api-url, this branch still permits a keyless cloud request. validateConfig(apiKey) checks the stale global URL instead of baseUrl; validate against the resolved URL or check the resolved key directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/agent.ts, line 97:
<comment>When a custom URL is stored globally but `agent thread` receives a differently cased cloud `--api-url`, this branch still permits a keyless cloud request. `validateConfig(apiKey)` checks the stale global URL instead of `baseUrl`; validate against the resolved URL or check the resolved key directly.</comment>
<file context>
@@ -91,8 +91,10 @@ function resolveApiBase(options: { apiKey?: string; apiUrl?: string }): {
+ // points at a self-hosted server, and those need no key. Compared through
+ // the normalizer, so a differently cased cloud URL is still the cloud and
+ // cannot slip through unauthenticated.
+ if (isDefaultApiUrl(baseUrl)) {
validateConfig(apiKey);
}
</file context>
Comment on lines
+30
to
+31
| // None of these cases should reach a network round trip, so anything | ||
| // that blocks is a broken assumption. Failing on it beats a suite that |
There was a problem hiding this comment.
P3: The new comment says "None of these cases should reach a network round trip," but the 'leaves URLs and schema unset' test in this same file deliberately passes --api-url http://127.0.0.1:9 to reach a failed request. Drop the claim or reword to "no remote call that would block," since the point of the timeout is to fail a hang, and the localhost refusal is the very round trip the assertion relies on.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/__tests__/cli-argv.test.ts, line 30:
<comment>The new comment says "None of these cases should reach a network round trip," but the 'leaves URLs and schema unset' test in this same file deliberately passes `--api-url http://127.0.0.1:9` to reach a failed request. Drop the claim or reword to "no remote call that would block," since the point of the timeout is to fail a hang, and the localhost refusal is the very round trip the assertion relies on.</comment>
<file context>
@@ -27,6 +27,11 @@ describe('CLI argv parsing', () => {
return spawnSync(process.execPath, [cliPath, ...args], {
cwd: process.cwd(),
encoding: 'utf8',
+ // None of these cases should reach a network round trip, so anything
+ // that blocks is a broken assumption. Failing on it beats a suite that
+ // hangs until the runner gives up.
</file context>
Suggested change
| // None of these cases should reach a network round trip, so anything | |
| // that blocks is a broken assumption. Failing on it beats a suite that | |
| // None of these cases should make a blocking network call, so anything | |
| // that hangs is a broken assumption. Failing on it beats a suite that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Adds agent threads so follow-up runs continue an earlier conversation with full context, and adds chat mode, Exchange approvals, and a
firecrawl agent thread <id>command to print a conversation. The CLI now remembers the last thread per API key, so--continuepicks it up without pasting an ID.Threads and follow-ups
--thread <id>,--continue, and--newcontrol which conversation a run belongs to;--threadwins over--continue.--no-urlsand--no-schemadrop context inherited from earlier turns on follow-ups; passing them with explicit--urls/--schemafails with a clear error.--urls/--no-urlsand--schema/--no-schemaattributes are read by type, so declaration order no longer matters.agent-threads.jsonkeyed by a hash of the server and effective key, never the key itself, so switching accounts or servers never crosses threads; concurrent updates take a lock, including on a machine that has never run the CLI.agent thread <id>reads keyless self-hosted servers through--api-urlwithout a key.Chat mode, Exchange, and approvals
--mode chatprints the agent's prose reply before the JSON result and lists suggested follow-ups, single-quoted so copying them can't run anything.--exchange,--toolkits,--max-calls,--require-approval) pass through to the API.--approve/--declineresolve a pending approval with a fixed control prompt and require a thread to resolve it in.firecrawlSDK drops unknown request keys; runs without them post the same body as before.Written for commit cf6ecb0. Summary will update on new commits.