fix: raise on ambiguous resource ID collision across sibling nested stacks

[Adityaj0]

Fixes #9185.

Which issue(s) does this change fix?

#9185

Scope note (added after review)

get_resource_by_id() and get_resource_full_path_by_id() are also used outside sam sync
--- package_context.py, guided_context.py, and image_repository_validation.py all call
get_resource_full_path_by_id() to map a user-supplied --image-repositories function ID to a
repository URI. The new AmbiguousResourceIdentifier therefore now also reaches
sam package/sam deploy --guided/image-repository validation for a template with two sibling
nested stacks sharing a logical ID that a user references there, not just the sam sync --resource-id path this PR was originally scoped around. This is intentional (see the extended
docstring on get_resource_full_path_by_id): mapping an image repository to the wrong function is
the same "operates on the wrong resource" hazard motivating this whole change, and unlike sam sync's interactive usage there is no further confirmation step before it's baked into a deploy.
Flagging explicitly since it widens this PR's user-facing blast radius beyond the issue it was
filed against.

Why is this change necessary?

get_resource_by_id() (samcli/lib/providers/provider.py) resolves a bare, unqualified resource logical ID (e.g. what `sam sync --resource-id Function1` and the underlying file-watch/code-trigger machinery for `sam sync --watch` use) by searching every stack and returning the first match, with no check for ambiguity:

for stack in stacks:
    if stack.stack_path == identifier.stack_path or search_all_stacks:
        ...
        if found_resource:
            return cast(Dict[str, Any], found_resource)   # first match wins, silently

If two different nested stacks both contain a resource with the same logical ID, and there's no root-stack resource with that ID to prefer, this silently returns whichever stack happens to come first in the internal stack list — no error, no warning. Since `sam sync` bypasses full CloudFormation deployment and pushes local code changes directly to the resolved physical resource, an ambiguous `--resource-id` can silently push code to the wrong live, deployed Lambda function (or other resource).

What does this change do?

Changes `get_resource_by_id` to detect this specific case and raise a new `AmbiguousResourceIdentifier` (a `UserException`, matching the existing `ResourceNotFound` etc. in `samcli/commands/local/cli_common/user_exceptions.py`) with a message telling the user how to disambiguate using the existing `Stack/ResourceId` identifier syntax that `ResourceIdentifier` already parses.

Importantly, this preserves the existing, intentional, and already-tested precedence rule that a root-stack resource always wins over a same-named nested-stack resource for a bare ID (see `TestGetResourceByID::test_get_resource_by_id_implicit_root`, which deliberately constructs a root-vs-nested collision and asserts root wins — that test still passes unchanged). The new error only fires for the previously-unhandled case: no root match, and two or more nested stacks collide.

Description of how you validated changes

Added `TestGetResourceByIDAmbiguousNestedStacks` to `tests/unit/commands/local/lib/test_provider.py`:

• raises `AmbiguousResourceIdentifier` when two sibling nested stacks both have a matching resource and there's no root match
• does not raise when only one nested stack matches
• does not raise when the caller explicitly qualifies the ID with a stack path

All 118 tests in `tests/unit/commands/local/lib/test_provider.py` pass (115 existing + 3 new), including all pre-existing root/nested precedence tests, unchanged. Also ran the full `tests/unit/lib/sync/`, `tests/unit/lib/utils/test_resource_trigger.py`, `tests/unit/lib/utils/test_code_trigger_factory.py`, `tests/unit/lib/utils/test_resource_type_based_factory.py`, and `tests/unit/lib/providers/` suites (374 passed, 4 skipped, all pre-existing skips) — no regressions in any of `get_resource_by_id`'s call sites. `black --check` clean.

Checklist

• I have read the CONTRIBUTING doc
• Local run of `brazil-build test`/`pytest` passes for the affected tests (ran via a local venv + pytest directly)
• Added unit tests covering the new behavior
• Added integration tests (not applicable — internal resolver bug, fully covered by unit tests; the failure mode requires two nested stacks with a colliding logical ID, which is exercised by the new unit test fixtures)
• Did not modify or add to generated files

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

[aws-sam-tooling-bot]

Code Review Results

Reviewed: e1f4bf6..5ed83e7
Files: 3
Comments: 2

---

Comments on lines outside the diff:

