Do not require os in recipe manifest Platform blocks (#974)

[allanli4]

Authored by an AI agent running an issue-resolution pipeline. Human review is
required before merge; this PR is not self-approved.

Issue

#974 —
"ggdeploymentd: Do not require os and architecture fields in component recipes"
Reported by: @aws-kevinrickard

Problem

The AWS component recipe reference documents os and architecture as optional
attributes of a manifest Platform block, but nucleus lite behaves as if os is
required. A manifest such as:

Manifests:
  - Platform:
      runtime: "*"

is silently treated as incompatible with the current platform. Expected
behaviour, per the reporter, is to match the non-lite nucleus and impose no
requirement when the fields are absent.

Scope

The error quoted in the issue —
doesn't claim platform {runtime=aws_nucleus_lite, os=linux, architecture=amd64} compatibility
— is emitted by cloud-side ResolveComponentCandidates, which is not in this
repository. This PR fixes the local platform-matching path, where the same
documented contract is violated independently and observably: a recipe omitting
os currently fails artifact resolution, systemd unit generation, and lifecycle
execution on-device. The platform attributes nucleus lite sends to the cloud
(modules/ggdeploymentd/src/deployment_handler.c:1152-1157) are unchanged — they
already populate all three fields.

Acceptance criteria

All criteria are DERIVED — the issue states none explicitly.

#	Criterion	Met	How
1	Platform with a lite-compatible runtime but no os is compatible on Linux	Yes	Fix at recipe.c:399-411; test select_manifest_platform_without_os
2	Platform with no architecture imposes no architecture requirement (no regression)	Yes	architecture.len == 0 branch preserved byte-identical; test select_manifest_platform_without_architecture
3	Platform: { runtime: "*" } (both omitted — the issue's exact input) is selected on Linux	Yes	Test select_manifest_platform_without_os_or_architecture — asserts GG_ERR_OK and deep map equality of the selected manifest
4	Existing negatives preserved: no runtime; non-lite runtime; explicit non-matching os; explicit non-matching architecture — all still skipped	Yes	Four tests: without_runtime, with_other_runtime, with_other_os, with_other_architecture
5	Non-string os / architecture still yields GG_ERR_INVALID	Yes	Tests with_invalid_os, with_invalid_architecture. See "Behaviour changes" — for architecture this is a strictness increase
6	Covered by an automated test in the existing framework	Yes	Inline GG_TEST_DEFINE in the module's own #ifdef GG_SDK_TESTING block, run by ctest as ggl-recipe-inline-test. No new framework or target

Root cause

manifest_selection() in modules/ggl-recipe/src/recipe.c carries a two-channel
contract: the return value answers "did I see malformed input?", while the
out-parameter answers "did this manifest match?". GG_ERR_OK is therefore
overloaded — it is returned both on a match and on a deliberate skip — so any
path that exits without writing the out-parameter is read by callers as "skip
this manifest". Skip is reached by omission rather than by decision.

At the base revision, if (gg_map_get(platform, GG_STR("os"), &os_obj))
(recipe.c:400) used the presence of the os key as the gate around the entire
remaining compatibility-and-selection body (recipe.c:401-484) — which contains
every statement that can assign the out-parameter. With os absent, the whole
body was skipped, the function returned GG_ERR_OK having selected nothing, the
caller's error guard did not fire, and the post-loop NULL check produced
No Manifest was found for linux (recipe.c:609) → GG_ERR_FAILURE.

The comment at recipe.c:398 — // If OS is not provided then do nothing —
described the code accurately and the requirement inaccurately: "impose no OS
requirement" was implemented as "do nothing", and in this function doing nothing
means being skipped. Tellingly, architecture one level down was already
implemented correctly (absent → zero-length buffer → accepted at recipe.c:434),
but only reachable when os was present.

The prediction this hypothesis made, and it held: making an absent os yield
a wildcard instead of skipping the body, with everything downstream semantically
unchanged, makes the failure disappear. It did, on the first attempt.

Set aside, one line each:

• Extract a platform_is_compatible() predicate (the alternative fix shape
  proposed during localization). Rejected: it bundles a refactor with a bug fix,
  which docs/CONTRIBUTING.md explicitly asks contributors not to do, and
  manifest_selection() already carries // TODO: Refactor it, so maintainers
  have scoped that separately. Its one correctness argument — the unreachable
  architecture type check — is delivered by this fix anyway. Offered as a
  follow-up below.
• manifest_selection() never reads architecture.detail, though nucleus lite
  advertises it. A real defect, but a different one — not folded in.
• Cloud-side resolver behaviour — not in this repository. See "Scope".

The fix

One file: modules/ggl-recipe/src/recipe.c.

The production change is four edits inside manifest_selection():

1. GgObject *os_obj; → GgObject *os_obj = NULL; so absence is representable.
2. The gg_map_get(... "os" ...) block now contains only the non-string type
   check; it no longer gates anything.
3. An absent os defaults to GG_STR("*"), i.e. "no OS requirement", applied
   with the same optional-fetch shape the adjacent architecture block already
   uses. recipe.c:332 already does GgBuffer arch_detail = GG_STR("");, so this
   is established local style.
4. The former gated body is de-indented one level so it always runs.

Why this is minimal: the wildcard default means the OS predicate itself needs
no edit at all. The runtime gate, the OS predicate, the architecture
fetch/validate/default, the architecture predicate, the Lifecycle pick, the
Selections pick, and the all default are all byte-identical apart from
indentation — git diff -w reduces the production change to exactly the four
items above. The remainder of the diff is the unavoidable re-indent plus tests.
No public signature changes; manifest_selection is static, and
modules/ggl-recipe/include/ggl/recipe.h is untouched.

Blast radius of the bug being fixed — one root cause, four entry points:

Entry point	Path	Effect of the bug
modules/ggdeploymentd/src/deployment_handler.c:722	ggl_get_recipe_artifacts_for_platform	artifact download fails
modules/recipe2unit/src/unit_file_generator.c:499	select_linux_lifecycle	systemd unit generation fails
modules/recipe-runner/src/runner.c:385	select_linux_lifecycle	lifecycle scripts never run
modules/ggl-docker-client/src/docker_artifact_cleanup.c:116, :260	ggl_get_recipe_artifacts_for_platform	Docker image cleanup blocked

Behaviour changes worth accepting knowingly

Both follow from the fix being correct, and both are user-visible:

1. Manifest precedence. In
   Manifests: [{runtime: "*"}, {runtime: "*", os: linux, architecture: amd64}]
   the first entry used to be skipped and the second selected; the first is now
   selected. That is correct first-match semantics and matches classic nucleus,
   but it changes which Lifecycle / Artifacts block a real deployment picks.
2. One previously-tolerated malformed recipe now hard-fails.
   Platform: {runtime: "*", architecture: 42} with no os used to be silently
   skipped, letting a later valid manifest win. Because the architecture type
   check was also nested inside the os gate, de-nesting makes it reachable, so
   this now returns GG_ERR_INVALID and fails the recipe. Consistent with how a
   malformed os already behaved, and it closes criterion 5, but it is a
   strictness increase beyond the reported defect.

Evidence

Oracle — the failing test that now passes:

• Test: modules/ggl-recipe/src/recipe.c ::
  select_manifest_platform_without_os_or_architecture

• Command: ctest --test-dir <build> -R ggl-recipe-inline-test --output-on-failure

• Before the patch, at base revision b33c36822f2f5f9b8ea873954142bfa98dff7aaa:

  E[ggl-recipe] modules/ggl-recipe/src/recipe.c:609: No Manifest was found for linux
  modules/ggl-recipe/src/recipe.c:758:test_gg_select_manifest_platform_without_os_or_architecture:FAIL: Return value was not GG_ERR_OK
  E[gg-test] .../unity/gg_test/process_wait.c:27: Process 8 exited with status 1.
  
  -----------------------
  10 Tests 1 Failures 0 Ignored
  FAIL
  

• After the patch: passes. That symptom no longer occurs.
  ggl-recipe-inline-test reports 19 Tests 0 Failures 0 Ignored / OK.

Reproduction was deterministic (1/1); the defect is a pure control-flow branch on
map-key presence, with no timing or I/O dependence.

Regression run — build and test:

Check	Command	Result
Build	cmake --build <build> -j$(nproc)	clean — 0 errors, 0 warnings
Test suite	ctest --test-dir <build> --output-on-failure	100% tests passed, 0 tests failed out of 10
Lint	nix flake check -L	not run — see below

• Patch-induced failures: none.
• Pre-existing failures at base revision, deliberately left alone: the test
  suite was clean at base, so there are none in the recorded command set. Two
  environment-specific ones were found by extra checks and left alone; see below.

Checks that could NOT be run, and why — this is the honest gap in
verification, and a maintainer with CI should simply let the gate answer it:

nix is not installed on the build host and cannot be, so the repository's real
CI entrypoint nix flake check -L never ran. That leaves unverified:
clang-tidy, iwyu, cmake-lint, spelling (cspell), build-clang, and
build-musl-pi. Two partial substitutes were run instead:

• clang-format (v22.1.8, obtained separately since the host has none): the
  changed file's only violation is a pre-existing one at
  is_recipe_variable_valid_three_part, which this patch does not touch and
  which is present at base revision. Zero violations in the changed region. A
  control run at base revision found that same violation and zero across 40 other
  module files, so this version is very nearly — not exactly — the one CI uses. It
  was used only as a check, never as a blanket -i reformat, and the pre-existing
  violation was deliberately left alone rather than "fixed".
• Clean build with -D ENABLE_WERROR=1 (what CI's build-clang enables):
  fails with 2 errors, both 'noreturn' function does return in
  _deps/unity-src/src/unity.c — vendored third-party Unity source, not
  repository code. Proven pre-existing by re-running the identical build with the
  patch stashed at base revision: same 2 errors, same 2 lines. It is gcc-specific
  (gcc 11.4) and invisible in CI because CI's -Werror checks use clang/musl
  while its unit-tests check sets BUILD_TESTING without ENABLE_WERROR, so
  that combination never occurs upstream. No repository source file emitted a
  single warning under -Werror, including the patched file.

The most material unverified check is clang-tidy: .clang-tidy has
readability-function-cognitive-complexity active and manifest_selection()
still carries a NOLINTNEXTLINE for it. This patch removes one nesting level, so
the suppression is either still valid or now unnecessary — that is reasoned, not
measured.

Reviewer verdict

An independent critic reviewed the diff, the acceptance criteria, the rejected
alternative hypothesis, and the test results, without access to the patch
author's self-assessment.

Field	Value
Verdict	concur
Confidence	Medium
Outcome applied	Patch preserved

Advisory notes, surfaced rather than acted on. They did not block delivery by
design — the reviewer is advisory, and it is the test run that verified the patch.

1. An explicitly empty os: is still silently skipped — same symptom, one
   token from being fixed. The reviewer reports that the YAML decoder
   (modules/ggl-yaml/src/yaml_decode.c) renders every scalar as a buffer, so
   os: with no value becomes a zero-length buffer, fails the OS predicate, and
   still produces the exact error from this issue — while architecture: left
   empty is honoured as "no requirement". Two sibling optional fields, same
   spelling, opposite outcomes. The remedy is one token (accept os.len == 0, or
   apply the wildcard when the value is absent or empty). This was left as a
   deliberate design decision for maintainers, because no acceptance criterion
   covers it, it is not a regression (base revision behaves identically), and
   "absent" versus "explicitly empty" is a semantic call this change should not
   make unilaterally. If it is intentionally left out, a GG_LOGD naming the
   empty value would at least make the next report diagnosable. This is the one
   item worth a maintainer decision in this thread.
2. Add a two-manifest test to pin skip-versus-abort. The four negative tests
   use single-manifest fixtures and GG_TEST_ASSERT_BAD (which only asserts
   != GG_ERR_OK), so nothing asserts that a non-matching manifest is skipped
   while the loop continues rather than aborting the whole recipe. A
   [non-matching, matching] fixture would close that and lock in first-match
   precedence.
3. Silent skips have no diagnostics. The runtime gate logs why it skipped; the
   os mismatch and the architecture mismatch say nothing, and the architecture
   mismatch exits by falling through ~50 lines to the terminal return GG_ERR_OK.
   A GG_LOGD on each skip would have made this issue self-diagnosing.
4. The fall-through shape that caused this bug is still present. Converting
   the implicit fall-through into an explicit else { return GG_ERR_OK; } would
   remove it at near-zero diff cost — without adopting the full predicate
   extraction.
5. Nits: select_lifecycle_platform_without_os_or_architecture asserts only
   selected.len > 0 where deep equality would be stronger and consistent;
   assertion style is mixed across the new tests; and encoding "no os
   requirement" as the sentinel "*" makes the fix depend on "*" remaining in
   the predicate, where a bool os_specified would state the intent directly
   (a deliberate trade for a smaller diff).
6. The reviewer was barred from modifying the tree, so it could not mutation-test
   the new assertions; its discrimination claims rest on control-flow reasoning
   plus per-test log paths, not on observed red-on-mutation.

For the human reviewer

Focus here:

• Advisory note 1 is the decision this PR deliberately does not make: should
  an explicitly empty os: be treated as "no requirement" (consistent with
  architecture:) or as a non-match (stricter)? Domain knowledge about non-lite
  nucleus behaviour settles this instantly and would change one line.
• The two behaviour changes above — manifest precedence, and the newly
  hard-failing malformed recipe — are correct consequences of the fix, but they
  affect real deployments. Please accept them knowingly.
• Whether the strictness increase in criterion 5 is wanted now or should be
  split into its own change.
• CI is the arbiter for the unrun checks, particularly clang-tidy against
  the existing NOLINTNEXTLINE, and cspell against the new identifiers
  (all ordinary words or existing repository terms).
• Convention deviations, both forced by the execution environment rather than
  chosen: docs/CONTRIBUTING.md asks for a dev/-prefixed branch and, for
  external contributors, a PR against an internally created dev/ branch. This
  branch is named wt/LiAllanPersonalAICapabilities-t6 and targets main
  directly. Happy to re-target or rename.

Branch: wt/LiAllanPersonalAICapabilities-t6 — base revision
b33c36822f2f5f9b8ea873954142bfa98dff7aaa

[allanli4]

Superseded by #1160, which carries this same change plus the clang-format fix that CI requires (CI formats with clang-format 19.1.7; this branch was checked against a newer version that disagrees).

This PR was raised from a fork branch, and uat.yml gates list-tests on head.repo.full_name == 'aws-greengrass/aws-greengrass-lite' — so UAT is skipped here and never verified the change. Per docs/CONTRIBUTING.md, the work now lives on dev/recipe-platform-os-optional in this repository, where the full UAT matrix is eligible to run.

Closing in favour of #1160. No review had started here, so no review effort is lost.