fix(runtime): resolve every interpolatable field before lowering (#276)

[pierrick-fonquerne]

Summary

Runtime part of #276. Export placeholder rewriting is split to #308.

Only environment values and the command were substituted at runtime, while image references, entrypoint, working directory, volumes, healthcheck and Dockerfile build inputs were advertised and scanned as interpolatable but never resolved. image: "app:${env.TAG}" passed validate and then ran the literal reference; entrypoint was omitted from both scanning and substitution.

Root cause

Two lists had drifted: ResourceKind::interpolatable_strings (scanning, drives validation and implicit dependency discovery) advertised nine field groups, while the runtime substitution resolved only two. Substitution also ran after lowering, so the parsed image/volume/healthcheck fields could not carry a resolved value back.

Fix

• One canonical field walk. interpolatable_fields_mut is the single enumeration of every interpolatable string. interpolatable_strings (read-only scan) and the new interpolate_in_place (substitution) both derive from it, so scanning and substitution cannot drift. entrypoint is now covered.
• Interpolate before lowering. The plan keeps resources in their raw manifest form; the runtime resolves every field and lowers to a ContainerSpec only at start time. The canonical image, volume and healthcheck parsers therefore only ever see fully resolved values, which is what allows a volume mapping to be interpolated at all.

Behaviour changes

• Spec-build failures (invalid image reference, port, volume mapping) now surface when a resource starts rather than when the plan is built. Both happen during lightshuttle up. lifecycle_plan test updated accordingly.
• secrets check scope is unchanged: it still reports only references in fields that become container env vars or command arguments (regression guard F3 preserved).
• Export still emits ${env.*} verbatim; placeholder rewriting is export: emit deployment placeholders for unresolved ${env.*} references #308.

Tests

• interpolation_before_lowering.rs: a resource with ${env.*} in image, env, volume, entrypoint, command, working dir and healthcheck is started; the started spec contains only resolved values.
• resource.rs: canonical-walk scan covers every field incl. entrypoint; interpolate_in_place resolves them; a Dockerfile drift guard covers build inputs.
• lifecycle_plan.rs: an invalid port spec now surfaces at start.

cargo fmt --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo test --workspace --all-features all green locally.

[github-actions]

Team Review

Verdict: DISCUSS 💬 · 0 confirmed · 6 contested

🇬🇧 English

Overview
This pull request addresses a critical issue with environment variable interpolation in lightshuttle-manifest by centralizing interpolation logic and ensuring all interpolatable fields are resolved before resource startup. The changes improve consistency between scanning and substitution, preventing malformed references from reaching Docker. The PR also adds comprehensive tests to validate the interpolation logic. Overall, the implementation is robust but requires attention to security and visibility concerns.

Strengths

• Centralizes interpolation logic to prevent drift between scanning and substitution.
• Adds comprehensive tests covering all interpolatable fields, including edge cases.
• Maintains architectural quality by adhering to SOLID principles and layered architecture.

⚠️ Contested (adversarial verification)

• 🔴 security crates/lightshuttle-manifest/src/model/resource.rs:107: The interpolatable_fields_mut function must be marked as private to prevent external use and avoid command injection risks from unsanitized interpolated values. (via correctness, security, architecture)
  • The function is already marked as private in the code under review.
• 🔴 security crates/lightshuttle-manifest/src/model/resource.rs:310: The line *field = interpolator.resolve(field)?; in interpolate_in_place could lead to command injection if interpolated values are not properly sanitized. (via security)
  • The code shows that interpolated values are resolved using a controlled interpolator, suggesting proper sanitization is handled elsewhere.
• 🟡 security crates/lightshuttle-runtime/src/lifecycle/handle.rs:211: The change from image_label(&node.spec.image) to node.image_label.clone() alters image label handling and could potentially expose sensitive information. (via security, architecture)
  • The change from image_label(&node.spec.image) to node.image_label.clone() is purely a refactoring and does not alter the handling of image labels.
• 🟡 design crates/lightshuttle-manifest/src/model/resource.rs:107: The interpolatable_fields_mut function lacks detailed documentation to ensure clarity for future maintainers. (via architecture)
  • The finding is about lack of detailed documentation, which is a stylistic concern and not a real, observable impact.
• 🟡 design crates/lightshuttle-runtime/src/lifecycle/env_report.rs:122: The comment explaining the scope of environment references could be moved to a more central location for better visibility, as noted by multiple agents. (via correctness, architecture)
  • The comment is already in a central location and clearly explains the scope of environment references.
• 🟡 design crates/lightshuttle-runtime/src/lifecycle/handle.rs:211: The image label extraction logic could be moved to a more appropriate location to avoid duplication, as suggested by the architecture agent. (via architecture)
  • The image label extraction logic has been intentionally moved to the node struct to avoid duplication.

🇫🇷 Français

Vue d'ensemble
Cette demande de tirage résout un problème critique lié à l'interpolation des variables d'environnement dans lightshuttle-manifest en centralisant la logique d'interpolation et en s'assurant que tous les champs interpolables sont résolus avant le démarrage des ressources. Les modifications améliorent la cohérence entre l'analyse et la substitution, empêchant les références malformées d'atteindre Docker. La PR ajoute également des tests complets pour valider la logique d'interpolation. Globalement, l'implémentation est robuste mais nécessite une attention particulière aux questions de sécurité et de visibilité.

Points forts

• Centralise la logique d'interpolation pour éviter les écarts entre l'analyse et la substitution.
• Ajoute des tests complets couvrant tous les champs interpolables, y compris les cas limites.
• Maintient la qualité architecturale en respectant les principes SOLID et une architecture en couches.

⚠️ Contestés (vérification adversariale)

• 🔴 security crates/lightshuttle-manifest/src/model/resource.rs:107: La fonction interpolatable_fields_mut doit être marquée comme privée pour empêcher son utilisation externe et éviter les risques d'injection de commandes dus à des valeurs interpolées non assainies. (via correctness, security, architecture)
  • La fonction est déjà marquée comme privée dans le code en revue.
• 🔴 security crates/lightshuttle-manifest/src/model/resource.rs:310: La ligne *field = interpolator.resolve(field)?; dans interpolate_in_place pourrait entraîner une injection de commandes si les valeurs interpolées ne sont pas correctement assainies. (via security)
  • Le code montre que les valeurs interpolées sont résolues à l'aide d'un interpolateur contrôlé, suggérant que la désinfection appropriée est gérée ailleurs.
• 🟡 security crates/lightshuttle-runtime/src/lifecycle/handle.rs:211: Le passage de image_label(&node.spec.image) à node.image_label.clone() modifie la gestion des étiquettes d'image et pourrait potentiellement exposer des informations sensibles. (via security, architecture)
  • Le changement de image_label(&node.spec.image) à node.image_label.clone() est purement une refactorisation et ne modifie pas le traitement des étiquettes d'image.
• 🟡 design crates/lightshuttle-manifest/src/model/resource.rs:107: La fonction interpolatable_fields_mut manque de documentation détaillée pour garantir la clarté pour les futurs mainteneurs. (via architecture)
  • La préoccupation porte sur l'absence de documentation détaillée, qui est une question de style et non un impact réel et observable.
• 🟡 design crates/lightshuttle-runtime/src/lifecycle/env_report.rs:122: Le commentaire expliquant la portée des références d'environnement pourrait être déplacé vers un emplacement plus central pour une meilleure visibilité, comme l'ont noté plusieurs agents. (via correctness, architecture)
  • Le commentaire est déjà dans une position centrale et explique clairement la portée des références d'environnement.
• 🟡 design crates/lightshuttle-runtime/src/lifecycle/handle.rs:211: La logique d'extraction des étiquettes d'image pourrait être déplacée vers un emplacement plus approprié pour éviter la duplication, comme suggéré par l'agent d'architecture. (via architecture)
  • La logique d'extraction de l'étiquette d'image a été intentionnellement déplacée vers la structure de nœud pour éviter la duplication.

---

Agents: Correctness, Security, Architecture, Performance
Findings: 7 raw -> 6 merged
Model: codestral-latest + mistral-large-latest · Diff: 11 file(s)