[samcli/lib/providers/provider.py:930] [GENERAL] getresource_full_path_by_id resolves the same identifier syntax but was left with the old first-match-wins logic, so after this change the two functions can disagree on what a bare ID means:

• No root-stack preference — it iterates stacks in list order and returns the first hit, so a bare ID with both a root and a nested match resolves to the root resource in get_resource_by_id and possibly to the nested one here.
• The sibling-nested-stack collision this PR promotes to a hard error is still resolved silently to whichever stack comes first.

That second point matters for user-supplied bare IDs on the packaging path: package_context.py:140, guided_context.py:358, and image_repository_validation.py:138 all map --image-repositories Function1=uri keys to full paths through this function, so an ambiguous key can be attached to the wrong nested resource's full path (wrong ECR repo mapping, or a spurious "not all image functions provided" validation failure) — the same failure mode the PR fixes for sam sync --resource-id.

Worth factoring the resolution rule (root preference + ambiguity detection) into a shared helper used by both functions so the two entry points cannot drift.

[aws-sam-tooling-bot]

[BUG] The remediation path in the error message is built from identifier.resource_iac_id, but a match can be found via either the normalized resource ID or the raw logical ID:

resource_id = ResourceMetadataNormalizer.get_resource_id(resource, logical_id)
if resource_id == identifier.resource_iac_id or (
   not identifier.stack_path and logical_id == identifier.resource_iac_id
):

The second branch is only reachable when identifier.stack_path is empty — which is exactly the ambiguous case this code raises on. So if the user passed a raw logical ID that differs from the normalized ID (CDK apps, or any template with SamResourceId/aws:cdk:path metadata — see ResourceMetadataNormalizer.get_resource_id), the message tells them to retry with e.g. NestedStackA/Function1ABC123. That qualified form can never resolve, because once stack_path is non-empty only resource_id == identifier.resource_iac_id is compared — the retry returns None and surfaces as "resource not found". The user is sent to a dead end.

Capture the matched resource ID and build the paths from it, the way get_resource_full_path_by_id already does (it returns get_full_path(stack.stack_path, resource_id)). This also makes the listed paths match the canonical full paths used by get_all_resource_ids:

matches: List[Tuple[str, Dict[str, Any]]] = []  # (full_path, resource)
for stack in stacks:
   if stack.stack_path == identifier.stack_path or search_all_stacks:
       found_resource = None
       found_resource_id = None
       for logical_id, resource in stack.resources.items():
           resource_id = ResourceMetadataNormalizer.get_resource_id(resource, logical_id)
           if resource_id == identifier.resource_iac_id or (
               not identifier.stack_path and logical_id == identifier.resource_iac_id
           ):
               found_resource = resource
               found_resource_id = resource_id
               break

       if found_resource:
           if not stack.stack_path:
               return cast(Dict[str, Any], found_resource)
           matches.append((get_full_path(stack.stack_path, cast(str, found_resource_id)), found_resource))
           if not search_all_stacks:
               break

if len(matches) > 1:
   colliding_paths = [full_path for full_path,  in matches]
   ...

[Adityaj0]

Thanks for the review — verified this independently and it's correct.

I traced get_resource_full_path_by_id (samcli/lib/providers/provider.py:930, pre-fix at develop) line by line: it iterates stacks and returns on the first match (return get_full_path(stack.stack_path, resource_id) inside the inner loop), with no root-stack preference and no ambiguity check — exactly the same bug class this PR fixes in get_resource_by_id, just unaddressed here. And you're right about the blast radius: package_context.py:140, guided_context.py:358, and image_repository_validation.py:138 all resolve user-supplied --image-repositories Function1=uri keys through this function, so a colliding bare ID could silently attach the wrong nested resource's full path (wrong ECR mapping, or a spurious "not all image functions provided" failure) on the packaging path.

Pushed 1ac9443: applied the same root-stack-priority + nested-stack-collision rule to get_resource_full_path_by_id (raises the existing AmbiguousResourceIdentifier on nested/nested collision, root still wins unconditionally), and added TestGetResourceFullPathByIDAmbiguousNestedStacks in tests/unit/commands/local/lib/test_provider.py mirroring the existing TestGetResourceByIDAmbiguousNestedStacks (collision raises, single nested match doesn't, explicit stack-path qualification doesn't, root-priority-over-nested holds even when root appears last in the stack list).

Verified: full tests/unit/commands/local/lib/test_provider.py (122 passed), plus tests/unit/lib/cli_validation/, tests/unit/commands/deploy/test_guided_context.py, tests/unit/commands/package/, tests/unit/lib/providers/, tests/unit/lib/sync/, and the resource-trigger/code-trigger-factory/resource-type-based-factory suites (670 passed, 4 pre-existing skips, no regressions). black --check clean on both changed files.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: e1f4bf6..1ac9443
Files: 3
Comments: 1

[aws-sam-tooling-bot]

[BUG] The remediation path in the ambiguity error is built from identifier.resource_iac_id, but a match can be established through either the normalized resource ID or the raw logical ID:

resource_id = ResourceMetadataNormalizer.get_resource_id(resource, logical_id)
if resource_id == identifier.resource_iac_id or (
   not identifier.stack_path and logical_id == identifier.resource_iac_id
):

When the match comes from the second branch (CDK / SamResourceId / aws:cdk:path resources, where get_resource_id() returns something other than the logical ID), the suggested path is Stack/<logicalId> — which cannot resolve. Once a stack path is present, the logical-ID fallback is disabled by the not identifier.stack_path guard, so a stack-qualified lookup only matches the normalized ID. The existing suite already pins this: tests/unit/commands/local/lib/test_provider.py asserts ResourceIdentifier("childStack/CDKResourceInChild1") resolves to None, while the bare CDKResourceInChild1 resolves to childStack/CDKResourceInChild1-x.

Concrete effect: a user with two sibling nested stacks each holding a CDK resource with logical ID Function1 gets told to run --resource-id NestedStackA/Function1, which then fails with a not-found error, leaving no working way to disambiguate. Note that get_resource_full_path_by_id (same PR, line 969) builds its message from the normalized resource_id, so the two error messages disagree for identical input.

Carry the matched ID alongside the resource so the message names an identifier that actually resolves:

matches: List[Tuple[Stack, str, Dict[str, Any]]] = []
for stack in stacks:
   if stack.stack_path == identifier.stack_path or search_all_stacks:
       found_resource = None
       found_resource_id = None
       for logical_id, resource in stack.resources.items():
           resource_id = ResourceMetadataNormalizer.get_resource_id(resource, logical_id)
           if resource_id == identifier.resource_iac_id or (
               not identifier.stack_path and logical_id == identifier.resource_iac_id
           ):
               found_resource = resource
               found_resource_id = resource_id
               break
       ...
       matches.append((stack, cast(str, found_resource_id), found_resource))

if len(matches) > 1:
   colliding_paths = [get_full_path(stack.stack_path, resource_id) for stack, resource_id,  in matches]

A regression test covering a resource carrying Metadata.SamResourceId in two sibling nested stacks would lock this in — the new test classes only use plain CFN resources where logical ID and normalized ID coincide, which is why the bug is invisible to them.

[Adityaj0]

Good catch, and confirmed real — for a bare logical ID that only matches via the logical-ID fallback (a CDK resource with SamResourceId/aws:cdk:path metadata, where the normalized ID differs from the logical ID), the suggested qualified retry path was built from the raw identifier.resource_iac_id and could never actually resolve, since a stack-qualified lookup only matches on the normalized ID.

Pushed a fix (4dce905) that tracks the matched (normalized) resource_id alongside each candidate in matches, and builds colliding_paths from that instead — matching get_resource_full_path_by_id's existing (correct) approach, so the two entry points now agree on identical input.

Added test_ambiguity_error_suggests_a_path_that_actually_resolves_for_cdk_resources, using the exact scenario you described: two sibling nested stacks each with a Function1 logical ID carrying SamResourceId: Function1-x metadata. It asserts the error message names NestedStackA/Function1-x (not NestedStackA/Function1), and — going one step further — that feeding the suggested qualified path back into get_resource_by_id actually resolves to the right resource, so the regression can't silently reappear if the message format changes again. Confirmed it fails on the pre-fix code (suggests NestedStackA/Function1, a dead end) and passes with the fix.

Full tests/unit/commands/local/lib/test_provider.py (123 tests) plus the broader dependent suites (517 tests) pass, ruff format clean.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: e1f4bf6..4dce905
Files: 3
Comments: 2

[aws-sam-tooling-bot]

[GENERAL] Raising from get_resource_full_path_by_id extends this change's blast radius well past sam sync, and turns some previously-working invocations into hard failures. The three callers of this function all resolve a user-supplied bare ID and are written to tolerate an unresolved key:

