Skip to content

Accept --session before subcommands in profiler-cli #6358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fatadel wants to merge 2 commits into
base: main
Choose a base branch
from
+79 −8
Open

Accept --session before subcommands in profiler-cli #6358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
abf1ae3
Extract the CLI session option helper
fatadel Sep 25, 2026
b63a8f2
Accept --session before CLI subcommands
fatadel Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions profiler-cli/guide.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,9 @@ SCRIPTING
profiler-cli profile info --session my-analysis
profiler-cli thread select t-0 --session my-analysis

The --session option can also appear before the command:

@fqueze fqueze Sep 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of adding these lines to the guide? The fact this bug was reported in the first place means some sessions tried to put the session parameter there already.

@fatadel fatadel Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For transparency. Do you think it'd be better not to have this line?

@canova canova Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I would also drop the guide.txt changes from this PR. With this fix --session just works wherever you put it, so users/agents don't need to think about where it goes. Mentioning it in the guide doesn't really help the agent. It only adds to the context it has to load, and we should keep that as small as we can.

profiler-cli --session my-analysis profile info

Always use --json for reliable output parsing. Plain text output is for human
reading and may change; the JSON schema is stable.

Expand Down
17 changes: 11 additions & 6 deletions profiler-cli/src/commands/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,21 @@ export function wasExplicit(parent: string, subcommand: string): boolean {
return idx !== -1 && args[idx + 1] === subcommand;
}

/**
* Add the --session option to a command.
*/
export function addSessionOption(cmd: Command): Command {
return cmd.option(
'--session <id>',
'Use a specific session (default: current session)'
);
}

/**
* Add --session and --json options to a command.
*/
export function addGlobalOptions(cmd: Command): Command {
return cmd
.option(
'--session <id>',
'Use a specific session (default: current session)'
)
.option('--json', 'Output results as JSON');
return addSessionOption(cmd).option('--json', 'Output results as JSON');
}

/**
Expand Down
13 changes: 11 additions & 2 deletions profiler-cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ import { startDaemon } from './daemon';
import { startNewDaemon, stopDaemon, sendCommand } from './client';
import { listSessions } from './session';
import { formatOutput } from './output';
import { addGlobalOptions, runCommand } from './commands/shared';
import {
addGlobalOptions,
addSessionOption,
runCommand,
} from './commands/shared';
import { VERSION } from './constants';
import { registerProfileCommand } from './commands/profile';
import { registerThreadCommand } from './commands/thread';
Expand Down Expand Up @@ -79,7 +83,12 @@ async function main(): Promise<void> {
return;
}

const program = new Command();
const program = addSessionOption(new Command());
program.hook('preAction', (rootCommand, actionCommand) => {
if (actionCommand.opts().session === undefined) {
actionCommand.setOptionValue('session', rootCommand.opts().session);
}
});
program
.name('profiler-cli')
.description('Profiler CLI — query Firefox profiles from the terminal')
Expand Down
54 changes: 54 additions & 0 deletions profiler-cli/src/test/integration/sessions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,60 @@ describe('profiler-cli multiple concurrent sessions', () => {
await cli(ctx, ['stop', '--session', session2]);
});

it('accepts --session before, between, and after subcommands', async () => {
const loaded = await cli(ctx, [
'--session',
'target-session',
'load',
'src/test/fixtures/upgrades/processed-1.json',
]);
expect(loaded.stdout).toContain('Session started: target-session');

await cli(ctx, [
'load',
'src/test/fixtures/upgrades/processed-2.json',
'--session',
'current-session',
]);

const expected = await cli(ctx, [
'profile',
'info',
'--session',
'target-session',
'--json',
]);
for (const args of [
['--session', 'target-session', 'profile', 'info'],
['profile', '--session', 'target-session', 'info'],
['--session=target-session', 'profile', 'info'],
[
'--session',
'missing-session',
'profile',
'info',
'--session',
'target-session',
],
]) {
const result = await cli(ctx, [...args, '--json']);
expect(JSON.parse(result.stdout)).toEqual(JSON.parse(expected.stdout));
}

const missing = await cliFail(ctx, [
'--session',
'missing-session',
'profile',
'info',
]);
expect(missing.stderr).toContain('missing-session');

await cli(ctx, ['--session', 'target-session', 'stop']);
const sessions = await cli(ctx, ['session', 'list']);
expect(sessions.stdout).not.toContain('target-session');
expect(sessions.stdout).toContain('current-session');
});

it('session list shows running sessions and marks the current one', async () => {
// Start two sessions
await cli(ctx, [
Expand Down
Loading