chore(golden): tighten test assertions, drop dead code, document known limitations

Cleanups surfaced by the code-quality review of PR 1:

- harness.run_package_pipeline: drop the dead first
  _export_global_artifacts_pass(original) call. The original template was
  read, mutated by that pass, and then never referenced again — the
  artifact rewrite operates on a deep-copied build_output, so the first
  pass had no observable effect. Update the docstring to match.
- harness._walk_artifact_properties: replace the bare `-> List:` return
  type with `-> List[Tuple[str, str, Dict[str, Any]]]` so downstream
  callers and reviewers can see the shape without reading the body.
- harness module docstring: add a Known limitations subsection covering
  four issues that the next contributor should expect (image / ECR
  over-rewrite, _stub_local_uris_for_translator drift, GoldenS3Uploader
  not subclassing S3Uploader, missing AWS::Include sentinel). They will
  be addressed by PR 2 / corpus growth.
- test_harness_package.test_package_sam_case_rewrites_code_to_s3_uri:
  replace the lenient `s3_uri_field == "golden-bucket"` short-circuit
  with two specific assertions that the dict has the expected keys.
- test_harness_build: drop unused `tmp_path` fixture parameters from
  two tests.
- update_goldens.py / check_semver_bump.py: silence mypy `[unreachable]`
  on the script-form sys.path bootstrap added in the previous fix.

Author: bnusunny

tests/golden/check_semver_bump.py

@@ -12,10 +12,12 @@
 # When run as a script, Python does not auto-add the repo root to sys.path,
 # so absolute imports rooted at `tests.golden...` would fail. This script
 # happens to have no such imports today, but keep the guard symmetrical with
-# update_goldens.py so future imports do not silently regress the script form.
+# update_goldens.py so future imports do not silently regress the script
+# form. mypy treats the __main__ guard as always-False, hence the
+# `type: ignore`.
 if __name__ == "__main__" and __package__ is None:
-    import sys
-    from pathlib import Path
+    import sys  # type: ignore[unreachable]  # noqa: E402
+    from pathlib import Path  # noqa: E402
 
     sys.path.insert(0, str(Path(__file__).resolve().parents[2]))
 

tests/golden/harness.py

@@ -36,14 +36,38 @@
   matches the user-visible behavior of ``sam build`` on such a template
   (the LE construct is preserved verbatim for CloudFormation server-side
   expansion).
+
+Known limitations (deferred to follow-up PRs / corpus growth):
+
+- Image-based Lambdas / ECR artifacts: ``_packageable_replacement`` always
+  rewrites ``AWS::Lambda::Function`` ``Code`` to an S3Bucket/S3Key dict and
+  ``Code.ImageUri`` to an ECR URI string, but it does not enforce that
+  exactly one of those properties is set on a given resource. A template
+  with both shapes will be over-rewritten. No image sentinel exists yet to
+  surface this — PR 2's corpus expansion will add one.
+- ``_stub_local_uris_for_translator`` duplicates the rewrite logic in
+  ``SamTemplateValidator._replace_local_codeuri``. The two will drift if
+  upstream extends the validator's set of properties or recognized URI
+  shapes. Until the harness can call the validator without pulling in
+  AWS-credential side-effects (boto Session at construction time), this
+  duplication is accepted.
+- ``GoldenS3Uploader`` reimplements the surface ``S3Uploader`` exposes
+  rather than subclassing it, to keep the harness off the boto Session
+  path. If the package code starts using S3Uploader methods we have not
+  stubbed, the next failure will say so. Subclassing is the cleaner long
+  term fix.
+- No ``AWS::Include`` sentinel: the ``_export_global_artifacts_pass`` call
+  in ``run_package_pipeline`` is exercised only indirectly. A corpus case
+  with ``AWS::Include`` should be added so that pass has explicit
+  coverage; deferred to follow-up.
 """
 
 from __future__ import annotations
 
 import copy
 import hashlib
 from pathlib import Path
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, List, Optional, Tuple
 
 import yaml
 
@@ -64,9 +88,11 @@ def _load_template(template_path: Path) -> Dict[str, Any]:
         return yaml.safe_load(f) or {}
 
 
-def _walk_artifact_properties(template: Dict[str, Any]) -> List:
+def _walk_artifact_properties(
+    template: Dict[str, Any],
+) -> List[Tuple[str, str, Dict[str, Any]]]:
     """Collect (resource_id, property_path, resource_dict) for every packageable artifact."""
-    results = []
+    results: List[Tuple[str, str, Dict[str, Any]]] = []
     resources = template.get("Resources", {}) or {}
     for resource_id, resource in resources.items():
         if not isinstance(resource, dict):
@@ -312,28 +338,22 @@ def run_package_pipeline(
 ) -> Dict[str, Any]:
     """In-process equivalent of `sam package` for one template + build output.
 
