fix(scan): close remaining check/remediation intent mismatches - #204
Merged
Conversation
Ship the audit After for SBOM→CI companions plus container harden, replicas vs PDB, health source probes, GHA-aware pipeline, tracing detect-only, honest policy text, and rbac analyzer emit. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
4 tasks
Co-authored-by: Cursor <cursoragent@cursor.com>
alimobrem
pushed a commit
that referenced
this pull request
Jul 23, 2026
…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>
alimobrem
added a commit
that referenced
this pull request
Jul 24, 2026
…manual/auto delivery parity, collision + placeholder gates (#205) * fix(remediation): Helm-aware replicas/health-probes patches, not deploy/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> * fix(portal): manual Deliver routes through auto_validate_and_deliver, 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> * fix(delivery): broaden placeholder detection beyond one hardcoded image 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> * fix(quality): refuse a batch that would create colliding (kind,name,namespace) 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> * fix(cluster-apply): distinguish capability-limited from cluster-incompatible 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> * fix(registry): dedupe expected_clear_lines() by category, not per finding 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> * fix(copy): stop describing review_final_manifests() as a safety gate 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> * test: real generate-deliver-reassess e2e proof, using pulse-agent's live 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> * docs: record PR-quality root-cause pass in changelog + release-notes 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> --------- Co-authored-by: Ali Mobrem <amobrem@example.com> 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.
Summary
docs/checks-remediations-intent-audit.md(audit doc included; supersedes/absorbs docs: checks & remediations intent audit #202 content)replicasvs PDB: analyzer emitsreplicas→workload-replicassource;availabilitystays PDB-onlyhealth: sourceworkload-health-probes+workload_probes; Kyverno companion refusedpipeline: detect skill sees GHA/GitLab/Jenkins/Tekton; non-Tekton →tekton_migrationdetect-onlytracing: detect-only (align instrumentation); otel-collector does not clearpolicy: honest labels-only clear_evidence textrbac: SecurityAnalyzer emits SA/Role/RoleBinding findingsecrets-scanning-in-ci→vulnerability-scanning-in-ciTest plan
tests/test_workload_patches.py,test_clear_evidence.py,test_skill_registry_agreement.py, phase4 detect parityscripts/ci-merge-gate.shMade with Cursor