Skip to content

AUTH-6733 fix: accept deprecated --no-commit/--commit flags - #220

Closed
nicknisi wants to merge 2 commits into
mainfrom
bosun/task-msi077q0-5xcn
Closed

AUTH-6733 fix: accept deprecated --no-commit/--commit flags#220
nicknisi wants to merge 2 commits into
mainfrom
bosun/task-msi077q0-5xcn

Conversation

@nicknisi

@nicknisi nicknisi commented Aug 6, 2026

Copy link
Copy Markdown
Member

bosun task: AUTH-6733 fix: accept deprecated --no-commit/--commit flags

Task id: task-msi077q0-5xcn
Shape: ship
Project: workos/cli

Refs: AUTH-6733 — #217

The installer no longer asks "Commit the changes?" or "Create a pull
request?" after a successful install. Committing and PR creation are the
user's workflow, not the tool's — changes are left uncommitted for review,
and the completion summary now ends with an explicit "Review the changes
(git status) and commit when ready" step.

Removed along with the prompts:
- postInstall machine states for commit/push/PR (promptingCommit through
  creatingPr, checkingGhCli, showingManualInstructions) plus their actions,
  actors, guards, and event types
- CLI/headless/dashboard adapter handlers for the commit and PR prompts
- --commit/--no-commit and --create-pr flags (the post-install git workflow
  they toggled no longer exists)
- post-install helpers (stageAndCommit, pushBranch, createPullRequest,
  getManualPrInstructions) and ai-content.ts (AI commit message / PR
  description generation), now unused
- hasGhCli/getDefaultBranch git utils, only used by the removed flow

detectChanges stays: it feeds the changed-files list in the completion
summary, which is how the user sees what to review. Branch creation and the
dirty-tree check (pre-install, --no-branch/--no-git-check) are unchanged.

BREAKING CHANGE: workos install no longer commits changes or creates PRs,
and the --no-commit and --create-pr flags are removed.

Refs: AUTH-6733
@linear-code

linear-code Bot commented Aug 6, 2026

Copy link
Copy Markdown

AUTH-6733

@nicknisi

nicknisi commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Closing as duplicate of #217 — both contain the same commits. The fix for Greptile's --no-commit flag comment landed on #217.

@nicknisi nicknisi closed this Aug 6, 2026
@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown

Greptile Summary

The PR removes automatic commit and pull-request creation from the installer while retaining --commit and --no-commit as deprecated no-op compatibility flags.

  • Simplifies the post-install state machine to detect and report changed files without committing them.
  • Removes obsolete commit/PR actors, events, adapter handlers, options, helpers, and documentation.
  • Adds an explicit manual review/commit step to completion output.
  • Adds subprocess coverage proving both deprecated commit flag forms parse successfully and warn only on stderr.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness, compatibility, or security defect identified.

The deprecated commit flags are accepted without restoring removed behavior, and the simplified installer lifecycle still reaches completion across every post-install detection outcome.

Important Files Changed

Filename Overview
src/bin.ts Adds a no-default boolean compatibility shim for deprecated commit flags and emits the intended stderr warning from both installer entry points.
src/bin-deprecated-flags.integration.spec.ts Exercises the real strict parser and verifies positive, negated, and absent deprecated-flag behavior.
src/lib/installer-core.ts Removes automatic commit/PR states while preserving complete transitions for changed, unchanged, and detection-error outcomes.
src/lib/run-with-core.ts Removes obsolete actor injection and headless options consistently with the simplified state machine.
src/lib/completion-data.ts Adds a manual review-and-commit next step only when changed files were detected.
src/utils/help-json.ts Keeps agent-facing JSON help synchronized with the deprecated commit flag accepted by the live parser.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[workos install] --> B[Parse installer flags]
  B --> C{commit flag supplied?}
  C -->|Yes| D[Write deprecation warning to stderr]
  C -->|No| E[Continue installation]
  D --> E
  E --> F[Run installer and modify project]
  F --> G[Detect changed files]
  G --> H[Build completion summary]
  H --> I[Leave changes uncommitted for review]
Loading

Reviews (1): Last reviewed commit: "fix: accept deprecated --no-commit/--com..." | Re-trigger Greptile

@devin-ai-integration devin-ai-integration Bot left a comment

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.

Devin Review found 4 potential issues.

Open in Devin Review

Comment thread src/bin.ts
Comment on lines 213 to 221
type: 'boolean' as const,
},
commit: {
default: true,
describe: 'Auto-commit after installation (use --no-commit to skip)',
type: 'boolean' as const,
},
'create-pr': {
default: false,
describe: 'Auto-create pull request after installation',
// Deprecated no-op kept for backward compatibility: the installer never
// commits, but scripts that still pass --commit/--no-commit must not fail
// strict parsing. No default so usage is detectable (undefined = absent).
describe: 'Deprecated: no-op flag, the installer never commits changes',
type: 'boolean' as const,
},

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.

