Skip to content

Isolate dry-run telemetry and make agents own security preflight - #66954

Merged
pelikhan merged 9 commits into
mainfrom
pelikhan-debug-smoke-agy
Oct 8, 2026
Merged

pelikhan merged 9 commits into
mainfrom
pelikhan-debug-smoke-agy

Conversation

@pelikhan

@pelikhan pelikhan commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Make workflow security preflight and diagnostic cleanup agent-owned, and remove telemetry export configuration from compiled --dry-run locks.

  • Remove OTEL_* and GH_AW_OTLP_* variables from workflow, job, step, container, and service environments, including user-defined values and engine/pre/post-step environments.
  • Prepare a cloned telemetry-free workflow environment before job and header generation, preventing residual OTLP masking steps and stale environment-source metadata. Dry-run presence guards preserve unrelated values containing literal telemetry variable names without generating masking steps or observability summaries.
  • Clear automatic OTLP export/authentication configuration in dry-run job data. Retain final filtering before secret collection and after reusable-workflow regeneration so telemetry-only secrets do not remain in declarations or manifests.
  • Preserve retained env scalar text and colon-space quoting, including dates, engine headers, multiline values and anchored/aliased environments.
  • Omit daily credit accounting and its app/ledger wiring in dry-run. Preserve existing per-run caps/expressions, promote explicitly configured/imported daily limits when needed, and retain the standard per-run default otherwise.
  • Add manual dispatch with maintainer/admin actor authorization, preserving an existing narrower privileged role subset and dispatch/reusable inputs; remove bot exemptions.
  • Log actual dry-run field/key changes, removed telemetry keys, disabled jobs and forced CLI flags through DEBUG=workflow:compiler_development,cli:compile_development, without configuration values or duplicate unchanged-state logs.
  • Preserve ordinary compilation, source hashes, network permissions, custom scripts, and input data/compiler reuse.
  • Keep technical preflight, independent review, evidence and cleanup agent-owned. Explicit bounded session authorization is permitted, but any compiler security warning invalidates it until fresh authorization; runtime readiness metadata is not a pre-dispatch gate.
  • Provide separate short user-visible security-review and dry-run result sentences. Any permitted live test must execute the exact reviewed lock revision; restoring a normal lock does not transfer suppression or approval.

Validation

  • Build, standard Go lint, schema freshness, impacted unit checks and focused compiler/CLI/credit/role/env/idempotence regressions passed.
  • The required publication gate remains blocked by 19 pre-existing custom-lint diagnostics: 15 in observability_otlp.go, the existing oversized builder function in workflow_builder.go, two unchanged indexing diagnostics in awf_env.go, and one unchanged indexing diagnostic in compile_development.go. New lint issues were fixed; no check was disabled or unrelated cleanup applied. This PR is not declared merge-ready.
  • make check-workflow-drift separately compiled all 333 workflows with normal generated locks unchanged.
  • Fresh ./gh-aw compile smoke-agy --dry-run --json passed strict/source/model validation and native shellcheck with zero errors/warnings. Verified 53 parsed environment mappings have no OTEL_* or GH_AW_OTLP_* keys, daily accounting is omitted, the admin/maintainer role gate is present, and the 50-credit per-run cap remains.
  • Independent source-scoped security review examined telemetry, credits, authorization, local test payloads and logging; no confirmed vulnerability. Hosted/custom payload execution is not approved or verified by that review.
  • Diagnostic evidence was retained privately and its disposable checkout removed. Normal smoke source/lock hashes are unchanged. No Actions dispatch occurred.
  • CI on the final pushed HEAD is unverified; the initial CI snapshot had no failed checks. No CI trigger or merge was attempted.

Architecture decision

Draft ADR-66954: telemetry suppression in diagnostic locks. Maintainer acceptance is not implied. The record describes scoped YAML rewriting and its formatting/alias limitations, separately authorized diagnostic execution, and the associated bounded dry-run controls.

Custom scripts that configure their own exporters remain outside the compiler's env suppression.

pelikhan and others added 2 commits October 8, 2026 10:04
Permit compile-only dry runs with scoped snapshot and restoration of generated changes, while preserving live authorization gates.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Suppress automatic OTLP export and authentication in dry-run job data, filter telemetry variables from emitted Actions env mappings, and preserve normal compilation and executable script contents.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@pelikhan pelikhan changed the title Make agents own workflow security preflight and dry-run cleanup Isolate dry-run telemetry and make agents own security preflight Oct 8, 2026
@pelikhan
pelikhan marked this pull request as ready for review October 8, 2026 18:32
Copilot AI balanced review requested due to automatic review settings October 8, 2026 18:32
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