-    1. Run _export_global_artifacts_pass on the original template (the pre-LE
-       AWS::Include pass added in PR #9030).
-    2. Walk packageable artifact properties on the build output and replace
+    1. Walk packageable artifact properties on the build output and replace
        each local path with an s3://golden-bucket/<sha256> URI.
-    3. Re-run _export_global_artifacts_pass on the result to handle any
-       AWS::Include nested in the LE-expanded template.
-    4. Return the final dict.
+    2. Run _export_global_artifacts_pass on the rewritten template to
+       handle any AWS::Include structural nodes.
+    3. Return the final dict.
     """
     from samcli.lib.package.artifact_exporter import _export_global_artifacts_pass
 
     template_dir = str(template_path.parent)
     uploader = GoldenS3Uploader(template_dir)
 
-    # Pre-LE AWS::Include pass on the original template's structural form.
-    original = _load_template(template_path)
-    _export_global_artifacts_pass(original, uploader, template_dir)
-
     # Walk build_output's artifact properties and rewrite local paths.
     pkg = copy.deepcopy(build_output)
     _rewrite_artifacts_to_s3(pkg, uploader, template_dir)
 
-    # Post-expansion AWS::Include pass to catch any remaining structural nodes.
+    # AWS::Include pass on the rewritten template.
     _export_global_artifacts_pass(pkg, uploader, template_dir)
     return pkg
 
@@ -353,6 +373,9 @@ def _rewrite_artifacts_to_s3(template: Dict[str, Any], uploader, template_dir: s
         if not isinstance(current, str):
             continue
         rtype = resource.get("Type")
+        if not isinstance(rtype, str):
+            # _walk_artifact_properties already filters, but re-narrow for mypy.
+            continue
         replacement = _packageable_replacement(
             rtype, prop_path, current, template_dir, resource_id
         )

tests/golden/test_harness_build.py

@@ -7,7 +7,7 @@
 CASES_ROOT = Path(__file__).parent / "templates"
 
 
-def test_build_sam_case_replaces_codeuri_with_placeholder(tmp_path):
+def test_build_sam_case_replaces_codeuri_with_placeholder():
     case = CASES_ROOT / "sam_resources" / "serverless_function_zip"
     result = run_build_pipeline(case / "template.yaml", language_extensions=False)
     func = result["Resources"]["HelloFunction"]
@@ -16,7 +16,7 @@ def test_build_sam_case_replaces_codeuri_with_placeholder(tmp_path):
     assert func["Properties"]["Code"] == "<<BUILT_ARTIFACT>>"
 
 
-def test_build_le_case_expands_foreach(tmp_path):
+def test_build_le_case_expands_foreach():
     case = CASES_ROOT / "language_extensions" / "foreach_static_zip"
     result = run_build_pipeline(case / "template.yaml", language_extensions=True)
     assert "AlphaFunction" in result["Resources"]

tests/golden/test_harness_package.py

@@ -13,8 +13,8 @@ def test_package_sam_case_rewrites_code_to_s3_uri():
     pkg_out = run_package_pipeline(case / "template.yaml", build_out)
     code = pkg_out["Resources"]["HelloFunction"]["Properties"]["Code"]
     assert isinstance(code, dict)  # CFN Lambda Code becomes S3Bucket/S3Key dict
-    s3_uri_field = code.get("S3Bucket") or code.get("ImageUri") or ""
-    assert s3_uri_field == "golden-bucket"
+    assert code["S3Bucket"] == "golden-bucket"
+    assert "S3Key" in code
 
 
 def test_package_le_case_rewrites_each_expanded_codeuri():

tests/golden/update_goldens.py

@@ -13,10 +13,11 @@
 # Allow direct script invocation (`python tests/golden/update_goldens.py`)
 # in addition to module form (`python -m tests.golden.update_goldens`).
 # When run as a script, Python does not auto-add the repo root to sys.path,
-# so the absolute `from tests.golden...` imports below would fail.
+# so the absolute `from tests.golden...` imports below would fail. mypy
+# treats the __main__ guard as always-False, hence the `type: ignore`.
 if __name__ == "__main__" and __package__ is None:
-    import sys
-    from pathlib import Path
+    import sys  # type: ignore[unreachable]  # noqa: E402
+    from pathlib import Path  # noqa: E402
 
     sys.path.insert(0, str(Path(__file__).resolve().parents[2]))