fix: persist guided deploy args to samconfig.toml on failure

[ljacobsson]

Which issue(s) does this change fix?

Fixes #3693

Why is this change necessary?

When running sam deploy --guided, the answers entered during the guided prompts are only written to samconfig.toml after the credential-requiring AWS calls (manage_stack / sync_ecr_stack) succeed. If those calls fail (e.g. invalid or expired SSO credentials), the command aborts before saving, so the user has to re-enter every prompt after fixing their credentials. This is especially painful for stacks with many parameters (see #3693).

How does it address the issue?

All config-relevant answers are now collected on the instance before any AWS call is made, and the config is saved even when the guided flow fails.

To avoid clobbering a known-good configuration during development (the concern raised in the issue discussion), save-on-failure is gated:

• No existing samconfig.toml (new project): answers are always saved on failure, since there is nothing to overwrite.
• Existing samconfig.toml: the file is left untouched on failure by default. Users can opt in to overwriting it with the new --save-params-on-failure/--no-save-params-on-failure flag.

Successful deploys continue to save the config exactly as before.

What side effects does this change have?

• A new --save-params-on-failure/--no-save-params-on-failure flag on sam deploy (default off), only relevant to --guided.
• On a failed guided deploy of a brand-new project, a samconfig.toml will now be written where previously none was. The original error is still surfaced.
• schema/samcli.json regenerated for the new option.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Review the generative AI contribution guidelines
• Add input/output type hints to new functions/methods
• [n/a] Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• [n/a] Write/update integration tests
• [n/a] Write/update functional tests if needed
• make pr passes
• [n/a] make update-reproducible-reqs if dependencies were changed
• [] Write documentation
  Documentation: the new --save-params-on-failure flag is documented via its --help text and the regenerated schema/samcli.json.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[roger-zhangg]

Thanks for pushing on this, @ljacobsson — and apologies for the long silence. I reviewed the branch at f5fa2d283 in a local worktree and probed the runtime behaviour rather than only reading the diff. Summary: the direction is right and has maintainer precedent, but there are a few things to fix before this can go in, plus one behaviour-scope question that a maintainer needs to rule on.

For the record, the gating design here matches what @qingchm proposed in #3693 (2022-12-19): keep the default behaviour and add an opt-in flag. So the shape of the change isn't the problem.

---

1. CI is red — must fix

make pr fails on all 6 platform jobs, and I reproduced both failures locally:

FAILED tests/unit/commands/samconfig/test_samconfig.py::TestSamConfigForAllCommands::test_deploy
FAILED tests/unit/commands/samconfig/test_samconfig.py::TestSamConfigForAllCommands::test_deploy_different_parameter_override_format
2 failed, 1 passed, 31 deselected

Both are positional do_cli assertions that weren't updated for the new argument. save_params_on_failure lands between confirm_changeset and region, so a False, is needed:

• tests/unit/commands/samconfig/test_samconfig.py:982 — insert after the True, (confirm_changeset), before "myregion" on line 983
• tests/unit/commands/samconfig/test_samconfig.py:1099 — same, before "myregion" on line 1100

Separately, the branch is 77 commits behind develop, so schema/samcli.json will need regenerating after a rebase.

2. Correctness bug: a failing save swallows the original error

samcli/commands/deploy/guided_context.py:594-599:

try:
    self.guided_prompts(_parameter_override_keys)
except Exception:
    if self._should_save_config() and (not config_already_exists or self.force_save_config):
        self._save_config(guided_config)   # <-- if this raises, the `raise` below never runs
    raise

If _save_config raises, the original exception is replaced. Verified with a read-only config dir + a simulated credential failure:

RAISED: PermissionError [Errno 13] Permission denied: '.../samconfig.toml'
   __context__ (the ORIGINAL, now hidden): RuntimeError 'InvalidClientTokenId: The security token included in the request is invalid'

The user sees a permissions error instead of "your credentials are invalid" — the exact opposite of this PR's goal. Please wrap the save in its own try/except, log at debug/warn, and always re-raise the original.

3. Behaviour-change question for maintainers (a): the save fires on every post-answer failure, not just credential failures

The PR description and code comments frame this as "the AWS calls failed". But the except Exception at line 596 is reached for anything raised after guided_stack_name is assigned at line 192 — which includes the whole image-repository flow. I verified two non-credential cases both write samconfig.toml on a fresh project:

Failure injected after answers collected	samconfig.toml written?
click.exceptions.Abort() (i.e. Ctrl-C — it subclasses RuntimeError)	yes
GuidedDeployFailedError("Unreferenced Auto Created ECR Repos Must Be Deleted.")	yes

So Ctrl-C at "Create managed ECR repositories for all functions?", declining the unreferenced-repo cleanup, an invalid ECR URI, or "No images found to deploy, try running sam build" all now leave a config file behind where previously nothing was written. The existing test test_run_does_not_save_config_when_aborted_before_stack_name only covers an abort at the first prompt, so this isn't covered. Consider narrowing to the intended failure set (or at least excluding click.exceptions.Abort), and documenting whichever is chosen.

4. Behaviour-change question for maintainers (b): the saved config is incomplete, and the success message claims otherwise

resolve_s3 / guided_s3_bucket / guided_image_repositories are deliberately assigned after the AWS calls (guided_context.py:220-224), so they are never in the failure-path save. Because save_config filters falsy values (if v: at guided_config.py:73-78), resolve_s3=False is dropped entirely. Actual file produced by a failed guided run on a new zip-based project:

version = 0.1

[default.deploy.parameters]
stack_name = "my-stack"
s3_prefix = "my-stack"
region = "us-west-2"
confirm_changeset = true
capabilities = "CAPABILITY_IAM"
parameter_overrides = "MyPlainParam=\"...\""

[default.global.parameters]
region = "us-west-2"

No resolve_s3, no s3_bucket, no image_repositories. Re-running sam deploy --guided works fine (this is the intended recovery path), but a plain sam deploy now hits resolve_s3_callback (samcli/commands/_utils/options.py:240) and raises PackageResolveS3AndS3NotSetError. It fails loudly rather than deploying something wrong, which is the important part — but the message won't point at the incomplete config.

The bigger issue is that save_config unconditionally prints its success text (guided_config.py:89-96) before the error surfaces:

	Saved arguments to config file
	Running 'sam deploy' for future deployments will use the parameters saved above.
	...
RAISED: RuntimeError InvalidClientTokenId: The security token included in the request is invalid

That second line is false for this config. The failure path needs its own wording, e.g. "Partially saved your answers to samconfig.toml so you don't have to re-enter them; re-run sam deploy --guided after fixing the error above."

5. --save-params-on-failure merges, it does not overwrite

The help text at samcli/commands/deploy/command.py:114-120 says "overwriting an existing configuration file if one is present". SamConfig.put merges key-by-key, so stale keys survive. Starting from a realistic existing config and force-saving a failed guided run against a different region:

# before                              # after
stack_name = "prod-stack"             stack_name = "dev-experiment"
s3_bucket = "my-artifacts-us-east-1"  s3_bucket = "my-artifacts-us-east-1"   # stale, wrong region
region = "us-east-1"                  region = "eu-west-1"
capabilities = "CAPABILITY_NAMED_IAM" capabilities = "CAPABILITY_IAM"        # silently downgraded
parameter_overrides = "Env=\"prod\""  parameter_overrides = "Env=\"prod\""   # stale

The result is internally inconsistent: region = eu-west-1 alongside a us-east-1 bucket. The merge semantics are pre-existing (the success path has the same property), but this PR extends them to a partial answer set on a file that was previously left untouched. Please at minimum correct the help text to say "merged into", and consider whether the flag should also clear the guided-owned keys.

6. Smaller items

• Flag naming. --save-params already exists globally (samcli/cli/cli_config_file.py:379) and is unrelated. --save-params-on-failure reads like a modifier of it. @qingchm's original suggestion (--force-save-config) avoids the collision and matches the internal kwarg you already use (force_save_config).
• Possible stickiness footgun. save_params_on_failure is not in params_to_exclude at samcli/cli/cli_config_file.py:305-309 (unlike save_params itself). Reading that code path, sam deploy --guided --save-params-on-failure --save-params looks like it would persist save_params_on_failure = true into samconfig.toml, silently enabling force-overwrite for every future run in that project. Worth confirming and excluding.
• Telemetry / redundant read. config_exists (guided_config.py:31-37) builds a third SamConfig, so each guided run now emits 3 SamConfigFileExtension events instead of 2 (verified). It's called immediately after read_config_showcase, which already constructed one — reuse it, or just use Path(...).exists().
• Comment accuracy. The comment at guided_context.py:186-189 says "All config-relevant answers are set on the instance before the AWS calls are made". Three of them aren't (see §4); worth saying so explicitly.

7. Overlap with #9183 — none

I checked, since both touch config handling. There is no conflict:

• File sets are disjoint. fix: SamConfig.get_all() mutates shared document, leaking one command's params into another's #9183 touches only samcli/lib/config/samconfig.py + tests/unit/lib/samconfig/test_samconfig.py; this PR touches neither.
• fix: SamConfig.get_all() mutates shared document, leaking one command's params into another's #9183 fixes the shared-document mutation in SamConfig.get_all(). This PR never calls get_all() — config_exists only calls SamConfig.exists(), which is a bare filepath.exists() (samconfig.py:164-165), and GuidedConfig.get_config_ctx constructs a fresh SamConfig per call, so no instance is shared.

Merge order doesn't matter. The only tangential note: the file this PR writes on failure includes a [global.parameters] region entry, which is the trigger condition for the #9183 leak — but guided already writes that on success, so exposure isn't meaningfully increased.

8. Test results

Run locally against f5fa2d283:

• python -m pytest tests/unit/commands/deploy -q → 84 passed, 22 subtests passed. The 4 new test_guided_context.py tests pass.
• python -m pytest tests/unit/lib/samconfig -q → passed.
• python -m pytest tests/unit/commands/samconfig/test_samconfig.py -k test_deploy → 2 failed (see §1).

On coverage of the failure path (question (d)): the 4 new tests do cover the three gating branches plus early-abort, which is good. What's missing is (i) a test asserting the contents of what gets saved on failure — specifically that resolve_s3/s3_bucket/image_repositories are absent (§4), and (ii) a test that the original exception survives a failing _save_config (§2).

---

Suggested path forward: rebase on develop, fix the two test_samconfig.py assertions and regenerate the schema, fix the error-masking bug in §2, and give the failure path its own message instead of reusing the success text. Then a maintainer should explicitly sign off on §3 (which failures should trigger a save) and §5 (merge-vs-overwrite wording), since those are the user-visible behaviour decisions and not something a contributor should have to guess at.