Lean already. Ship.

Generated by Ponytail Reviewer for #66954

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills...

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (401 new lines in pkg/, >100 threshold) but does not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/66954-strip-telemetry-env-from-dry-run-locks.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

🔍 Decision inferred from the diff
  • Decision: suppress telemetry configuration in dry-run locks by rewriting emitted YAML after generation, using a node-aware env-mapping filter that splices line-range edits back into the original text (pkg/workflow/compiler_development_telemetry.go, 136 new lines; hooked from compiler_yaml.go and compiler_development.go).
  • Driver: dry-run locks are retained, shareable diagnostic artifacts that never execute, yet disclosed OTEL_* / GH_AW_OTLP_* endpoints and pulled telemetry-only credentials into generated secret declarations and manifests.
  • Alternatives: (1) suppress at configuration-construction time at every telemetry contributor site; (2) full YAML unmarshal/marshal round-trip with keys deleted; (3) leave dry-run locks unchanged since they are never dispatched.
  • Consequences: single chokepoint that future telemetry producers cannot bypass, and formatting/anchors/executable scalars preserved — at the cost of a post-emission self-parse pass, prefix-based (not semantic) matching, and two invocation points that a new emission path could bypass silently.

The .github/aw/ and .github/skills/review-agentic-workflows/ agent-ownership changes are recorded as a Neutral consequence, since they are a process change separable from the compiler decision.

📋 What to do next
  1. Review the draft ADR committed to your branch — it was generated from the PR diff
  2. Complete the missing sections — add context the AI couldn't infer, refine the decision rationale, and list real alternatives you considered
  3. Commit the finalized ADR to docs/adr/ on your branch
  4. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-66954: Strip Telemetry Environment Variables from Dry-Run Locks

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

"AI made me procrastinate on key design decisions. Because refactoring was cheap, I could always say 'I'll deal with this later.' Deferring decisions corroded my ability to think clearly."

ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you.

📋 Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

  • Context — What is the problem? What forces are at play?
  • Decision — What did you decide? Why?
  • Alternatives Considered — What else could have been done?
  • Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · pi · opus50 · 40.4 AIC · ⌖ 50.8 AIC · ⊞ 1.8K · ◷
Comment /review to run again

Remove workflow activation and secret availability/validity metadata as pre-dispatch gates while preserving credential-flow review, destination authorization, and live safety controls.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Comment Memory

Peek at saved memory (pr-code-quality-reviewer)
reviewed_at: 2026-10-08T18:35:42.963+00:00
review_event: REQUEST_CHANGES
top_themes:
  - dry-run telemetry cleanup leaves OTLP-gated helper steps enabled via stale WorkflowData.Env
files_reviewed:
  - .github/aw/debug-agentic-workflow.md
  - .github/aw/debug-security-review.md
  - .github/skills/review-agentic-workflows/SKILL.md
  - docs/src/content/docs/reference/compilation-process.md
  - docs/src/content/docs/setup/cli.md
  - pkg/cli/compile_development_report.go
  - pkg/workflow/compiler_development.go
  - pkg/workflow/compiler_development_telemetry.go
  - pkg/workflow/compiler_development_telemetry_test.go
  - pkg/workflow/compiler_yaml.go
comment_count: 1

Note

This comment is managed by comment memory.

Expand the saved memory block to view or edit the persistent context for this thread.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 104.4 AIC · ⌖ 5.57 AIC · ⊞ 19.8K · ◷
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes

Dry-run still leaves OTLP-only helper behavior in the emitted lock because the new path strips rendered env mappings after job generation but keeps the already-injected telemetry env in WorkflowData.

Blocking theme
  • dryRunConclusionData clears OTLP fields and removes observability, but it does not clear WorkflowData.Env.
  • OTLP-gated helpers still key off data.Env, so dry-run locks can continue to emit telemetry-specific steps such as Generate observability summary, Mask OTLP telemetry headers, and Mask OTLP custom attribute values.
  • The added regression only checks emitted env mappings plus a couple of auth/default-credential steps, so this hole is currently untested.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 104.4 AIC · ⌖ 5.57 AIC · ⊞ 19.8K
Comment /review to run again

Comment thread pkg/workflow/compiler_development.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Environment rewriting can alter retained values and remove quoting required for valid YAML.

2 open findings
What changed in this PR

Isolates telemetry from compiled dry-run workflows and assigns security preflight and diagnostic cleanup to agents.

