fix(agentic-ci): improve dependency PR quality

[andreatgretel]

Summary

• build a deterministic AST-based import and dependency inventory for the dependency audit
• classify transitively guaranteed declaration gaps as low-severity hygiene and batch up to three gaps per target package
• require dependency PRs to include a regenerated, clean uv.lock

Validation

• .venv/bin/ruff check --fix .
• .venv/bin/ruff format .
• .venv/bin/pytest packages/data-designer/tests (991 passed, 1 skipped)
• .venv/bin/python scripts/update_license_headers.py --check
• uv lock --check
• workflow YAML parse and git diff --check

[greptile-apps]

Greptile Summary

This PR improves the agentic dependency-audit pipeline on three axes: a deterministic AST-based inventory script replaces ad-hoc grep commands, the scope/lockfile workflow gates are refactored to operate on PR/branch groups rather than individual findings, and dependency PRs are now required to include a regenerated uv.lock.

• scripts/audit_package_dependencies.py — new script that walks Python ASTs, resolves module→distribution mappings (with an explicit override table for packages like pyyaml/pillow), computes transitive-dependency closures, and classifies each gap as low (guaranteed by a workspace sibling) or high (no installer guarantee).
• scripts/manage_agentic_ci_attempts.py — extracts the inline Python here-docs from the workflow into a proper script with atomic state writes; groups batched attempted_fixes entries by (pr_number, branch) and marks all members of a rejected group abandoned together.
• Workflow — adds an inventory build step gated to the dependencies suite, extends the allowlist/LOC exclusion to uv.lock, and adds two new lockfile checks: presence (uv.lock must appear in the diff) and cleanliness (uv.lock must not change after make install-dev).

Confidence Score: 5/5

Safe to merge — the changes are additive scripts with atomic state writes, well-tested helper logic, and conservative workflow gate ordering.

Both new Python scripts are covered by targeted unit tests. The audit script's core logic (closure BFS, marker evaluation, override table) is exercised end-to-end. The workflow changes follow existing patterns and the two new lockfile checks are guarded so that an already-abandoned group is invisible to the lockfile gate. No existing functionality is removed without a replacement.

No files require special attention.

Important Files Changed

Filename	Overview
scripts/audit_package_dependencies.py	New deterministic AST-based dependency inventory script; uses packaging.requirements for marker-aware dep parsing, frozenset-keyed BFS for transitive closure, and correct override table for ambiguous module→distribution mappings
scripts/manage_agentic_ci_attempts.py	New script for grouping and abandoning attempted-fix entries by PR/branch; uses atomic rename write and validates marker values against an allowlist
.github/workflows/agentic-ci-daily.yml	Adds inventory build step, rewrites scope and lockfile gates to operate on PR/branch groups instead of individual entries, and adds two new uv.lock presence/cleanliness checks
packages/data-designer/tests/test_dependency_audit.py	New tests covering low/high severity classification, inactive marker handling, workspace-transitive guarantee, extras-conditional guarantee, ambiguous module routing, and module override table
packages/data-designer/tests/test_agentic_ci_attempts.py	New tests for grouping batched attempts by PR/branch and for batch-abandoning all finding entries in a rejected group
.agents/recipes/dependencies/recipe.md	Replaces ad-hoc grep instructions with structured inventory-based workflow; requires uv.lock regeneration in dependency PRs and clarifies severity classification
.agents/recipes/_fix-policy.md	Policy updates to exclude uv.lock LOC from scope limits, refine transitive-gap severity definitions, and tighten batch-grouping key to target package

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant W as Workflow
    participant Inv as audit_package_dependencies.py
    participant Agent as Claude Agent
    participant Mgr as manage_agentic_ci_attempts.py
    participant State as runner-state.json

    W->>Inv: Build dependency inventory (dependencies suite only)
    Inv-->>W: /tmp/dependency-inventory.json (AST-derived gaps + severities)

    W->>Agent: Audit phase — read inventory, classify gaps, populate fix_backlog
    W->>Agent: Fix phase — batch up to 3 transitive-gap entries per target package, push PR with uv.lock

    Agent->>State: "Write attempted_fixes entries (pr_number, branch, outcome=open)"

    Note over W: Scope Gate
    W->>Mgr: groups --state --prior
    Mgr->>State: Read open entries grown since snapshot
    Mgr-->>W: OPEN_GROUPS (entries grouped by pr_number/branch)
    loop Each GROUP
        W->>W: Check path allowlist and LOC delta (uv.lock excluded from LOC)
        alt Scope violation
            W->>W: gh pr close + delete branch
            W->>Mgr: abandon --marker gate_violation
            Mgr->>State: Mark all finding_ids abandoned
        end
    end

    Note over W: Lockfile Gate (dependencies suite, scope gate passed)
    W->>Mgr: groups --state --prior
    Mgr-->>W: OPEN_GROUPS (already-abandoned entries filtered out)
    loop Each GROUP
        W->>W: git checkout branch
        alt uv.lock absent from diff
            W->>W: gh pr close
            W->>Mgr: abandon --marker lockfile_missing
        end
        W->>W: make install-dev
        alt uv.lock dirty after install
            W->>W: gh pr close
            W->>Mgr: abandon --marker lockfile_not_committed
        end
        alt install-dev failed
            W->>W: gh pr close
            W->>Mgr: abandon --marker lockfile_verification_failed
        end
        W-->>W: Lockfile clean — PR proceeds
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant W as Workflow
    participant Inv as audit_package_dependencies.py
    participant Agent as Claude Agent
    participant Mgr as manage_agentic_ci_attempts.py
    participant State as runner-state.json

    W->>Inv: Build dependency inventory (dependencies suite only)
    Inv-->>W: /tmp/dependency-inventory.json (AST-derived gaps + severities)

    W->>Agent: Audit phase — read inventory, classify gaps, populate fix_backlog
    W->>Agent: Fix phase — batch up to 3 transitive-gap entries per target package, push PR with uv.lock

    Agent->>State: "Write attempted_fixes entries (pr_number, branch, outcome=open)"

    Note over W: Scope Gate
    W->>Mgr: groups --state --prior
    Mgr->>State: Read open entries grown since snapshot
    Mgr-->>W: OPEN_GROUPS (entries grouped by pr_number/branch)
    loop Each GROUP
        W->>W: Check path allowlist and LOC delta (uv.lock excluded from LOC)
        alt Scope violation
            W->>W: gh pr close + delete branch
            W->>Mgr: abandon --marker gate_violation
            Mgr->>State: Mark all finding_ids abandoned
        end
    end

    Note over W: Lockfile Gate (dependencies suite, scope gate passed)
    W->>Mgr: groups --state --prior
    Mgr-->>W: OPEN_GROUPS (already-abandoned entries filtered out)
    loop Each GROUP
        W->>W: git checkout branch
        alt uv.lock absent from diff
            W->>W: gh pr close
            W->>Mgr: abandon --marker lockfile_missing
        end
        W->>W: make install-dev
        alt uv.lock dirty after install
            W->>W: gh pr close
            W->>Mgr: abandon --marker lockfile_not_committed
        end
        alt install-dev failed
            W->>W: gh pr close
            W->>Mgr: abandon --marker lockfile_verification_failed
        end
        W-->>W: Lockfile clean — PR proceeds
    end

Loading

_{Reviews (4): Last reviewed commit: "Merge branch 'main' into andreatgretel/f..." | Re-trigger Greptile}

[github-actions]

Code Review: PR #787 — fix(agentic-ci): improve dependency PR quality

Summary

This PR reworks the agentic-CI dependencies suite to make its dependency audit deterministic and its fix PRs safer:

• New script scripts/audit_package_dependencies.py (227 lines): builds an AST-based import/dependency inventory. For each workspace package it compares imported third-party modules against static + dynamic (uv-dynamic-versioning) declarations, resolves modules to distributions, and classifies missing declarations by severity.
• Severity model shift: a gap is low when the imported distribution is transitively guaranteed by a mandatory declared dependency (guaranteed_by), and high only when nothing guarantees it (true standalone-install breakage).
• New tests test_dependency_audit.py (91 lines) exercising the three severity paths (transitively-guaranteed low, uncovered high, external-transitive low).
• Recipe/policy (dependencies/recipe.md, _fix-policy.md): consume the inventory JSON instead of ad-hoc grep; batch up to 3 gaps per target package; require dependency PRs to commit a regenerated, clean uv.lock.
• Workflow (agentic-ci-daily.yml): builds the inventory before the run, widens the dependencies allowlist to include uv.lock, and adds a lockfile gate that rejects PRs missing uv.lock or where make install-dev mutates it.

Change is well-scoped and matches the stated validation (991 passed, ruff clean, uv lock --check).

Findings

Correctness — script looks sound

• The guaranteed_by closure logic is correct: installed_requirements filters environment markers via evaluate({"extra": ""}), so only mandatory transitive requirements count toward the "guaranteed" (low-severity) classification — extras/optional deps correctly do not downgrade severity. This is the right conservative direction (unknown → high → more scrutiny).
• Workspace siblings are handled by short-circuiting requirements_for to the sibling's own declared set, so config→engine jinja2 correctly resolves to guaranteed_by: [data-designer-config]. The test confirms this.
• Relative imports are excluded (node.level == 0), stdlib is filtered via sys.stdlib_module_names, and data_designer is skipped — all correct for a third-party inventory.
• dependency_closure is an iterative DFS with a closure visited-set, so cyclic dependency graphs terminate safely.

Nit (low) — shadowing of the imported distribution

• audit_repository rebinds the name distribution as a loop variable in two places (distribution = resolve_distribution(...) and for distribution, usage in sorted(...)), shadowing the module-level from importlib.metadata import ... distribution. It's not a bug — distribution() is only called inside installed_requirements, which has its own scope — but it hurts readability and is the kind of thing a future edit could trip over. Consider renaming the locals to dist/distribution_name.

Determinism caveat (informational, not a defect)

• The default path uses packages_distributions() and installed_requirements(), which reflect the installed environment, and marker evaluation uses the running platform. A dependency required only on another platform could be counted as guaranteed on some runners and not others. This is conservative (worst case = flagged high) and acceptable, but worth a one-line comment in the script noting the inventory is environment-relative.

unresolved_modules / resolve_distribution ambiguity

• resolve_distribution returns None (→ unresolved_modules) when a module maps to multiple candidate distributions none of which are declared. Good conservative behavior, and the recipe correctly routes unresolved_modules to manual review ("Never guess a package name"). No direct unit test covers the multi-candidate/unresolved branch or the MODULE_DISTRIBUTION_OVERRIDES path — worth adding for completeness given the audit's authority over auto-generated PRs, but not blocking.

Workflow gate — logic is correct

• The uv.lock-present check (git diff --name-only origin/main...HEAD -- uv.lock | grep -qx 'uv.lock') and the post-install-dev cleanliness check (git diff --quiet -- uv.lock) together enforce the "committed, resolves-clean" contract. Both rejection branches call abandon_open_attempt and close the PR/branch consistently with the surrounding error handling. Note grep -qx 'uv.lock' treats . as a regex wildcard, but since git only ever emits the literal path this is harmless.

Docs/recipe consistency

• _fix-policy.md batching now keys on "target package, test target, and branch type" and the recipe/allowlist/constraints all consistently add uv.lock. The severity table, report template, and eligibility notes are internally consistent with the new guaranteed_by/severity fields emitted by the script. No drift spotted.

Test coverage

• The three included tests map cleanly onto the three severity outcomes and use injected distributions/requirements maps to stay hermetic (no reliance on the installed env) — good design. Gaps: no test for unresolved_modules, the override map, or the unused-dependency reporting path.

Structural Impact

Structural Impact (graphify, 2.3s)

Risk: LOW (localized change)

• 2 Python files, 0 AST entities, 0/81 clusters

Verdict