🟡 Older automation scripts using the pull-request flag still crash

The backward-compatibility shim only accepts the two commit-related flags (commit option at src/bin.ts:213-221) while the previously documented pull-request flag was removed outright, so any existing script that still passes it is rejected outright instead of being ignored.
Impact: Automation that ran the installer with the old pull-request option now fails immediately with an "Unknown argument" usage error rather than continuing.

Strict yargs parsing rejects the removed --create-pr option

The base commit removed both --no-commit/--commit and --create-pr from installerOptions. This PR restores only commit as a deprecated no-op. Because the parser is configured with .strict() (src/bin.ts:2795), an unrecognized --create-pr triggers the .fail() path (src/bin.ts:257-273), which emits invalid_usage and exits with code 1. --create-pr was documented in README.md (removed in this PR) and in the machine-readable registry src/utils/help-json.ts (entry deleted here), so scripts and agents that discovered it earlier will break. If backward compatibility is the goal, create-pr should get the same deprecated no-op treatment as commit.

Prompt for agents
The PR reintroduces `--commit`/`--no-commit` as deprecated no-op flags so older scripts keep working under yargs `.strict()`, but the sibling flag `--create-pr` (removed in the same earlier change, and deleted from README.md and src/utils/help-json.ts in this PR) is not shimmed. Passing `--create-pr` now hits the `.fail()` handler in src/bin.ts and exits 1 with an "Unknown argument" usage error. Consider adding a `create-pr` boolean option to `installerOptions` in src/bin.ts with no default and a deprecated description, and extend the deprecation warning helper (`warnIfDeprecatedCommitFlag`) to cover it, so all removed post-install flags behave consistently as accepted no-ops.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +51 to +68
return spawnSync('bun', ['--preload', forceInsecureStorageImport, binPath, ...args], {
cwd: repoRoot,
encoding: 'utf-8',
env,
});
}

describe('deprecated install flags (backward-compat shims)', () => {
it('--no-commit is accepted as a no-op and warns on stderr', () => {
const result = runCli(['install', '--no-commit']);

// Past strict parsing: auth-required (4), not a validation error (1).
expect(result.status).toBe(4);
expect(result.stderr).not.toContain('Unknown argument');
expect(result.stderr).toContain('Deprecated flag: --no-commit');
// JSON/machine stdout stays clean.
expect(result.stdout).not.toContain('Deprecated flag');
}, 30_000);

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.

🔍 New spec drives the real CLI as a subprocess — slow and environment-sensitive

This spec is picked up by the default vitest run include glob (src/**/*.spec.ts in vitest.config.ts), so every bun run test spawns three real bun src/bin.ts install ... subprocesses (up to 30s each). The assertions depend on the CLI reaching the auth-required exit (4) after credential resolution fails against an unroutable WORKOS_API_URL; any change that makes credential resolution fail differently (e.g. a network error path that classifies as a general error) will flip the exit code to 1 and break these tests for reasons unrelated to flag parsing. Consider isolating it behind a separate integration test script/project, or asserting only that stderr lacks Unknown argument and contains the deprecation text rather than pinning the exit code.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread src/bin.ts
Comment on lines +249 to +256
function warnIfDeprecatedCommitFlag(argv: { commit?: boolean }): void {
if (argv.commit === undefined) return;
const flag = argv.commit ? '--commit' : '--no-commit';
renderStderrNotice(
`${pill('WARN', 'warn')} ${chalk.bold(`Deprecated flag: ${flag}`)} ${chalk.dim('— accepted as a no-op.')}`,
chalk.dim('The installer never commits changes; review and commit manually when ready.'),
);
}

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.

🔍 Deprecation notice is emitted even in JSON/agent runs

renderStderrNotice (src/utils/box.ts:9-13) unconditionally writes to stderr, unlike other startup notices (telemetry notice) which self-guard in JSON mode. The install/dashboard NDJSON stream on stdout stays clean, but agents that parse stderr expecting only {"error":{...}} objects (the documented contract in CLAUDE.md's Non-TTY Behavior section) will now see a free-form styled warning line. Worth confirming this is acceptable for the machine contract.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread src/lib/installer-core.ts
Comment on lines 951 to 952
},
},

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.

🔍 postInstall now always runs change detection

With shouldSkipPostInstall removed, detectChanges runs on every successful install (previously skipped when --no-commit was set). detectChanges shells out to git status --porcelain in the process cwd (src/utils/git-utils.ts:74-85), not options.installDir, so with --install-dir <elsewhere> the reported changedFiles (and hence the new "Review the changes (git status)" next step) can reflect the wrong repository or be empty. Pre-existing, but it now affects every run rather than only the commit path.

(Refers to lines 931-952)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant