feat(cli): implement phased plans for rollout and cutover#439
Merged
calvinbrewer merged 2 commits intomainfrom May 8, 2026
Merged
feat(cli): implement phased plans for rollout and cutover#439calvinbrewer merged 2 commits intomainfrom
calvinbrewer merged 2 commits intomainfrom
Conversation
|
📝 WalkthroughWalkthroughAdds plan-step-aware plan summaries and context threading, per-column rollout-state detection and DB readers, plan-step selection with a --complete-rollout override, impl-time deploy-gate verification for cutover plans, a quest-log status model with TTY/plain/JSON renderers, expanded tests, and documentation updates across integrations. ChangesColumn Rollout State Detection & Classification
Plan Summary Schema & Context Threading
Plan Step Detection & Selection
Multi-Step Plan & Implement Prompts
Deploy-Gate Enforcement at Impl
Status Command Quest-Log System
CLI & Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
calvinbrewer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@packages/cli/src/bin/stash.ts`:
- Around line 399-409: Add an end-to-end test that exercises the top-level CLI
dispatch in stash.ts to lock argv parsing and exit behavior: spawn the CLI
binary (or invoke the bin entry) with the commands 'plan', 'impl', and 'status'
using flags that cover flags.quest, flags.plain, and flags.json, assert the
invoked handlers (planCommand, implCommand, statusCommand) run with expected
args, and verify correct process exit codes and JSON/plain output semantics;
ensure the test fails if unknown flags or incorrect wiring change command
selection or exit status.
In `@packages/cli/src/commands/impl/index.ts`:
- Around line 87-90: The current filter blocks any state not classified as
'cutover' which incorrectly includes completed phases (e.g., dropped) and also
relies on detectColumnStates collapsing DB errors into hard blocks; change the
verifier call to use an error-signaling lookup path (e.g., call
detectColumnStates with a verifier option or use detectColumnStatesForVerifier
so DB outages return null/undefined phases) and tighten the predicate to only
treat true pre-dual-write phases as blockers (replace the broad filter with a
check like isPreDualWritePhase(classifyPhase(s.phase)) or compare against the
explicit pre-dual-write phase names), keeping the map to ({ table: s.table,
column: s.column }).
In `@packages/cli/src/commands/init/lib/setup-prompt.ts`:
- Around line 421-433: The example JSON emitted by planSummaryBlockExample is
invalid because it uses the TypeScript/DSL syntax `"path": "new" | "migrate"`;
update the returned example so the "path" field is a valid JSON string (e.g.
`"path": "new"` or `"path": "migrate"`) instead of the pipe expression so the
<cipherstash:plan-summary> header is parseable; locate function
planSummaryBlockExample and replace that array element accordingly.
In `@packages/cli/src/commands/plan/index.ts`:
- Around line 56-100: The call to detectColumnStates inside detectPlanStep can
throw on DB connection errors but isn't caught, violating the doc promise to
fall back to 'rollout' when the database is unreachable; wrap the await
detectColumnStates(databaseUrl, columns) call in a try/catch (or broaden the
existing try) and on any error log or ignore the error and return 'rollout' so
rollupPlanStep is only called for successful state detection and the function
always returns 'rollout' on DB failures.
In `@packages/cli/src/commands/status/__tests__/status.test.ts`:
- Around line 110-120: The test title says "no signals" but both quest and
noEvents are created with phase: 'dual-writing' and the assertions don't check
the claimed states; change noEvents to be created without a phase signal (e.g.
call buildColumnQuest without phase or pass an empty signal object) so it
represents the no-signals path, then add assertions:
expect(noEvents.path).toBe('migrate') (keep),
expect(noEvents.progress.done).toBe(0), expect(noEvents.progress.total).toBe(5)
(or match existing total), expect(noEvents.objectives[0].status).toBe('active')
and assert that the remaining objectives (objectives[1..]) have status 'locked'
so the test validates the title; use the existing variables buildColumnQuest,
quest, noEvents, progress, and objectives to locate and update the code.
In `@packages/cli/src/commands/status/index.ts`:
- Around line 214-223: The current logic uses flags.quest to run the Clack frame
functions (p.intro, p.note, p.outro) even when stdout isn't a TTY; split frame
vs body decisions so frames only run when process.stdout.isTTY && !flags.plain
but the fancy body can still be forced by flags.quest. Change the code around
useTTY to compute enableFrames = process.stdout.isTTY && !flags.plain and
compute body = flags.quest ? renderQuestLogTTY(log) : renderQuestLogPlain(log);
then if (enableFrames) call p.intro('CipherStash'), p.note(body, 'Quest log'),
p.outro(nextMoveHint(log, cli)); otherwise write the chosen body and the Next
line directly to stdout using process.stdout.write. Ensure you only call
p.intro/p.note/p.outro when enableFrames is true and keep nextMoveHint(cli)
usage identical.
In `@packages/cli/src/commands/status/quest.ts`:
- Around line 89-97: The current inferQuestPath(ColumnObservation) returns
'migrate' when obs.phase === undefined which incorrectly treats manifest-only
columns as migrate flows; update inferQuestPath to first check for a
manifest-driven signal (e.g., a field you populate from readManifest(), such as
obs.manifestPresent or obs.manifestPath) and return the
manifest/“new”/manifest-specific quest path when a manifest exists; if no
manifest info is available, return an explicit unobserved/unknown value (e.g.,
'unknown' or 'unobserved') instead of defaulting to 'migrate' so downstream
logic (stash status, deploy guidance) does not assume migrate behavior. Ensure
you reference and use the same ColumnObservation property that readManifest()
sets and adjust callers that expect only 'migrate'/'new' to handle the new
unknown/manifest value returned by inferQuestPath.
In `@skills/stash-drizzle/SKILL.md`:
- Around line 341-344: Remove the blank line between the two adjacent blockquote
paragraphs so the blockquotes are contiguous (i.e., merge "**Runner note.**
Examples below use `npx stash`..." and "**Where am I?** Run `stash status`
first..." into back-to-back blockquote lines) to satisfy markdownlint MD028;
update the SKILL.md block containing those two blockquote entries so there is no
empty line separating them.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03bbb83a-7f59-48d9-9a96-7610a0d0533b
📒 Files selected for processing (20)
packages/cli/src/bin/stash.tspackages/cli/src/commands/impl/index.tspackages/cli/src/commands/init/lib/__tests__/parse-plan.test.tspackages/cli/src/commands/init/lib/__tests__/rollout-state.test.tspackages/cli/src/commands/init/lib/__tests__/setup-prompt.test.tspackages/cli/src/commands/init/lib/parse-plan.tspackages/cli/src/commands/init/lib/rollout-state.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/init/lib/write-context.tspackages/cli/src/commands/init/types.tspackages/cli/src/commands/plan/index.tspackages/cli/src/commands/status/__tests__/status.test.tspackages/cli/src/commands/status/index.tspackages/cli/src/commands/status/quest.tspackages/cli/src/commands/status/render.tsskills/stash-cli/SKILL.mdskills/stash-drizzle/SKILL.mdskills/stash-dynamodb/SKILL.mdskills/stash-encryption/SKILL.mdskills/stash-supabase/SKILL.md
Comment on lines
+399
to
+409
| await planCommand(flags) | ||
| break | ||
| case 'impl': | ||
| await implCommand(flags) | ||
| break | ||
| case 'status': | ||
| await statusCommand() | ||
| await statusCommand({ | ||
| quest: flags.quest, | ||
| plain: flags.plain, | ||
| json: flags.json, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add an E2E test for the new CLI flag wiring
Line 399 and Lines 405-409 change top-level CLI dispatch behavior; this needs an accompanying E2E test to lock argv/command behavior and exit semantics.
As per coding guidelines, packages/cli/src/bin/stash.ts: “Add an E2E test when touching src/bin/stash.ts argv parsing, exit codes, or top-level error handling.”
🤖 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 `@packages/cli/src/bin/stash.ts` around lines 399 - 409, Add an end-to-end test
that exercises the top-level CLI dispatch in stash.ts to lock argv parsing and
exit behavior: spawn the CLI binary (or invoke the bin entry) with the commands
'plan', 'impl', and 'status' using flags that cover flags.quest, flags.plain,
and flags.json, assert the invoked handlers (planCommand, implCommand,
statusCommand) run with expected args, and verify correct process exit codes and
JSON/plain output semantics; ensure the test fails if unknown flags or incorrect
wiring change command selection or exit status.
Comment on lines
+87
to
+90
| const states = await detectColumnStates(databaseUrl, migrate) | ||
| return states | ||
| .filter((s) => classifyPhase(s.phase) !== 'cutover') | ||
| .map((s) => ({ table: s.table, column: s.column })) |
There was a problem hiding this comment.
Cutover gate predicate can block valid/completed states and transient DB failures
Line 89 currently blocks anything that is not classified as cutover, which incorrectly includes dropped (already complete). Also, detectColumnStates collapses DB errors into unknown/null phases, so transient DB issues can become hard blocks instead of “could not verify, continue” as documented.
Use a stricter precondition check for “pre-dual-write phases only” and use an error-signaling lookup path for this verifier so DB outages return null instead of a false negative block.
🤖 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 `@packages/cli/src/commands/impl/index.ts` around lines 87 - 90, The current
filter blocks any state not classified as 'cutover' which incorrectly includes
completed phases (e.g., dropped) and also relies on detectColumnStates
collapsing DB errors into hard blocks; change the verifier call to use an
error-signaling lookup path (e.g., call detectColumnStates with a verifier
option or use detectColumnStatesForVerifier so DB outages return null/undefined
phases) and tighten the predicate to only treat true pre-dual-write phases as
blockers (replace the broad filter with a check like
isPreDualWritePhase(classifyPhase(s.phase)) or compare against the explicit
pre-dual-write phase names), keeping the map to ({ table: s.table, column:
s.column }).
Comment on lines
+421
to
+433
| function planSummaryBlockExample(step: PlanStep): string { | ||
| return [ | ||
| ' ```', | ||
| ' <!-- cipherstash:plan-summary', | ||
| ' {', | ||
| ` "step": "${step}",`, | ||
| ' "columns": [', | ||
| ' {"table": "<table_name>", "column": "<column_name>", "path": "new" | "migrate"}', | ||
| ' ]', | ||
| ' }', | ||
| ' -->', | ||
| ' ```', | ||
| ].join('\n') |
There was a problem hiding this comment.
Make the cipherstash:plan-summary example valid JSON.
This helper is presented as the exact parseable shape, but "path": "new" | "migrate" is not valid JSON. If an agent copies it literally, stash impl can't parse the header it depends on.
Suggested fix
function planSummaryBlockExample(step: PlanStep): string {
return [
' ```',
' <!-- cipherstash:plan-summary',
' {',
` "step": "${step}",`,
' "columns": [',
- ' {"table": "<table_name>", "column": "<column_name>", "path": "new" | "migrate"}',
+ ' {"table": "<table_name>", "column": "<column_name>", "path": "new"}',
' ]',
' }',
' -->',
' ```',
].join('\n')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function planSummaryBlockExample(step: PlanStep): string { | |
| return [ | |
| ' ```', | |
| ' <!-- cipherstash:plan-summary', | |
| ' {', | |
| ` "step": "${step}",`, | |
| ' "columns": [', | |
| ' {"table": "<table_name>", "column": "<column_name>", "path": "new" | "migrate"}', | |
| ' ]', | |
| ' }', | |
| ' -->', | |
| ' ```', | |
| ].join('\n') | |
| function planSummaryBlockExample(step: PlanStep): string { | |
| return [ | |
| ' |
🤖 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 `@packages/cli/src/commands/init/lib/setup-prompt.ts` around lines 421 - 433,
The example JSON emitted by planSummaryBlockExample is invalid because it uses
the TypeScript/DSL syntax `"path": "new" | "migrate"`; update the returned
example so the "path" field is a valid JSON string (e.g. `"path": "new"` or
`"path": "migrate"`) instead of the pipe expression so the
<cipherstash:plan-summary> header is parseable; locate function
planSummaryBlockExample and replace that array element accordingly.
Comment on lines
+110
to
120
| it('with no signals: 0/5, schema-add active, rest locked', () => { | ||
| const quest = buildColumnQuest(obs({ phase: 'dual-writing' })) | ||
| // Picking dual-writing forces the migrate path; reset and check no-events. | ||
| const noEvents = buildColumnQuest({ | ||
| table: 'users', | ||
| column: 'email', | ||
| phase: 'dual-writing', | ||
| }) | ||
| const stages = buildStages(readProjectStatus(cwd), 'pnpm dlx stash') | ||
| expect(stages[0].detail).toContain('1 table') | ||
| expect(stages[0].detail).not.toContain('1 tables') | ||
| expect(noEvents.path).toBe('migrate') | ||
| expect(quest.progress.total).toBe(5) | ||
| }) |
There was a problem hiding this comment.
Test does not validate what its title claims.
Title says "with no signals: 0/5, schema-add active, rest locked", but both quest and noEvents are built with phase: 'dual-writing' — that is the strongest possible signal, not "no signals". The only assertions are noEvents.path === 'migrate' and quest.progress.total === 5; nothing checks done: 0, objectives[0].status === 'active', or that the rest are locked. The test will pass even if buildColumnQuest regresses on the no-signals path.
💚 Proposed fix
- it('with no signals: 0/5, schema-add active, rest locked', () => {
- const quest = buildColumnQuest(obs({ phase: 'dual-writing' }))
- // Picking dual-writing forces the migrate path; reset and check no-events.
- const noEvents = buildColumnQuest({
- table: 'users',
- column: 'email',
- phase: 'dual-writing',
- })
- expect(noEvents.path).toBe('migrate')
- expect(quest.progress.total).toBe(5)
- })
+ it('with no signals: 0/5, schema-add active, rest locked', () => {
+ // No phase, no EQL state, no physical twin column — the genuine
+ // "no signals" case for a column the user declared in
+ // migrations.json but for which nothing has happened yet.
+ const quest = buildColumnQuest(
+ obs({ phase: null, eql: null, physicalEncryptedTwinExists: false }),
+ )
+ expect(quest.path).toBe('migrate')
+ expect(quest.progress).toEqual({ done: 0, total: 5 })
+ expect(quest.objectives[0].status).toBe('active')
+ expect(quest.objectives.slice(1).every((o) => o.status === 'locked')).toBe(true)
+ })🤖 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 `@packages/cli/src/commands/status/__tests__/status.test.ts` around lines 110 -
120, The test title says "no signals" but both quest and noEvents are created
with phase: 'dual-writing' and the assertions don't check the claimed states;
change noEvents to be created without a phase signal (e.g. call buildColumnQuest
without phase or pass an empty signal object) so it represents the no-signals
path, then add assertions: expect(noEvents.path).toBe('migrate') (keep),
expect(noEvents.progress.done).toBe(0), expect(noEvents.progress.total).toBe(5)
(or match existing total), expect(noEvents.objectives[0].status).toBe('active')
and assert that the remaining objectives (objectives[1..]) have status 'locked'
so the test validates the title; use the existing variables buildColumnQuest,
quest, noEvents, progress, and objectives to locate and update the code.
Comment on lines
+214
to
+223
| const useTTY = flags.quest ?? (process.stdout.isTTY && !flags.plain) | ||
|
|
||
| const deeper = [ | ||
| `Database state: \`${cli} db status\``, | ||
| `Per-column state: \`${cli} encrypt status\``, | ||
| ].join('\n') | ||
| p.note(deeper, 'Deeper inspection') | ||
| if (useTTY) { | ||
| p.intro('CipherStash') | ||
| p.note(renderQuestLogTTY(log), 'Quest log') | ||
| p.outro(nextMoveHint(log, cli)) | ||
| } else { | ||
| process.stdout.write(`${renderQuestLogPlain(log)}\n`) | ||
| process.stdout.write(`Next: ${nextMoveHint(log, cli)}\n`) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file and exact code at the specified lines
cat -n packages/cli/src/commands/status/index.ts | head -230 | tail -40Repository: cipherstash/stack
Length of output: 1620
🏁 Script executed:
# Get broader context to understand the full function
wc -l packages/cli/src/commands/status/index.tsRepository: cipherstash/stack
Length of output: 107
🏁 Script executed:
# Find the render functions referenced
rg -n "renderQuestLogTTY|renderQuestLogPlain|nextMoveHint" packages/cli/src/commands/status/index.tsRepository: cipherstash/stack
Length of output: 402
🏁 Script executed:
# Check where these functions are defined
fd -t f "*.ts" -x rg -l "renderQuestLogTTY|renderQuestLogPlain" {} \;Repository: cipherstash/stack
Length of output: 345
🏁 Script executed:
# Check the flags definition to understand what --quest is meant to do
rg -B 5 -A 5 "quest.*=" packages/cli/src/commands/status/index.ts | head -50Repository: cipherstash/stack
Length of output: 43
🏁 Script executed:
# Find the definitions of render functions
fd --glob "*.ts" --exec rg -l "export.*renderQuestLogTTY|export.*renderQuestLogPlain" {} \;Repository: cipherstash/stack
Length of output: 106
🏁 Script executed:
# Check the import statements to find where they're from
head -30 packages/cli/src/commands/status/index.tsRepository: cipherstash/stack
Length of output: 954
🏁 Script executed:
# Look at what `@clack/prompts` p.note and related functions do
rg -A 10 "import.*from.*@clack" packages/cli/src/commands/status/index.tsRepository: cipherstash/stack
Length of output: 437
🏁 Script executed:
# Check the StatusFlags interface to see what flags are available
rg -B 5 -A 15 "interface StatusFlags|type StatusFlags" packages/cli/src/commands/status/index.tsRepository: cipherstash/stack
Length of output: 766
🏁 Script executed:
# Look at the render functions to understand the difference
cat -n packages/cli/src/commands/status/render.ts | head -80Repository: cipherstash/stack
Length of output: 2994
🏁 Script executed:
# See how renderQuestLogTTY and renderQuestLogPlain differ
rg -A 20 "export function renderQuestLogTTY|export function renderQuestLogPlain" packages/cli/src/commands/status/render.tsRepository: cipherstash/stack
Length of output: 1406
🏁 Script executed:
# Check what `@clack/prompts` functions return/do in non-TTY contexts
# by looking for any documentation or usage patterns
rg -A 5 -B 5 "p\.(intro|note|outro)" packages/cli/src/commands/status/index.tsRepository: cipherstash/stack
Length of output: 441
🏁 Script executed:
# Look for any other places where flags.quest is used or tested
rg -n "flags\.quest|quest.*true" packages/cli/src/commands/status/Repository: cipherstash/stack
Length of output: 493
🏁 Script executed:
# Check if there's any test that validates the TTY behavior
fd --glob "*.test.ts" --glob "*.spec.ts" packages/cli/src/commands/status/ 2>/dev/null || echo "No test files found in pattern"Repository: cipherstash/stack
Length of output: 119
🏁 Script executed:
# Check the test file for the status command
cat -n packages/cli/src/commands/status/__tests__/status.test.ts | head -100Repository: cipherstash/stack
Length of output: 3668
🏁 Script executed:
# Search for tests related to TTY or quest flags
rg -n "quest|isTTY|useTTY" packages/cli/src/commands/status/__tests__/status.test.tsRepository: cipherstash/stack
Length of output: 3093
🏁 Script executed:
# Check if there's any documentation on `@clack/prompts` behavior in non-TTY contexts
rg -r "clack.*prompts" packages/cli/package.jsonRepository: cipherstash/stack
Length of output: 257
🏁 Script executed:
# See what the actual `@clack/prompts` package version is and check its documentation
cat packages/cli/package.json | grep -A 5 -B 5 "@clack"Repository: cipherstash/stack
Length of output: 334
🏁 Script executed:
# Check if there are any issues or comments about `@clack/prompts` in non-TTY contexts
rg -i "clack|tty|non-tty|isTTY" packages/cli/src/ -A 2 -B 2 | head -50Repository: cipherstash/stack
Length of output: 3716
🏁 Script executed:
# Look at the exact behavior: what does p.note do and when
# Check if there's any test that mocks stdout.isTTY
rg -n "isTTY" packages/cli/src/commands/status/__tests__/Repository: cipherstash/stack
Length of output: 43
🏁 Script executed:
# See if statusCommand is tested with different flag combinations
rg -B 5 -A 10 "statusCommand" packages/cli/src/commands/status/__tests__/status.test.tsRepository: cipherstash/stack
Length of output: 43
🏁 Script executed:
# Look for any issue reports or comments about `@clack/prompts` behavior
rg -i "clack.*pipe|clack.*non.tty|clack.*ci|prompts.*break|prompts.*fail" packages/cli/src/Repository: cipherstash/stack
Length of output: 309
🏁 Script executed:
# Check the `@clack/prompts` package documentation or types
cat packages/cli/package-lock.json 2>/dev/null | grep -A 20 '"@clack/prompts"' | head -40Repository: cipherstash/stack
Length of output: 43
🏁 Script executed:
# Look at the exact comment about TTY frames in the code
cat -n packages/cli/src/commands/status/index.ts | sed -n '210,223p'Repository: cipherstash/stack
Length of output: 679
Keep forced --quest output out of the Clack frame when stdout isn't a TTY.
flags.quest currently forces p.intro, p.note, and p.outro even for pipes and CI environments. The Clack decorative frames are documented as TTY-only (see lines 210–211), but the --quest flag bypasses this check. The intended behavior is to force the fancy body content (with emojis and progress bars) in any context, but the TTY-specific frame functions should only run when process.stdout.isTTY is true.
🤖 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 `@packages/cli/src/commands/status/index.ts` around lines 214 - 223, The
current logic uses flags.quest to run the Clack frame functions (p.intro,
p.note, p.outro) even when stdout isn't a TTY; split frame vs body decisions so
frames only run when process.stdout.isTTY && !flags.plain but the fancy body can
still be forced by flags.quest. Change the code around useTTY to compute
enableFrames = process.stdout.isTTY && !flags.plain and compute body =
flags.quest ? renderQuestLogTTY(log) : renderQuestLogPlain(log); then if
(enableFrames) call p.intro('CipherStash'), p.note(body, 'Quest log'),
p.outro(nextMoveHint(log, cli)); otherwise write the chosen body and the Next
line directly to stdout using process.stdout.write. Ensure you only call
p.intro/p.note/p.outro when enableFrames is true and keep nextMoveHint(cli)
usage identical.
Comment on lines
+89
to
+97
| * When DB connectivity is missing entirely (`phase === undefined`), default | ||
| * to migrate — the 5-objective shape is more informative when we don't know. | ||
| */ | ||
| export function inferQuestPath(obs: ColumnObservation): QuestPath { | ||
| if (obs.phase === undefined) return 'migrate' | ||
| if (obs.phase !== null) return 'migrate' | ||
| if (obs.physicalEncryptedTwinExists) return 'migrate' | ||
| return 'new' | ||
| } |
There was a problem hiding this comment.
Don't assume manifest-only columns are migrate flows.
When DB observability is missing, inferQuestPath() hardcodes 'migrate'. That makes stash status tell additive columns to create <col>_encrypted, deploy dual-writes, and so on, which is the wrong fallback for the manifest-only mode documented in this PR.
A safer fix is to preserve the manifest path if it's available from readManifest(), or introduce an explicit unobserved/unknown quest shape instead of emitting migrate-specific guidance by default.
🤖 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 `@packages/cli/src/commands/status/quest.ts` around lines 89 - 97, The current
inferQuestPath(ColumnObservation) returns 'migrate' when obs.phase === undefined
which incorrectly treats manifest-only columns as migrate flows; update
inferQuestPath to first check for a manifest-driven signal (e.g., a field you
populate from readManifest(), such as obs.manifestPresent or obs.manifestPath)
and return the manifest/“new”/manifest-specific quest path when a manifest
exists; if no manifest info is available, return an explicit unobserved/unknown
value (e.g., 'unknown' or 'unobserved') instead of defaulting to 'migrate' so
downstream logic (stash status, deploy guidance) does not assume migrate
behavior. Ensure you reference and use the same ColumnObservation property that
readManifest() sets and adjust callers that expect only 'migrate'/'new' to
handle the new unknown/manifest value returned by inferQuestPath.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/stash-cli/SKILL.md`:
- Line 63: Replace the awkward phrase "default-no confirm" in the sentence
explaining the --complete-rollout flag with a clearer wording such as
"confirmation prompt (default: no)"; update the sentence that currently reads
"The flag prints a default-no confirm with a loud warning before generating" to
"The flag displays a confirmation prompt (default: no) with a loud warning
before generating" so readers immediately understand the behavior.
In `@skills/stash-encryption/SKILL.md`:
- Around line 594-600: The fenced diagram block in SKILL.md lacks a language
hint which triggers markdownlint MD040; update the triple-backtick fence to
include a language (e.g., add "text" after the opening ```), so the block
becomes ```text ... ```, ensuring the diagram block is lint-clean without
changing its contents.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cdea9f8-23bf-475a-8131-9636ebff3134
📒 Files selected for processing (19)
packages/cli/src/commands/encrypt/lib/db-readers.tspackages/cli/src/commands/encrypt/status.tspackages/cli/src/commands/impl/index.tspackages/cli/src/commands/impl/steps/handoff-wizard.tspackages/cli/src/commands/init/lib/__tests__/parse-plan.test.tspackages/cli/src/commands/init/lib/parse-plan.tspackages/cli/src/commands/init/lib/rollout-state.tspackages/cli/src/commands/init/lib/write-context.tspackages/cli/src/commands/plan/index.tspackages/cli/src/commands/status/__tests__/status.test.tspackages/cli/src/commands/status/index.tspackages/cli/src/commands/status/quest.tspackages/cli/src/commands/status/render.tsskills/stash-cli/SKILL.mdskills/stash-drizzle/SKILL.mdskills/stash-dynamodb/SKILL.mdskills/stash-encryption/SKILL.mdskills/stash-secrets/SKILL.mdskills/stash-supabase/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/commands/impl/steps/handoff-wizard.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- skills/stash-dynamodb/SKILL.md
- packages/cli/src/commands/init/lib/write-context.ts
- packages/cli/src/commands/plan/index.ts
- packages/cli/src/commands/init/lib/tests/parse-plan.test.ts
- packages/cli/src/commands/init/lib/parse-plan.ts
- packages/cli/src/commands/status/quest.ts
- packages/cli/src/commands/status/tests/status.test.ts
- packages/cli/src/commands/impl/index.ts
|
|
||
| The split is invisible to the user — they just keep running `stash plan` and `stash impl`; the CLI knows where they are. | ||
|
|
||
| For users without a deployed application to gate on (local dev, sandboxes, freshly-seeded test DBs), `stash plan --complete-rollout` produces a single end-to-end plan with no deploy gate. The flag prints a default-no confirm with a loud warning before generating; only safe when no production app writes to this database. |
There was a problem hiding this comment.
Clarify the phrase “default-no confirm” for readability.
Line 63 reads awkwardly; “confirmation prompt (default: no)” is clearer and avoids ambiguity for users skimming runbook text.
Proposed wording tweak
-For users without a deployed application to gate on (local dev, sandboxes, freshly-seeded test DBs), `stash plan --complete-rollout` produces a single end-to-end plan with no deploy gate. The flag prints a default-no confirm with a loud warning before generating; only safe when no production app writes to this database.
+For users without a deployed application to gate on (local dev, sandboxes, freshly-seeded test DBs), `stash plan --complete-rollout` produces a single end-to-end plan with no deploy gate. The flag shows a confirmation prompt (default: no) with a loud warning before generating; only safe when no production app writes to this database.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| For users without a deployed application to gate on (local dev, sandboxes, freshly-seeded test DBs), `stash plan --complete-rollout` produces a single end-to-end plan with no deploy gate. The flag prints a default-no confirm with a loud warning before generating; only safe when no production app writes to this database. | |
| For users without a deployed application to gate on (local dev, sandboxes, freshly-seeded test DBs), `stash plan --complete-rollout` produces a single end-to-end plan with no deploy gate. The flag shows a confirmation prompt (default: no) with a loud warning before generating; only safe when no production app writes to this database. |
🧰 Tools
🪛 LanguageTool
[grammar] ~63-~63: Use a hyphen to join words.
Context: ...eploy gate. The flag prints a default-no confirm with a loud warning before gener...
(QB_NEW_EN_HYPHEN)
🤖 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 `@skills/stash-cli/SKILL.md` at line 63, Replace the awkward phrase "default-no
confirm" in the sentence explaining the --complete-rollout flag with a clearer
wording such as "confirmation prompt (default: no)"; update the sentence that
currently reads "The flag prints a default-no confirm with a loud warning before
generating" to "The flag displays a confirmation prompt (default: no) with a
loud warning before generating" so readers immediately understand the behavior.
Comment on lines
594
to
600
| ``` | ||
| schema-added → dual-writing → backfilling → backfilled → cut-over → dropped | ||
| ENCRYPTION ROLLOUT → ⛔ deploy gate → ENCRYPTION CUTOVER | ||
| ───────────────────── ────────────────────── | ||
| schema-add backfill historical rows | ||
| dual-write code switch reads to encrypted | ||
| db push (writes pending) drop plaintext column | ||
| ``` |
There was a problem hiding this comment.
Add a language hint to the fenced block to satisfy markdownlint (MD040).
The diagram block should declare a language (e.g., text) to keep docs lint-clean.
Proposed markdown fix
-```
+```text
ENCRYPTION ROLLOUT → ⛔ deploy gate → ENCRYPTION CUTOVER
───────────────────── ──────────────────────
schema-add backfill historical rows
dual-write code switch reads to encrypted
db push (writes pending) drop plaintext column</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 594-594: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@skills/stash-encryption/SKILL.md` around lines 594 - 600, The fenced diagram
block in SKILL.md lacks a language hint which triggers markdownlint MD040;
update the triple-backtick fence to include a language (e.g., add "text" after
the opening ```), so the block becomes ```text ... ```, ensuring the diagram
block is lint-clean without changing its contents.
auxesis
approved these changes
May 8, 2026
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
2 participants
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 CodeRabbit
New Features
Tests
Documentation