fix(ir): reject cross-pipeline duplicate StepIds; strict de-indent in lower_raw_yaml; document EnvValue::Literal contract

src/compile/ir/env.rs

@@ -43,7 +43,18 @@ use super::output::OutputRef;
 /// `env:` mapping entry.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum EnvValue {
-    /// Plain string literal.
+    /// Plain string literal — emitted **verbatim** into the YAML
+    /// `env:` value position with no escaping or sanitisation.
+    ///
+    /// **Compiler-internal use only.** Construction sites must pass
+    /// a hardcoded string, a constant, or a value derived from
+    /// front-matter that has already been routed through
+    /// `crate::validate::reject_pipeline_injection` (or a stronger
+    /// equivalent). Never construct from raw user-supplied input —
+    /// use [`EnvValue::PipelineVar`] (`$(NAME)` macro form) or
+    /// [`EnvValue::Secret`] when the value should come from an ADO
+    /// variable, or [`EnvValue::AdoMacro`] for predefined variables
+    /// on the allowlist.
     Literal(String),
     /// ADO predefined-variable macro. Must be a member of
     /// [`ALLOWED_ADO_MACROS`].
@@ -124,7 +135,10 @@ pub const ALLOWED_ADO_MACROS: &[&str] = &[
 ];
 
 impl EnvValue {
-    /// Construct an [`EnvValue::Literal`].
+    /// Construct an [`EnvValue::Literal`]. **Compiler-internal use
+    /// only** — see the variant's doc-comment for the contract on
+    /// safe inputs (hardcoded strings, constants, or values
+    /// pre-validated against `reject_pipeline_injection`).
     pub fn literal(s: impl Into<String>) -> Self {
         EnvValue::Literal(s.into())
     }

src/compile/ir/graph.rs

@@ -209,21 +209,34 @@ fn add_manual_job_edges(job: &super::job::Job, g: &mut Graph) {
 fn index_job_steps(stage: Option<StageId>, job: &super::job::Job, g: &mut Graph) -> Result<()> {
     for step in &job.steps {
         if let Some(id) = step.id() {
-            // Step ids are pipeline-wide identifiers in ADO when
-            // referenced via `dependencies.<job>.outputs['<step>.X']`,
-            // so duplicate ids across jobs are technically allowed if
-            // both jobs are referenced through the qualifying job
-            // name. We still reject true duplicates inside the SAME
-            // job, which would silently shadow.
-            if let Some(prev) = g.step_locations.get(id)
-                && prev.stage == stage
-                && prev.job == job.id
-            {
-                bail!(
-                    "ir::graph: duplicate StepId '{}' inside job '{}'",
-                    id,
+            // [`super::output::OutputRef`] carries only a [`StepId`] —
+            // no qualifying job name — so the IR's producer-resolution
+            // is keyed on `StepId` alone. Duplicate ids across jobs
+            // (or stages) would silently overwrite `step_locations`
+            // and every `OutputRef` to that id would resolve to
+            // whichever producer was indexed last. ADO YAML allows
+            // qualifying duplicates at the wire level (`dependencies.<job>.outputs[...]`)
+            // but the IR does not model the job-qualified form, so
+            // duplicates are rejected pipeline-wide.
+            if let Some(prev) = g.step_locations.get(id) {
+                let prev_loc = format!(
+                    "{}.{}",
+                    prev.stage
+                        .as_ref()
+                        .map(|s| s.as_str())
+                        .unwrap_or("<no stage>"),
+                    prev.job
+                );
+                let this_loc = format!(
+                    "{}.{}",
+                    stage.as_ref().map(|s| s.as_str()).unwrap_or("<no stage>"),
                     job.id
                 );
+                bail!(
+                    "ir::graph: duplicate StepId '{id}' — previously declared in {prev_loc}, \
+                     now also declared in {this_loc}. StepIds must be pipeline-wide unique \
+                     because OutputRef carries only the StepId (no job qualifier)."
+                );
             }
             let outputs: BTreeSet<String> = collect_step_outputs(step);
             g.step_locations.insert(
@@ -899,6 +912,55 @@ mod tests {
         assert!(msg.contains("B"));
     }
 
+    #[test]
+    fn cross_job_duplicate_step_id_is_rejected() {
+        // OutputRef carries only StepId (no job qualifier), so a
+        // duplicate StepId across two jobs would silently overwrite
+        // step_locations and every consumer would resolve to the
+        // last-indexed producer. Reject loudly at graph-build time.
+        let dup = StepId::new("synthPr").unwrap();
+        let mut job_a = Job::new(JobId::new("A").unwrap(), "A", pool());
+        job_a.push_step(Step::Bash(
+            BashStep::new("s", "echo a")
+                .with_id(dup.clone())
+                .with_output(OutputDecl::new("X")),
+        ));
+        let mut job_b = Job::new(JobId::new("B").unwrap(), "B", pool());
+        job_b.push_step(Step::Bash(
+            BashStep::new("s", "echo b")
+                .with_id(dup)
+                .with_output(OutputDecl::new("X")),
+        ));
+        let p = pipe(PipelineBody::Jobs(vec![job_a, job_b]));
+        let err = build_graph(&p).unwrap_err();
+        let msg = format!("{err:#}");
+        assert!(msg.contains("duplicate StepId 'synthPr'"), "got: {msg}");
+        assert!(msg.contains("previously declared in"), "got: {msg}");
+        assert!(msg.contains("pipeline-wide unique"), "got: {msg}");
+    }
+
+    #[test]
+    fn cross_stage_duplicate_step_id_is_rejected() {
+        // Same contract across stages.
+        let dup = StepId::new("marker").unwrap();
+        let mut job_a = Job::new(JobId::new("A").unwrap(), "A", pool());
+        job_a.push_step(Step::Bash(
+            BashStep::new("m", "echo a").with_id(dup.clone()),
+        ));
+        let mut stage_a = Stage::new(StageId::new("StageA").unwrap(), "StageA");
+        stage_a.push_job(job_a);
+
+        let mut job_b = Job::new(JobId::new("B").unwrap(), "B", pool());
+        job_b.push_step(Step::Bash(BashStep::new("m", "echo b").with_id(dup)));
+        let mut stage_b = Stage::new(StageId::new("StageB").unwrap(), "StageB");
+        stage_b.push_job(job_b);
+
+        let p = pipe(PipelineBody::Stages(vec![stage_a, stage_b]));
+        let err = build_graph(&p).unwrap_err();
+        let msg = format!("{err:#}");
+        assert!(msg.contains("duplicate StepId 'marker'"), "got: {msg}");
+    }
+
     #[test]
     fn stage_condition_rejects_same_stage_step_output() {
         let producer_id = StepId::new("producer").unwrap();

src/compile/ir/ids.rs

@@ -16,6 +16,20 @@
 //! [`Result`] so call sites can surface a meaningful error rather
 //! than panic.
 //!
+//! ## Uniqueness contract
+//!
+//! - [`StageId`] is unique within a [`super::Pipeline`].
+//! - [`JobId`] is unique within a stage (or pipeline-wide for
+//!   stage-less pipelines). The graph builder rejects duplicates
+//!   with a typed error.
+//! - **[`StepId`] is unique pipeline-wide** — not just within a job.
+//!   [`super::output::OutputRef`] carries only a `StepId` (no
+//!   qualifying job name), so the IR's producer-resolution is keyed
+//!   on `StepId` alone. The graph builder rejects cross-job
+//!   duplicates with a typed error. (ADO YAML technically allows
+//!   `dependencies.<job>.outputs[...]` to disambiguate, but the IR
+//!   does not model the job-qualified form.)
+//!
 //! `Display` round-trips to the original string. `AsRef<str>` is
 //! provided so ids slot into format strings cheaply.
 

src/compile/ir/lower.rs

@@ -824,8 +824,18 @@ pub(crate) fn lower_step(step: &Step, ctx: &LoweringContext<'_>) -> Result<Value
 /// The body must be a single YAML mapping; we accept it with or
 /// without a leading `- ` because some legacy emitters include it
 /// (they're emitting a step inside an enclosing sequence). When the
-/// `- ` is present, every subsequent line is also de-indented by two
-/// columns so the mapping parses as a top-level document.
+/// `- ` is present, every subsequent line is also de-indented by
+/// exactly two columns so the mapping parses as a top-level
+/// document.
+///
+/// # Errors
+///
+/// Returns `Err` if a `- `-prefixed body has a continuation line
+/// that is non-empty and does not start with at least two spaces.
+/// The current producers in [`crate::compile::standalone_ir`] all
+/// emit exactly two-space-indented continuations via
+/// [`step_value_to_dash_yaml`], so any failure here indicates an
+/// out-of-contract producer (compiler bug).
 fn lower_raw_yaml(raw: &str) -> Result<Value> {
     let trimmed = raw.trim_start();
     let body = if let Some(rest) = trimmed.strip_prefix("- ") {
@@ -836,9 +846,24 @@ fn lower_raw_yaml(raw: &str) -> Result<Value> {
         for (i, line) in rest.split_inclusive('\n').enumerate() {
             if i == 0 {
                 out.push_str(line);
-            } else {
-                out.push_str(line.strip_prefix("  ").unwrap_or(line));
+                continue;
             }
+            // Empty / whitespace-only continuation lines are fine
+            // verbatim (a literal block-scalar terminator, a blank
+            // line between keys, etc.).
+            if line.trim().is_empty() {
+                out.push_str(line);
+                continue;
+            }
+            let dedented = line.strip_prefix("  ").ok_or_else(|| {
+                anyhow::anyhow!(
+                    "ir::lower: Step::RawYaml continuation line is not indented by at \
+                     least two spaces (the leading `- ` prefix on the first line \
+                     requires every subsequent non-blank line to start with at least \
+                     two spaces so the mapping parses as a top-level document). Line: {line:?}"
+                )
+            })?;
+            out.push_str(dedented);
         }
         out
     } else {
@@ -1581,6 +1606,40 @@ mod tests {
         assert!(format!("{err:#}").contains("Step::RawYaml"));
     }
 
+    #[test]
+    fn raw_yaml_dash_prefix_rejects_unindented_continuation() {
+        // A `- bash: ...` first line obligates every non-blank
+        // continuation line to start with at least two spaces (the
+        // standard YAML sequence-item indent that the dedent strips).
+        // The previous `unwrap_or(line)` fallback silently emitted
+        // misaligned YAML; the strict form bails with a clear error
+        // citing the offending line.
+        let body = "- bash: echo hi\n  displayName: greet\nnot-indented: oops\n";
+        let err = super::lower_raw_yaml(body).unwrap_err();
+        let msg = format!("{err:#}");
+        assert!(
+            msg.contains("not indented by at least two spaces"),
+            "got: {msg}"
+        );
+        assert!(msg.contains("not-indented: oops"), "got: {msg}");
+    }
+
+    #[test]
+    fn raw_yaml_dash_prefix_accepts_blank_continuation_lines() {
+        // Blank/whitespace-only continuation lines are legal between
+        // mapping keys; the strict dedent must not bail on them.
+        let body = "- bash: echo hi\n\n  displayName: greet\n";
+        let v = super::lower_raw_yaml(body).unwrap();
+        let m = match v {
+            Value::Mapping(m) => m,
+            _ => panic!("expected mapping"),
+        };
+        assert_eq!(
+            m.get(Value::String("displayName".into())).and_then(|v| v.as_str()),
+            Some("greet")
+        );
+    }
+
     // ── Phase 0: top-level pipeline lowering tests ─────────────────
 
     /// `parameters:` with a Boolean default round-trips through emit

src/compile/ir/output.rs

@@ -93,9 +93,16 @@ impl OutputDecl {
 /// output it wants; at lower time the IR picks the correct ADO
 /// reference syntax based on whether the consumer lives in the same
 /// job / a sibling job in the same stage / a different stage.
+///
+/// **Uniqueness:** the [`StepId`] is the *only* key used to resolve
+/// the producer's location — there is no job-qualified form here.
+/// Step IDs are therefore required to be pipeline-wide unique; see
+/// [`crate::compile::ir::ids`] for the contract and
+/// [`crate::compile::ir::graph::build_graph`] for the enforcement.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct OutputRef {
-    /// The producer step's id.
+    /// The producer step's id. **Must be pipeline-wide unique** —
+    /// see type-level doc on uniqueness.
     pub step: StepId,
     /// The output variable name (must match an [`OutputDecl::name`]
     /// on the producer).