Approve (non-blocking nits). The new script is correct, hermetically testable, and the severity model is a genuine improvement over the prior grep-based transitive check. The workflow lockfile gate closes a real failure mode (deps that pass the old lockfile but don't resolve standalone). None of the findings block merge:

• Rename the shadowed distribution locals for readability.
• Consider a short comment noting the inventory is environment-relative.
• Optionally add tests for unresolved_modules and the override path.

All changes are confined to .agents/, .github/workflows/, scripts/, and test files — no impact on the shipped data_designer packages or the interface→engine→config import direction.

[nabinchha]

Thanks for putting this together, @andreatgretel!

Summary

This PR replaces the dependency suite's grep-based discovery with a structured AST/import-metadata inventory, distinguishes guaranteed declaration gaps from uncovered ones, batches compatible fixes, and requires dependency PRs to carry a clean regenerated lockfile. The implementation matches the stated intent, but two edge cases in the new classification and batching behavior are worth fixing before merge.

Findings

Warnings — Worth addressing

scripts/audit_package_dependencies.py:42 — Declaration parsing drops markers and selected extras

• What: declared_dependencies() reduces every requirement to only its normalized distribution name. That discards project-level environment markers and selected extras before dependency_closure() runs; installed_requirements() then evaluates all transitive markers with extra="".
• Why: This can classify gaps in both directions incorrectly. For example, a package declaring pydantic[email] is guaranteed to install email-validator, but an email_validator import would be reported as uncovered/high. Conversely, an inactive conditional declaration such as foo; python_version < "3.11" would still be treated as guaranteed on newer Python. That undermines the low/high distinction this PR is introducing.
• Suggestion: Preserve each declaration's marker and selected extras (using packaging.Requirement for ordinary requirements, with the existing dynamic-version template handled separately), evaluate project markers for the runner environment, and evaluate transitive requirements once for the base dependency plus each explicitly selected extra. Please add focused tests for an enabled extra and an inactive project marker.

.agents/recipes/dependencies/recipe.md:181 — Batched PRs are still validated once per finding

• What: The recipe now puts up to three findings in one PR and records one attempted_fixes entry per dependency, but both post-fix gates still iterate those entries independently. In particular, the lockfile gate runs make install-dev once per finding even when all entries point to the same branch/PR, and each rejection updates only the current finding ID.
• Why: A three-finding batch can run the same expensive validation three times. More importantly, if an early repetition passes and a later repetition fails transiently, the shared PR is closed while the earlier finding IDs remain recorded as open until a future reconciliation run, violating the policy's atomic-state guarantee.
• Suggestion: Group new entries by (pr_number, branch), validate each pushed diff once, and apply the result atomically to every finding ID in that group. The scope gate can use the same grouping to avoid redundant fetch/diff work; a small fixture covering a multi-finding rejection would lock the state transition down.

What Looks Good

• The AST walk, stable sorting, conservative unresolved-module handling, and injected metadata maps make the inventory predictable and testable.
• The three tests cleanly cover workspace-transitive, external-transitive, and genuinely uncovered severity paths without depending on the host environment.
• The recipe, allowlist, and workflow agree on committing uv.lock, and the presence-plus-cleanliness checks close the stale-lockfile failure mode.

Verdict

Needs changes — preserve requirement markers/extras in closure classification, and make batched PR validation/state updates operate once per shared PR rather than once per finding.

---

This review was generated by an AI assistant.

[andreatgretel]

Addressed the review feedback:

• Dependency declarations now preserve and evaluate environment markers and selected extras when computing transitive guarantees. Dynamic {{ version }} dependencies remain supported.
• Post-fix scope and lockfile gates now group attempts by (pr_number, branch), validate each shared diff once, and atomically abandon every associated finding on rejection.
• Renamed the shadowing distribution locals.
• Added focused tests for enabled extras, inactive project markers, batched grouping, and multi-finding rejection.

[nabinchha]

Circling back on this one, @andreatgretel — took another pass now that it's in shape. Overall it's holding up well: the AST inventory reads cleanly, ruff is happy, the three new tests pass, and the workflow YAML parses. A couple of things are still worth a look before merge, but nothing that changes the shape of the PR.

Still worth addressing

uv.lock counts against the 50-LOC net cap (_fix-policy.md:14, agentic-ci-daily.yml:372)
The suite now requires uv.lock in every dependency PR (the new lockfile gate closes any that omit it), yet the localized-fix bar is still "≤50 LOC net" and the scope gate's LOC_DELTA is computed across the whole diff — uv.lock included. For the eligible case (a gap already guaranteed_by a required sibling, so the dist is resolved and locked) the lock delta is usually a line or two and this survives. But batching up to 3 deps, or uv re-serializing a block, could tip the combined diff past 50 lines and get the PR silently closed — leaving the two gates mutually unsatisfiable. Could we confirm a realistic 3-dep regeneration stays under 50 net lines, or just exclude uv.lock from LOC_DELTA (... -- ':(exclude)uv.lock') and lean on the file-count/allowlist checks?

guaranteed_by/severity depend on the runner's environment (audit_package_dependencies.py:97)
installed_requirements evaluates markers with only {"extra": ""}, so sys_platform/python_version markers resolve against the current runner. A platform-conditional transitive dep could downgrade a genuinely-missing dependency from high to low on CI. Not blocking given the recipe re-confirms severity, but a one-line comment noting marker evaluation is environment-relative would keep future readers from over-trusting guaranteed_by.

Minor / optional

• audit_package_dependencies.py:167,180 — the local distribution shadows the imported importlib.metadata.distribution. Harmless today (only called from installed_requirements), but a trap for a future edit; dist / distribution_name would be safer.
• test_dependency_audit.py — the severity paths are well covered; the unresolved_modules, MODULE_DISTRIBUTION_OVERRIDES, and ambiguous multi-candidate branches of resolve_distribution aren't yet. A couple of small cases there would close the gap.
• audit_package_dependencies.py:62 — imported_modules counts TYPE_CHECKING/guarded imports as runtime imports (intentional, since the recipe re-verifies). A brief comment saying so would save the next reader a double-take.

Still looking good

• The deterministic AST + metadata inventory is a genuine upgrade over the old grep, and driving the low/high split off a transitive closure cleanly separates hygiene from real standalone-install breaks.
• Hermetic tests via injected module_distributions/requirement_map — no dependency on the installed env.
• The lockfile gate reuses the scope gate's snapshot-based "grew this run" selector and fails closed on checkout/fetch failures, consistent with the atomicity model.
• Spec and enforcement stay in sync — uv.lock added to both the _fix-policy.md allowlist and the workflow regex.

Verdict

Needs changes — no correctness issues, but the uv.lock-vs-50-LOC interaction is worth confirming or resolving before this goes live, since it can quietly make dependency fixes unlandable. Everything else is take-it-or-leave-it.

---

This review was generated by an AI assistant.

[andreatgretel]

Addressed the follow-up feedback:

• Excluded generated uv.lock changes from the dependency-suite LOC cap while retaining file-count and allowlist enforcement.
• Documented that requirement markers are evaluated relative to the audit environment.
• Added coverage for ambiguous module mappings and distribution overrides.
• Documented the intentional inclusion of guarded and TYPE_CHECKING imports.

[nabinchha]

Re-reviewed after c70f2006 (fix: address dependency audit review) — thanks for the thorough turnaround, @andreatgretel. Both earlier concerns are resolved and the extra refactoring is a genuine improvement.

What got addressed

• uv.lock vs the 50-LOC cap — the scope gate now excludes the lockfile from LOC_DELTA via git diff --shortstat ... -- ':(exclude)uv.lock' (dependencies suite only), and _fix-policy.md documents that uv.lock still counts toward the 3-file cap and the allowlist but not the LOC total. That's exactly the split I was hoping for — the two gates are no longer mutually unsatisfiable.
• Environment-relative marker evaluation — now called out explicitly with a comment, and marker handling was upgraded to propagate selected extras through the closure (requirement_dependencies + the selected_extras threading), so pydantic[email] correctly guarantees email-validator. Nice — that's better than the one-liner I suggested.
• distribution shadowing — renamed to distribution_name throughout audit_repository.
• Test coverage — the previously-untested branches now have cases: selected-extra guarantee, inactive project marker, ambiguous module → unresolved_modules, and the MODULE_DISTRIBUTION_OVERRIDES path.
• TYPE_CHECKING/guarded imports — the intentional over-report is now documented inline on imported_modules.

Bonus: extracting the inline gate Python into scripts/manage_agentic_ci_attempts.py (with groups/abandon subcommands + tests) and grouping attempts by (pr_number, branch) so a batched PR is validated once is a real readability and correctness win over the duplicated heredocs.

Verified locally: ruff check + ruff format --check clean on all four changed Python files, the workflow YAML parses, and the audit/attempts test files pass (9 passed).

Minor / optional

• scripts/audit_package_dependencies.py:58 — dependencies[name] as a bare statement relies on the defaultdict side effect to seed an empty-extras entry for a {{ version }} template dep. It works, but dependencies.setdefault(name, set()) would make the intent obvious to the next reader (and won't trip anyone reaching for a "useless expression" cleanup).

Verdict

Ship it (with nits) — everything from the prior review is handled, the new helper is well-tested, and only the one cosmetic setdefault nit remains, entirely take-it-or-leave-it.

---

This review was generated by an AI assistant.