# samcli/commands/package/package_context.py:140
# samcli/commands/deploy/guided_context.py:358
repo_full_path = get_resource_full_path_by_id(stacks, ResourceIdentifier(image_repo_func_id))
if repo_full_path:
   updated_repo[repo_full_path] = image_repo_uri

samcli/lib/cli_validation/image_repository_validation.py:138 is similar — it collects results into a set and compares against the image-function full paths, expecting a clean click.BadOptionUsage when the sets differ.

So for a template with two sibling nested stacks each containing Function1, sam deploy --image-repositories Function1=<uri> and sam package previously resolved to the first match and continued; they now abort with AmbiguousResourceIdentifier. For sam deploy --guided the abort lands mid-flow, after the interactive prompts have already been answered.

Erroring here is defensible (mapping an image repo to the wrong function is the same class of hazard as syncing the wrong function), but it is a user-facing behavior change for deploy/package, not just for the sync --resource-id path the PR describes. Worth either scoping the hard error to the sync resolution path or calling the deploy/package impact out explicitly in the PR description/changelog so it isn't discovered as a regression.

[aws-sam-tooling-bot]

[GENERAL] The same "bare ID matches resources in more than one stack" condition is already handled elsewhere in the codebase with the opposite strategy — warn and pick deterministically (samcli/lib/providers/sam_function_provider.py:139-160):

# If multiple functions are found, only return one of them
if len(found_fs) > 1:
   found_fs.sort(key=lambda f0: f0.full_path.lower())
   message = (
       f"Multiple functions found with keyword {name}! Function {found_fs[0].full_path} will be "
       f"invoked! If it's not the function you are going to invoke, please choose one of them from below:"
   )
   LOG.warning(Colored().yellow(message))
   resolved_function = found_fs[0]

After this change the two resolvers disagree on the same user input: sam sync --resource-id Function1 hard-fails, while sam local invoke Function1 still silently picks the alphabetically-first full path with a warning. That matters for two reasons: the "operates on the wrong resource" hazard motivating this PR is only closed on one of the two resolution paths, and future maintainers now have two contradictory precedence rules to reason about (first-nested-match is an error here; lowest-sorted-full-path wins there).

Either align the two (reuse the warn-and-pick behavior, or extend the ambiguity error to SamFunctionProvider.get), or add a short comment next to this raise explaining why sync-style resolution must be stricter than invoke-style resolution, so the divergence is a documented decision rather than an accident.

[Adityaj0]

Both fair, and I confirmed the caller behavior claim directly — `package_context.py`, `guided_context.py`, and `image_repository_validation.py` all treat a falsy/graceful return as the norm, so yes, this raise now reaches package/deploy/validate-image-repositories, not just `sam sync --resource-id`.

On reflection I don't think this should be scoped back or made non-fatal for those callers, though — an ambiguous match in `--image-repositories Function1=` means the wrong function could silently get the wrong image repository mapped to it, which is exactly the same "operates on the wrong physical resource" class of hazard the whole PR exists to close for sync. Silently picking one is arguably worse there than for sync, since there's no interactive confirmation step before it's baked into the deploy. So: erroring is the right call, but you're right it needed to be said explicitly rather than left implicit.

On the `SamFunctionProvider.get()` divergence: agreed that's a real inconsistency worth documenting rather than leaving as an surprise for the next person touching either function. I'm inclined to leave the two resolution strategies as-is rather than unify them in this PR — `sam local invoke`/`SamFunctionProvider.get()` resolves a function for one-off interactive/dev use where a warning-and-continue is recoverable, while this function's callers all feed the result into a deploy/package/validate decision with no further human-in-the-loop step. Unifying them would be a separate, larger behavioral change (and risks its own regressions) outside what this PR set out to fix.

Pushed 383d8f0: expanded the `AmbiguousResourceIdentifier` docstring section to state both of these explicitly — the broadened blast radius beyond sync, and why it's intentionally stricter than `SamFunctionProvider.get()` — so this reads as a documented decision rather than something the next reader has to rediscover. No behavior change, test_provider.py (123 tests) still green.

I'll also flag this in the PR description for anyone reviewing the merge, since "package/deploy can now hard-fail on ambiguous image-repository IDs" is worth surfacing at that level too, not just in a docstring.