fix(scan): PR-quality root-cause pass — Helm-aware workload patches, manual/auto delivery parity, collision + placeholder gates - #205
Merged
Conversation
…oy/deployment.yaml theater pulse-agent#5/#6 (gitops): the workload-replicas/workload-health-probes skills fabricate a disconnected deploy/deployment.yaml stub for every app, even one with a perfectly real Helm chart -- root cause is _workload_yaml_paths()/_snapshot_file_text() depending on an analyzers.snapshot ContextVar that is only ever set during run_assessment()'s own analyzer pass and is always gone by the time a separate, later onboarding job calls these generators. - source_patches.enrich_workload_files_from_repo(): the same GitHub-REST read_file/tree_paths mechanism apply_containerfile_pin_only() / enrich_sbom_from_repo() already use to find the app's real workload manifest instead of guessing. When the workload templates `replicas:` via `{{ .Values.<key> }}` (Helm), patches the chart's own values.yaml numeric key instead of rewriting the templated line -- avoids inserting a second, conflicting literal replicas: key that would corrupt the chart. Drops (fail-closed) rather than opens a PR when no real workload can be found or the chart key is ambiguous/nested, same posture as self_managed_hpa.py / fleet_hpa.py's scaleTargetRef gates. - Wired into auto_delivery.py's existing pre-clear-evidence enrichment step and routes/assessments.py's manual Deliver path (parity). - workload_patches.py: helm_templated_replicas_key/chart_root_for_template_path/ values_yaml_path_for_chart/patch_values_numeric_key/values_yaml_replicas_at_least; verify_workload_replicas() now also accepts a values.yaml-shaped clear-evidence. - analyzers/ha_dr.py: the `replicas` finding was also Helm-blind at detection time (raw literal-digit regex only) -- a chart correctly setting replicaCount>=2 in values.yaml would never clear the finding even after a perfect fix merged. Resolves a templated replicas: line against any values.yaml in the repo before flagging. Tests: tests/test_workload_patches.py (new TestHelmChartAwareness, TestEnrichWorkloadFilesFromRepo), tests/test_ha_dr.py (Helm values.yaml resolution, still-flagged-below-minimum). Full non-browser suite: 3397 passed, 452 skipped, 0 failed. Co-authored-by: Cursor <cursoragent@cursor.com>
… closing the gate gap
Hello-World gitops#31 class of bug: this route's own hand-maintained
subset of quality gates (Phase A finding gate, file filter, wrong-layer
strip, per-cluster cap, clear-evidence) had already drifted once from
auto_validate_and_deliver()'s full bar (missing Phase C SSA/property
checks, the fleet HPA scaleTargetRef gate, the capability filter, and the
bounded validate/fix converge loop) -- confirmed via a real entry-point x
gate audit.
POST /assessments/{id}/deliver's dry_run=False path now calls
auto_validate_and_deliver() directly instead of re-implementing a subset
of its own gates -- one pipeline every real-delivery entry point shares,
so this class of gap cannot reopen in a new shape. dry_run=True stays a
lightweight route_and_deliver(dry_run=True) preview, unchanged. The
narrow octocat/Hello-World probe-repo hard-refuse stays in the route
(not a general delivery gate, doesn't belong inside a function that must
stay reusable for any real app).
Test updates reflect real, expected behavior differences now that manual
Deliver shares the automatic pipeline's own validate/fix loop (which can
leave its own dry-run validation-probe delivery rows behind, and surfaces
"could not converge"-shaped refusal reasons for batches that previously
only failed a raw file-count cap) -- not weakened assertions; every test
still proves no PR/commit happens when it shouldn't. Also fixed a fixture
gap surfaced by the new Phase C property check now actually running for
this path: test_pr_repo_labeling.py's NetworkPolicy stub was missing a
real Ingress policyType, which the auto path's own validation would
always have refused.
Full non-browser suite: 3397 passed, 452 skipped, 0 failed.
Co-authored-by: Cursor <cursoragent@cursor.com>
…ge token
Two real incidents reached merged content before being caught live:
an Argo Application repoURL of "your-org/hello-world.git" (gitops#32)
and a container image of "quay.io/org/pinky" -- neither is
REPLACE_WITH_AGENTIT_IMAGE, the only string
has_unresolved_placeholders() ever checked.
- delivery.py::has_unresolved_placeholders(): adds field-scoped regexes
for repoURL:/image: lines matching your-org/your_org, example.com, or
quay.io/org/ -- deliberately field-scoped, not a blanket content scan,
so a legitimate placeholder-shaped value elsewhere in a generated file
(skills/incident/alertmanager-config.md's own sample
"team@example.com" alerting address) is never mistaken for a
fabricated deploy target. Deliberately does NOT flag generic
"github.com/org/..." repo URLs -- this codebase's own test fixtures
already rely on that exact shape for a fine, real-looking repo, so a
broader match would have been a false-positive regression, not a fix.
- clear_evidence.py::verify_argocd_application(): the same "your-org"/
"example.com" repoURL patterns were previously explicitly *accepted*
("Template placeholder still ok" pass-through) whenever a path/chart
was also present -- now hard-refused outright, matching the general
placeholder gate.
Tests: tests/test_delivery_router.py (new placeholder-catch and
real-looking-URL-not-flagged cases), tests/test_clear_evidence.py (new
refuse-your-org, refuse-example.com, generic-org-still-allowed cases).
Full non-browser suite: 3401 passed, 452 skipped, 1 pre-existing flaky
failure unrelated to this change (test_finding_escalation.py, passes
standalone and within its own file -- known test-isolation flakiness).
Co-authored-by: Cursor <cursoragent@cursor.com>
…amespace) resources Confirmed root cause of a real incident: duplicate GVK/name resources across generated files blocked managed-pinky's Argo sync after merge -- nothing in this pipeline ever checked for this pre-open. SSA dry-run validates each file/resource independently; Argo's own "who owns this" ownership conflict only ever surfaced live, after merge. - quality_prs.find_resource_collisions(): parses every generated file's YAML (multi-doc `---` aware) and flags any exact (apiVersion, kind, namespace, name) repeat -- within one file or across the whole batch. Different API groups sharing a bare kind name (two CRDs both calling themselves "Policy") are correctly not a collision; only an exact GVK match is. - cluster_validation_ok() gains an optional collisions= param -- a collision is a hard Phase C refusal, same posture as an SSA/property failure, not a silent drop-one-keep-one guess. - Wired into auto_delivery.py's one cluster_validation_ok() call site -- since manual Deliver now shares this same pipeline, both entry points get this check for free, no separate wiring needed. Tests: tests/test_quality_prs.py (new TestResourceCollisions -- distinct resources, same-name-different-kind, same-name-different-namespace, cross-file and within-file collisions, different-API-group same-kind-name non-collision, cluster_validation_ok wiring). Full non-browser suite: 3410 passed, 452 skipped, 0 failed. Co-authored-by: Cursor <cursoragent@cursor.com>
…patible soft dry-run skips Both an RBAC Forbidden and a genuinely-missing optional CRD classify as "soft" (non-blocking, unchanged) -- but they are not the same kind of gap, and classify_dry_run_error() folded them into one flat string. A human reading a PR's dry-run notes could not tell "grant AgentIT's SA more rights and this would apply" from "this cluster does not run that operator at all, no RBAC change will ever make this sync" -- the real ambiguity behind a permanently-unsyncable resource reaching a merged GitOps PR (managed-pinky's real incident: missing Kyverno/Litmus/audit CRDs needed manual cleanup after merge). - classify_soft_dry_run_reason(): additive, does not change classify_dry_run_error()'s existing "hard"/"soft" contract (many callers/tests already assert on those exact two strings) -- further splits an already-soft message into capability_limited / cluster_ incompatible / other. - label_soft_dry_run_warning(): tags a dry-run warning string with which bucket it's in before it reaches skipped/warnings -- wired into both places dry_run_manifests_against_cluster() builds a soft-skip warning (the exception-catch path and the per-document errors path). Purely a labeling change: which files get soft-skipped (dropped from the delivery pack) is unchanged. Tests: tests/test_cluster_dry_run.py (new TestClassifySoftDryRunReason -- capability_limited/cluster_incompatible/other cases, label wording, original message preserved). Full non-browser suite: 3418 passed, 452 skipped, 0 failed. Co-authored-by: Cursor <cursoragent@cursor.com>
…ding pulse-agent#2, confirmed live: a PR targeting seven separate "container" findings (:latest across three Dockerfiles, missing HEALTHCHECK, non-UBI base) printed the identical "Clears \`container\` by pinning..." sentence seven times in a row. expected_clear_lines() looped once per target finding, but the emitted line's content depends only on the finding's category (via contract_for(cat)), never its own description -- every finding in the same category was always going to produce the exact same sentence. Now emits one line per distinct category (order of first appearance preserved), not one per finding. Purely a PR-body readability fix -- does not change which categories/files are targeted or any gate decision. Tests: tests/test_skill_registry_agreement.py (new TestExpectedClearLinesDeduped), tests/test_quality_prs.py (new build_helpful_pr_body regression case using pulse-agent#2's real 7 container findings). Full non-browser suite: 3423 passed, 452 skipped, 0 failed. Co-authored-by: Cursor <cursoragent@cursor.com>
Audited every "LLM" + "safety"/"gate" reference across live code, live
templates, and current (non-historical) docs for the class of
overstatement the underlying function's own docstring already warns
against: "Callers should therefore treat a 'not approved' result as
something to surface clearly... not as a reason to block PR creation."
Two real, live instances found and fixed; everything else already
correctly says "advisory"/"non-blocking"/"human gate remains merge":
- llm.py::review_final_manifests()'s own docstring opened with "Final
pre-PR quality/safety gate..." -- directly contradicting its own next
paragraph ("never a reason to block PR creation"). Reworded to
"quality/completeness opinion... advisory only, never a gate".
- templates/settings.html's "LLM Safety Gate" stat card is a stale
AutoMode-era label: AutoMode's own LLM classify_action (a genuine
blocking safety gate) was removed entirely (2026-07-18); the stat it
labels (llm_available) is just "is an LLM client configured" -- used by
several mostly-advisory features (secret classification, skill
generation, the final quality review, capability-scout's evidence
gate), none of which is "the" safety gate. Relabeled "LLM Availability".
Deliberately left unchanged (already accurate, not the class of bug
this audit was for): quality_prs.py's PR-body "non-blocking — human gate
remains merge" copy; cli.py's "LLM review — first approver gate" for
CLI self-fix's review_fix() specifically, which really is fail-closed
(rejects on low confidence / LLM unavailable) -- an accurate label for a
genuinely different, blocking function, not the portal's advisory
review_final_manifests(); docs/history/*.md's historical/superseded
proposal docs, which document what shipped or was proposed at the time,
not current behavior.
Tests: tests/test_portal.py updated for the corrected settings.html
label. Full non-browser suite: 3422 passed, 452 skipped, 1 pre-existing
flaky error unrelated to this change (test_finding_scoped_verification.py,
passes standalone -- known test-isolation flakiness).
Co-authored-by: Cursor <cursoragent@cursor.com>
…ive findings
Every existing test proves staged content / routing / refuse reasons in
isolation -- none of them run the real loop this product's own contract
depends on ("a good AgentIT PR clears a real finding on next re-Assess").
That gap is exactly why the #199-#204 class of bug (a verifier too weak
to notice a "fix" that would not actually clear the finding) was only
ever caught by live dogfooding, one incident at a time, not CI.
tests/test_e2e_finding_clears.py: real temp-directory "repo" + the real
analyzer + skill-generation + enrichment functions; only the GitHub API
boundary (read_file/tree_paths) is backed by that same temp directory
instead of a live network call, so the whole loop runs hermetically and
fast.
- TestReplicasFindingActuallyClearsAfterMerge: pulse-agent's real Helm-
templated replicas shape -- proves this session's workload-Helm-
awareness fix closes the loop end-to-end (generate -> enrich -> merge
-> re-Assess -> finding gone), not just that the enrichment function
returns the right dict in isolation.
- TestContainerFindingActuallyClearsAfterMerge: pulse-agent's real
Dockerfile :latest finding, proving the already-working containerfile-
pin mechanism closes the same loop.
- A genuine, real gap this test-writing effort surfaced and documents
rather than silently missing: HADRAnalyzer's replicas check is
repo-wide (any YAML file with replicas>=2 anywhere in the tree), not
scoped to the real deployed workload -- so even the OLD, un-enriched,
fully disconnected stub also "clears" the finding dishonestly. Flagged
as a known limitation (TestReplicasFindingActuallyClearsAfterMerge::
test_known_gap_the_disconnected_stub_alone_also_fools_the_analyzer),
not fixed in this pass -- the detection side has the same "is this the
real workload" blind spot the generation side just had.
sbom (enrich_sbom_from_repo) is a natural next fixture to add here, not
yet built out. Full non-browser suite: 3426 passed, 452 skipped, 0
failed.
Co-authored-by: Cursor <cursoragent@cursor.com>
Per this repo's own established convention (README stays a short pitch; dated writeups go to docs/history/changelog-dogfood-notes.md, which docs/release-notes.md's current-state summary points readers to) -- records this branch's 8 fixes (Helm-aware workload patches, manual- Deliver/auto-delivery gate parity, broadened placeholder detection, resource-collision check, soft dry-run reason labeling, PR-body dedup, LLM-review copy audit, real generate-deliver-reassess e2e test) and the live incident that prompted them. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Prompted by a live, reproducing incident:
pulse-agentwas stuck at score 66.63 for 6 re-Assess cycles over 4+ hours whilepinky/AgentITboth held 100. Caught PRpulse-agent#6fabricating the identicaldeploy/deployment.yamlstub#5was just closed for — generated by already-deployed#204code, ten minutes after#5's close. That live evidence, plus a full audit of the delivery pipeline, produced this fix list.What's in this PR (8 fixes, each its own commit)
workload-replicas/workload-health-probesdepended onanalyzers.snapshot's active-snapshotContextVar, only populated duringrun_assessment()'s own analyzer pass, always gone by the time a separate onboarding job calls these generators. Newenrich_workload_files_from_repo()reuses the GitHub-RESTread_file/tree_pathsmechanismapply_containerfile_pin_only()already relies on; patches a Helm chart'svalues.yamlinstead of rewriting a templatedreplicas:line.ha_dr.py's own detection was also Helm-blind — fixed too.POST /assessments/{id}/delivernow callsauto_validate_and_deliver()directly instead of a hand-maintained gate subset that had already drifted once (Hello-World gitops#31).your-org/hello-world.gitandquay.io/org/pinkyboth reached merged content before being caught;has_unresolved_placeholders()/verify_argocd_application()now catch this class.find_resource_collisions(), the confirmed root cause of duplicate GVK/name resources blockingmanaged-pinky's real Argo sync.expected_clear_lines()no longer repeats the same sentence once per finding in the same category (pulse-agent#2 printed it 7 times).review_final_manifests()'s docstring and a stale AutoMode-era "LLM Safety Gate" settings label no longer imply a blocking gate for an advisory-only, fail-open review.tests/test_e2e_finding_clears.pyruns generate → deliver → re-Assess for real (temp-dir repo, real analyzer/generation/enrichment code), forreplicas/container. Surfaced and documented (not fixed) a further gap:HADRAnalyzer'sreplicascheck is repo-wide, not workload-scoped.Testing
Full non-browser suite green after every commit; final state: 3426 passed, 452 skipped, 0 failed (one pre-existing, unrelated test-isolation flake seen intermittently during development, confirmed to pass standalone and within its own file each time).
Docs
docs/history/changelog-dogfood-notes.mdanddocs/release-notes.mdupdated with this pass's summary, per this repo's own documentation convention.Made with Cursor