Changes:

  • Removes telemetry environment variables and automatic OTLP configuration from dry-run output.
  • Adds regression coverage for environment scopes, reusable workflows, and compiler reuse.
  • Updates guidance for agent-owned validation, evidence collection, and scoped cleanup.
File Description
pkg/​workflow/​compiler_yaml.go Filters telemetry before secret collection and after regeneration.
pkg/​workflow/​compiler_development.go Clears dry-run telemetry configuration.
pkg/​workflow/​compiler_development_telemetry.go Implements environment filtering.
pkg/​workflow/​compiler_development_telemetry_test.go Adds telemetry-isolation regressions.
pkg/​cli/​compile_development_report.go Updates the dry-run scope message.
docs/​src/​content/​docs/​setup/​cli.md Documents telemetry suppression.
docs/​src/​content/​docs/​reference/​compilation-process.md Explains suppression scope and limitations.
.github/​skills/​review-agentic-workflows/​SKILL.md Assigns review and cleanup responsibilities.
.github/​aw/​debug-security-review.md Defines compile-only evidence and restoration rules.
.github/​aw/​debug-agentic-workflow.md Aligns debugging guidance with agent ownership.

🧠 Review effort: Balanced

Comment thread pkg/workflow/compiler_development_telemetry.go Outdated
Comment thread pkg/workflow/compiler_development_telemetry.go
pelikhan and others added 2 commits October 8, 2026 12:00
Prepare cloned telemetry-free workflow environment before building jobs and headers, omit stale env-source metadata, and disable telemetry mask detection for dry-run data even when retained values contain OTEL literals. Preserve the final all-scope filter and normal compile behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Always summarize the reviewed artifact, outcome, checks performed, and material findings or coverage gaps in short separate user-facing sentences, including blocked and unavailable attempts.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (pkg/workflow/compiler_development.go:91): This doesn't actually disable all dry-run telemetry behavior, because result.Env still contains the OTLP vars injected earlier, so helpers like isOTLPEnabled, isOTLPHeadersPresent, and isOTLPAttributesPresent will keep emitting observability summary and mask steps into the lock. - Isolate dry-run telemetry and make agents own security preflight #66954 (comment)
  3. Review (pkg/workflow/compiler_development_telemetry.go:115): Decoding retained values into any changes unrelated environment values when a telemetry key is removed from the same map. For example, engine.env.START_DATE: "2026-01-01" is emitted unquoted by appendEnvVarLine; yaml.v3 then decodes it as time.Time, and this rewrite emits a timestamp instead of the original date. Decode scalar environment values as strings to preserve their text, and add a regression with a date-valued engine variable beside a telemetry variable. - Isolate dry-run telemetry and make agents own security preflight #66954 (comment)
  4. Review (pkg/workflow/compiler_development_telemetry.go:132): This re-marshaling bypasses quoteEnvValuesContainingColonSpace, which the existing environment emitters apply. When an env map contains a telemetry variable and ANTHROPIC_CUSTOM_HEADERS: "x-aw-gw-github-repo: ${{ github.repository }}"filtering removes the required quoting from the retained header, producing invalid YAML. Apply the same quoting helper before restoring the anchor, and extend the regression in compiler_yaml_test.go:999–1042 to cover dry-run filtering with a neighboring telemetry key. - Isolate dry-run telemetry and make agents own security preflight #66954 (comment)

Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI.

Sous-chef head: a0ea82d
Sous-chef work: 22e24c893a19cedc857c646084fdc49334f298274fc06a9b9a9f838678f5b82e 57c11e4e7c8ee8e741817ce08ea9ffc74431fe8f971cd8cbecc5ded826190e36 ac61abc40561c0eb834aa649a08b67b43d0852fa433082d758adb4e182ca2e62
Sous-chef state: 26a3f13734390a902f837ca2ede85ace82fafa671216d8924e41c393f1992ad0

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 7.98 AIC · ⌖ 15 AIC · ⊞ 1K · ◷
Comment /souschef to run again

Copilot AI and others added 3 commits October 8, 2026 19:55
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Preserve retained env scalars and quoting; keep dry-run diagnostics bounded, restrict manual dispatch to maintainers/admins, and report value-free mutation logs. Document explicit session grants and compiler-warning invalidation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…elikhan-debug-smoke-agy

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot AI requested a review from gh-aw-bot October 8, 2026 20:13
@pelikhan
pelikhan merged commit 5c6e8cb into main Oct 8, 2026
2 checks passed
@pelikhan
pelikhan deleted the pelikhan-debug-smoke-agy branch October 8, 2026 20:19
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.91.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants