Skip to content

fix(ci): submit matching master dependency baselines - #3326

Open
imbajin wants to merge 14 commits into
apache:masterfrom
hugegraph:fix/ci-dependency-baseline
Open

imbajin wants to merge 14 commits into
apache:masterfrom
hugegraph:fix/ci-dependency-baseline

Conversation

@imbajin

@imbajin imbajin commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

Purpose of the PR

Provide matching master dependency snapshots so PR dependency review can compare resolved Maven dependencies against a real baseline. Reduce the extra CI queue pressure by sharing existing runners and reactor builds.

Main Changes

  • Generate the baseline in the existing read-only dependency job, sharing one Maven reactor install and cache with dependency inventory checks. Fixture validation runs in preflight; PR dependency review becomes a step in the dependency job.
  • Run Helm checks in preflight, Struct tests and contracts in Store (with a standalone fallback when Store is not selected), and RocksDB compatibility in one selected PD/Store/HStore runner. Preserve the existing test selections, vulnerability threshold, failure checks and Javadoc generation for inventory builds. Increase shared job budgets to leave room for the moved checks.
  • Keep submission in a separate checkout-free writer. It verifies the trusted source, successful producer, run attempt, artifact digests and per-POM package/scope/relationship equality against an independent Maven inventory. Maven receives no submission token.
  • Cancel superseded automatic master runs, while keeping manual, weekly and release/test branch runs independent. Add a dependencies manual preset for focused validation and baseline recovery.

Representative runner allocations compared with the previous PR design:

Scenario Before After
Mainline PR 16 12
Store PR 7 5
Helm PR 2 1
Master push changing a POM, including baseline jobs 8 5
Documentation-only trusted master push, including baseline jobs 4 3

These counts come from workflow routing with one project JDK, not measured runner minutes or queue times. A trusted master push still needs one complete reactor install for its exact-commit baseline; the change removes duplicate builds and unnecessary runner allocations. Weekly runs do not add a baseline submission.

flowchart LR
    P[Preflight: selection, fixtures, Helm] --> R[Shared read-only dependency runner]
    R --> C[Existing inventory checks and PR review]
    R --> F[Ferstl snapshot]
    R --> D[Independent Maven inventory]
    F --> W[Isolated checkout-free writer]
    D --> W
    W --> S[Verified master snapshot]
Loading

Verifying these changes

  • Python regression and workflow-routing checks, shell contracts, workflow validation, and Java 17 Struct tests pass.
  • Java 17 formatting and clean reactor compilation pass.

Moved checks still run after ordinary host-job failures. They now share host timeouts and execution environments; increased budgets provide headroom, but a timeout or cancellation can stop later checks. Real GitHub runs are still needed to validate the combined execution and queue effect.

The first trusted ASF and org master submissions and a real dependency-review comparison remain acceptance gates. Historical SHA backfill remains deferred: a superseded base commit needs a new PR event based on a successfully submitted master commit. Native alias handling still awaits an unmodified exporter reproduction. Existing security and Codecov checks retain their own results.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects: CI job layout and automatic master-run cancellation
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done: updated docs/ci.md with runner sharing, budgets, manual validation and baseline recovery.
  • Doc - No Need

Visual overview

Conceptual overview of PR #3326

Conceptual overview of the proposed dependency-baseline pipeline: a shared read-only Maven job produces a snapshot and an independent inventory; an isolated checkout-free writer validates source, run, artifact and per-POM agreement before submission. Maven receives no submission token. Trusted master submissions and a real dependency-review comparison still need validation.

@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.69%. Comparing base (92af2f8) to head (c5677d5).
⚠️ Report is 31 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3326      +/-   ##
============================================
+ Coverage     54.31%   54.69%   +0.38%     
- Complexity     2250     2278      +28     
============================================
  Files           770      774       +4     
  Lines         67952    68532     +580     
  Branches       9213     9506     +293     
============================================
+ Hits          36905    37482     +577     
- Misses        27470    27543      +73     
+ Partials       3577     3507      -70     
Flag Coverage Δ
commons 96.37% <ø> (?)
hstore 35.88% <ø> (?)
pd 49.27% <ø> (?)
server_memory 54.31% <ø> (?)
server_rocksdb 42.37% <ø> (?)
store 31.93% <ø> (?)

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The head, base and complete two-file patch are identical to fork #300. Independent cross-check confirms the existing source-review evidence applies: 8/10 for the original full change at 189ca23, and 9/10 for the a35c43e follow-up delta. These are separate scopes, not a new full-PR 9/10. The writer remains limited to two named master repositories and submits only into its own repository. No new source blocker found. Verify the first continuous Java 17 master submission, manifest completeness and real dependency-review comparison independently in each repository; fork CI and manual snapshot claims do not replace those gates.

@bitflicker64 bitflicker64 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.

Blocking: yes. Summary: The submit job depends on an action that the ASF allow list does not include, so the workflow would fail at startup in apache/hugegraph and the ASF baseline would not be produced. The rest of the workflow checks out: the three pinned SHAs match their tags, every input exists in the pinned action, and the validate job's tests pass at this head. Evidence: gh api -X GET repos/apache/infrastructure-actions/contents/approved_patterns.yml and actions.yml grepped for advanced-security and maven-dependency; gh api -X GET repos/<action>/git/ref/tags/<tag> for the three pins; action.yml of the submission action at a64327a7; python3 -m unittest discover -s .github/scripts -p 'test_ci_policy.py' and -s install-dist/scripts/dependency -p 'test_*.py' at a35c43e (both OK).

Comment thread .github/workflows/submit-dependencies.yml Outdated

@bitflicker64 bitflicker64 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.

Blocking: no. Summary: the read-only generator and the checkout-free writer hold up against the code at this head, and the serializer matches the pinned upstream parser on the points I compared; the one finding is that a partial re-run of submit can never pass, which the docs do not say. The workflow has not run end to end in either repository, so runner and API behaviour (artifact download with the job token, snapshot acceptance, dependency-review comparison) is still unverified. Evidence: read the full diff at 02ad423 (4 files, +817); python3 -m unittest discover for test_ci_policy.py, test_dependency_snapshot.py (9 tests) and install-dist/scripts/dependency (7 tests), all OK; gh api -X GET repos/actions/<name>/git/ref/tags/<tag> confirms the checkout v7.0.1, setup-java v6.0.1 and upload-artifact v4.6.2 pins; ran depgraph-maven-plugin:4.0.3:graph over the 38-module reactor locally and dependency-snapshot.py on the result (38 manifests, 5054 resolved entries, 1.7 MB), then applied the writer's manifest, package URL and direct-dependency checks to it with no rejection; compared manifest() with src/depgraph.ts at actions/maven-dependency-submission-action 1f666f7a.

Comment thread .github/workflows/submit-dependencies.yml Outdated

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Blocking: no. Summary: The new graph test does not exercise an indirect-only dependency, so it would not catch an all-direct relationship regression. Evidence: the fixture at .github/scripts/test_dependency_snapshot.py:175 makes every package direct.

Comment thread .github/scripts/test_dependency_snapshot.py
Pengzna
Pengzna previously approved these changes Oct 13, 2026
- submit canonical master snapshots in a separate job
- limit write permission to trusted push and dispatch
- pin privileged actions and verify the event revision
- document repository scope and baseline acceptance
- allow the designated org master producer
- keep fork and pull request events read-only
- document isolated baseline acceptance checks
- use the pinned GitHub-maintained upstream fork
- preserve master-only writes and freshness guards
- document the permitted publisher and source reference
- build and serialize Maven graphs in a read-only job
- validate a bounded JSON ZIP in a fresh data-only writer
- preserve dependency identity and master freshness
- document boundaries and add archive redirect regressions
- document all-job reruns or fresh master dispatch
- test dependency paths containing only indirect edges
- verify direct edges take precedence over indirect paths
- select dependency checks for producer and workflow inputs
- keep fixture tests in the existing entry job
- verify mixed changes retain runtime coverage

@bitflicker64 bitflicker64 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.

Blocking: yes. Summary: The read-only generator, the checkout-free writer and the serializer hold up against the code at this head, and both earlier review threads (re-run recovery, indirect-only fixture) are addressed. One gap remains: a master commit that stops being the tip before its run finishes never gets a baseline and cannot be backfilled, so PRs based on it still fail dependency review. Evidence: full diff at 2fa78c1 (6 files, +860/-1); python3 -m unittest discover -s .github/scripts for test_ci_policy.py (22 tests) and test_dependency_snapshot.py (11 tests), both OK; manifest() compared with src/depgraph.ts and src/snapshot-generator.ts at actions/maven-dependency-submission-action 1f666f7a; checkout, setup-java and upload-artifact pins match their tags; gh api -X GET repos/apache/hugegraph/dependency-graph/compare/92af2f89...2fa78c13 (5058 additions, per-module POM manifests); master commit times and open-PR base SHAs from the GitHub API; the dependency-review log of run 38464100215. The workflow has not run in either repository, so artifact download, snapshot acceptance and the base/head comparison are unverified.

EXPECTED_SHA: ${{ github.sha }}
run: |
test "$(git rev-parse HEAD)" = "$EXPECTED_SHA"
test "$(git ls-remote origin refs/heads/master | cut -f1)" = "$EXPECTED_SHA"

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.

Important: a master commit that is no longer the branch tip when this check runs never gets a baseline, and nothing can add one later, so pull requests based on that commit keep failing dependency review.

Evidence: this step, the repeat at line 100 and the writer's Stale master before submission check (line 307) all fail the run when refs/heads/master is not github.sha. workflow_dispatch only runs for the current tip, and the concurrency group (line 28) keeps one pending run, so a superseded or replaced commit has no recovery path. Master moves faster than one run: #3337 and #3335 merged 46 seconds apart (20:01:23Z and 20:02:09Z on 2026-10-13), four commits landed between 08:56:45Z and 08:58:58Z on 2026-10-12, and mvn install alone took 3m48s in this PR's dependency-check job. dependency-review compares exact SHAs: on this head it logged "snapshots compared for the base SHA (0) and the head SHA (1) do not match", listed every dependency as added (5058 entries from the compare API) and failed on vulnerable packages. Open PRs sit on such commits: #3334 has base c1e7aba77, which was the tip for under 8 minutes before b84454fb0.

Requested change: submit the snapshot for the event SHA even after master advances. The run, artifact, SHA and ref identity checks already bind it to that commit, and the concurrency group already serializes submissions. If the tip-only rule has to stay, add a way to backfill a named master SHA and state in docs/ci.md that a PR whose base commit was skipped has no baseline until it is updated onto a submitted commit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This remains a known limitation of the current-tip producer. Superseded event SHAs are not backfilled by this PR. The updated docs/ci.md provides recovery by waiting for a successful current-master baseline, updating/pushing the PR branch to create a new event, and checking that event's exact base SHA. It does not repair the old event.

Named historical-SHA submission remains deferred pending confirmation of the submission semantics. This is an explicit scope deferral, not a completed fix; this discussion remains open. The separate current-tip documentation alternative is addressed in #3326 (comment).

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The writer accepts schema-valid but incomplete dependency graphs from the Maven producer, allowing dependencies to be omitted from the submitted baseline. Evidence: exact-head static review of .github/workflows/submit-dependencies.yml:89-106, 294-306; run 38464100215 logged zero base snapshots versus one head snapshot.

Comment thread .github/workflows/submit-dependencies.yml
- integrate current build and CI policy changes
- preserve dependency snapshot submission safeguards
- retain existing inventory validation coverage
- sync the dependency candidate with current master
- preserve caller transaction and collection fixes
- include selective coverage and snapshot test fixes
- resolve an independent per-module Maven inventory
- reject incomplete snapshots before token-bearing submission
- cover inventory omissions and document acceptance gates

@bitflicker64 bitflicker64 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.

Blocking: no. Score: 8/10 (10 - 2 for the open tip-only baseline finding).

Summary: the new inventory job closes the completeness gap. On the real 38-POM reactor the Ferstl snapshot and the dependency:tree inventory agree exactly, and dropping a package is rejected before the POST.

Evidence: read the PR-scoped delta 2fa78c1..3974147 (dependency-inventory.py, submit-dependencies.yml, test_dependency_snapshot.py, ci-policy.py, test_ci_policy.py, docs/ci.md). Built the reactor with depgraph-maven-plugin:4.0.3:graph and maven-dependency-plugin:3.8.1:tree -DoutputType=json at this head, ran both outputs through create_snapshot() and create_inventory(), then applied the writer's per-manifest equality (line 390). Result: 38 manifests, 5054 packages, 0 mismatches. That includes classifier packages (netty natives, store exec) and 112 test-scope entries. No POM uses an external SNAPSHOT or a version range, so the two runners cannot resolve different versions. test_dependency_snapshot.py (17), test_ci_policy.py (21) and install-dist/scripts/dependency (7) pass. dependency-review still fails here with 0 base snapshots; this workflow should fix that once it runs on master.

Status of earlier threads:

  • @imbajin, incomplete producer graph: fixed in 3974147 (exact package URL, scope and relationship equality per POM, line 390).
  • My earlier point, a master commit that stops being the tip never gets a baseline: still open, and the new inventory job adds the same tip check at lines 144 and 157. I do not treat it as blocking. The PR body defers SHA backfill. A skipped commit gives the same failing dependency-review as today, not a wrong baseline. Also, the concurrency group already drops superseded pending runs, so only a backfill would close it. The thread still needs a reply, and docs/ci.md should say that a PR based on a skipped master commit has no baseline until it is updated onto a submitted one.
  • Earlier rounds (ASF allow list, re-run guidance, indirect-only fixture): still fixed.

Not verified: the first master run in either repository (artifact download, snapshot acceptance, a real base/head comparison), which the PR body lists as post-merge acceptance.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The inventory path still rejects valid mixed test/runtime dependency scopes, so baseline submission is not reliable until scope precedence matches. Evidence: static inspection of .github/scripts/dependency-snapshot.py:92, .github/scripts/dependency-inventory.py:62-69, and .github/workflows/submit-dependencies.yml:388-390.

Comment thread .github/scripts/dependency-snapshot.py Outdated
- classify mixed test and non-test scopes as runtime
- verify scope parity through the existing inventory writer
- document precedence without changing submission flow

@bitflicker64 bitflicker64 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.

Blocking: yes. Summary: At a75e16b the inventory gate, the mixed-scope fix and the checkout-free writer hold up, and I found no new correctness problem in dependency-snapshot.py, dependency-inventory.py or the writer. The line 88 thread (a master commit that stops being the tip before its run finishes never gets a baseline) has no reply yet and still applies at this head. I count it as blocking because a PR whose base is such a commit keeps failing dependency review and nothing can add that baseline later. One new Minor point: a PR pushed while the current tip's baseline run is still going also fails dependency review. A re-run fixes it, but docs/ci.md does not say so. Evidence: read the full diff at a75e16b (7 files, +1210/-1); python3 -m unittest discover -s .github/scripts with test_dependency_snapshot.py (18 tests) and test_ci_policy.py (21 tests), both OK; read Ferstl 4.0.3 DependencyGraphMojo.createGraphBuilder() and DependencyNodeIdRenderer (node ids are groupId:artifactId:type[:classifier], so two Ferstl ids never share one package URL, and the last-wins scopes[purl] for aliases cannot disagree with the inventory in practice); the dependency-review log of job 116115771998 (base 4b77b7b, 0 base vs 1 head snapshots, failure on spring-webmvc@5.3.27); dependency-graph/compare for the #3350 and #3348 heads; actions/dependency-review-action action.yml at a1d282b3 for the retry inputs. Not verified: the first master run (artifact download, snapshot acceptance, a real base/head comparison), which the PR body lists as post-merge acceptance. The codecov/project failure is a project-wide drop; this PR changes no Java code.

Comment thread docs/ci.md
- require a successful baseline for the exact PR event base SHA
- distinguish the same repository from other baseline streams
- explain missing or skipped baselines are not fixed by review reruns
Pengzna
Pengzna previously approved these changes Oct 15, 2026

@bitflicker64 bitflicker64 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.

Blocking: yes. Summary: Score 8/10. At e821d8e the read-only Ferstl generator, the independent dependency:tree inventory, the mixed-scope fix and the checkout-free writer hold up, and I found no new defect in dependency-snapshot.py, dependency-inventory.py or the inline writer. One gap is still open, and the new rerun guidance at docs/ci.md lines 193-197 now states it without a way out: a master commit that is no longer the tip when its run reaches a freshness check never gets a baseline, so a PR whose event base is that commit keeps failing dependency review. Evidence: read the full diff against base 06f90a9 (7 files, +1216/-1) and the unresolved line 88 thread; python3 -m unittest discover -s .github/scripts for test_dependency_snapshot.py (18 tests) and test_ci_policy.py (21 tests), both OK; server-ci.yml line 100 discovers test_*.py, so the fixture-only set() entry in ci-policy.py is covered; no POM declares a test-jar dependency or a version range, so the separate -Dmaven.test.skip=true install does not starve either resolver; master commit times from gh api -X GET repos/apache/hugegraph/commits and mvn install step timing from job 116186719254; the dependency-review job 116186719345 at this head logs 0 base vs 1 head snapshots and fails on spring-webmvc@5.3.27, which this PR is meant to fix once a baseline exists. Not verified: the first master run (artifact download, digest, snapshot acceptance, a real base/head comparison), which the PR body keeps as post-merge acceptance. codecov/project fails on a project-wide drop; this PR changes no Java code.

Comment thread .github/workflows/submit-dependencies.yml Outdated

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Blocking: yes. Summary: Aliases that share a package URL can overwrite dependency scope and cause the independent inventory check to reject the baseline. Evidence: exact-head static comparison of the snapshot producer, inventory merger, and submission writer.

raise ValueError(f'Duplicate artifact ID: {identifier}')
ids[identifier] = purl
packages.setdefault(purl, [])
scopes[purl] = 'development' if set(artifact.get('scopes') or []) == {'test'} else 'runtime'

@imbajin imbajin Oct 15, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Update: the scenario below uses hand-built input. It has not been reproduced in unmodified output from the pinned Ferstl 4.0.3 exporter. The blocking classification was withdrawn in the follow-up. This discussion remains open for the requested minimal POM and native exporter output; no adapter change is currently justified.

⚠️ Unconfirmed. Blocking: no (classification withdrawn). Summary: scopes[purl] is overwritten for each artifact, even when distinct artifact IDs map to the same PURL. If a runtime-scoped alias is followed by a test-only alias, this records development, while dependency-inventory.py merges the same PURL with runtime precedence; the writer then rejects the otherwise valid baseline on inventory mismatch. Please merge scope across aliases with runtime precedence and add cases for both artifact orders. Evidence: the snapshot code merges aliased IDs by PURL below, the independent inventory preserves runtime when merging duplicate PURLs, and the writer requires exact scope equality.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The assignment is order-dependent for hand-built aliases, but I have not established that this input is produced by the workflow's pinned Ferstl 4.0.3 graph goal. Its node ID configuration includes group/artifact/type/classifier and omits scope. The visitor merges existing nodes, and the merge unions their scopes. Runtime and test occurrences therefore reach the serializer in one artifact's scopes array, which the current code already classifies as runtime.

The hand-built alias test covers serializer behavior, not proof that this split-scope shape comes from the native exporter. Could you provide a minimal POM and unmodified Ferstl 4.0.3 output containing distinct IDs with the same PURL and separate runtime/test scopes? That would let us verify a reachable case before changing the producer-specific adapter.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Follow-up on the alias-scope finding at this revision: I checked the pinned Ferstl graph exporter configuration and its node merge implementation. Effective nodes with the same artifact identity are merged and their scopes are unioned. The hand-built split-scope alias case demonstrates order-dependent adapter behavior, but does not establish that the workflow can produce that input. I am withdrawing the blocking classification for this finding unless an unmodified native-exporter reproduction establishes reachability. This does not resolve the separate old-event baseline recovery concern or establish first-master acceptance.

Reviewed source: snapshot adapter at e821d8e79e74666435d1d6fa73fa1432ebbd1cca. Upstream evidence linked in the author's reply: node ID configuration, visitor node merge, and scope union. This is source inspection; no native exporter test was independently run for this follow-up.

- reuse one cached reactor install for both resolvers
- cancel superseded master runs within the same ref
- document current-tip recovery and test producer guards
MrJs133
MrJs133 previously approved these changes Oct 16, 2026

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fresh independent cross/adversarial review of this revision: 8/10 overall. Two independent full reviews found no new confirmed source blocker.

Independent execution passed 20 snapshot/inventory/writer tests, 21 CI-policy tests and 500 additional deterministic synthetic graph/tree differential cases. The read-only producer and checkout-free token-bearing writer boundary remains sound. In the workflow at this revision, combining resolvers removes duplicate reactor installation, but a reactor install still runs on every master push, including documentation-only pushes; existing selective dependency/config PR checks are separate. The smallest suggested cost reduction is to fold lightweight validation into the read-only producer while retaining the isolated writer. Further trigger reduction needs an explicit exact-base snapshot/comparison policy.

Current-tip recovery is documented; historical backfill is deferred. No independent native Maven generation or real snapshot submission was run. The first trusted master artifact/submission and exact-base comparison remain operational acceptance gaps.

Current ASF CI fails dependency-review, and codecov/project is red; runtime lanes pass. The dependency-review log has no base snapshot and flags pre-existing spring-webmvc:5.3.27, not a newly introduced dependency. This score is an evidence-bounded review assessment, not CI approval or first-master acceptance.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Blocking: no. Summary: The writer checks package membership but not child-edge equality. Evidence: .github/workflows/submit-dependencies.yml:334-349 validates edge references and compares package scope/relationship; the independent inventory records no child edges.

'Missing direct dependencies')
observed = {purl: {'scope': package['scope'], 'relationship': package['relationship']}
for purl, package in resolved.items()}
require(observed == inventory['manifests'][location], 'Dependency inventory mismatch')

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

⚠️ Important. Blocking: no. Summary: The writer does not compare submitted dependency edges with the independent Maven inventory. Evidence: lines 347-349 compare only scope and relationship, while dependency-inventory.py records no child edges; GitHub's submission schema consumes these as direct-child PURLs. Please compare child edges too and add a regression that changes an edge while retaining the same package set.

@bitflicker64 bitflicker64 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.

Blocking: no. Score: 9.5/10 (10 - 0.5 for the open child-edge item at line 349, which I count as Minor: dependency-review diffs package sets, not edges).

Summary: the delta since e821d8e runs the dependency:tree inventory inside generate after one shared reactor install, adds the setup-java Maven cache, scopes the concurrency group per ref with cancel-in-progress: true, and documents recovery for a PR whose base SHA has no baseline. I found no new defect, and the workflow's security boundary is unchanged.

Evidence:

  • Triggers are only push to master and workflow_dispatch. Every job is gated to refs/heads/master in apache/hugegraph (non-fork) or hugegraph/hugegraph.
  • Only submit has contents: write plus actions: read. generate keeps contents: read, blanks the token variables and does not persist checkout credentials. The writer still binds both artifacts to this run, attempt, SHA and digest, and now requires exactly one successful generate job (lines 262-265).
  • Maven cache: setup-java 6.0.1 restores only the exact key (a hash of the pom.xml files, no restore-keys). A master run can read only master-scoped entries. Those are written by master runs (server-ci push, schedule, dispatch), never by pull_request runs, whose entries stay in the PR merge-ref scope. The only pull_request_target workflow posts a comment and has no cache or checkout. The job also reinstalls the reactor before both resolvers run.
  • Child edges: the dependency-review-action change schema has change_type, manifest, ecosystem, name, version, package_url, license, source_repository_url, scope and vulnerabilities, with no relationship or edge fields. An unchecked edge can affect the transitive path GitHub shows for an alert, but it cannot change a review result. Package set, scope and relationship are already checked against the inventory.
  • I reproduced the head's job sequence locally on all 38 POMs (reactor package, then Ferstl 4.0.3 graph, then dependency:tree 3.8.1) and applied the writer's line 349 equality check: 38 manifests, 5054 packages, 0 mismatches. test_dependency_snapshot.py (20 tests), test_ci_policy.py (21) and install-dist/scripts/dependency (7) pass.
  • dependency-review still fails at this head with 0 base snapshots, which is the condition this PR fixes.

Status of earlier threads:

  • My line 90 thread (a superseded master commit never gets a baseline): the documented alternative in 81a73d4 addresses it. docs/ci.md lines 199-211 say a skipped or superseded SHA has no baseline, and that the author must update and push so the new event's pull_request.base.sha has one. SHA backfill stays deferred, as the PR body states.
  • Earlier rounds (ASF allow list, re-run guidance, indirect-only fixture, inventory completeness, mixed scope): still fixed.
  • Alias-scope thread (dependency-snapshot.py:93): withdrawn as unreproduced. The real reactor output has no duplicate package URLs. Not counted.
  • Child edges are not compared with the inventory (line 349): still open, counted as Minor above.

Not verified: the first master run in either repository (artifact download, snapshot acceptance, a real base/head comparison). The PR body keeps this as post-merge acceptance, so I am leaving a comment and not an approval for now.

- share dependency inventory and baseline reactor builds
- fold Helm, Struct and RocksDB checks into existing jobs
- retain failure checks, Javadoc and isolated snapshot writes
- reserve merged-job budgets and cancel stale master runs
- document runner reductions and focused manual validation
- integrate current upstream source and build fixes
- preserve all updated Helm checks in the shared action
- retain runner consolidation and snapshot verification

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent cross-review of this revision: 7/10, needs the shared-job ordering fix before merge.

[P1] Struct is skipped because Codecov artifacts break the root RAT check. The shared workflow invokes mvn -pl hugegraph-struct -am test at lines 552–558 after Codecov at lines 521–528. The uploader leaves codecov.SHA256SUM and codecov.SHA256SUM.sig in the checkout; -am re-enters the root lifecycle and RAT rejects exactly these two files. In the exact-head Store job, the files are downloaded at 21:35:12 UTC, Struct starts at 21:35:19, and at 21:35:23 RAT fails with two unapproved files and hugegraph-struct is SKIPPED. Prior Store suites pass. Move Struct/reactor checks before Codecov, or isolate uploader artifacts outside the checked source tree; do not weaken RAT. Add a shared-workspace/step-order regression to catch this integration hazard.

Validation boundary: two independent source/log reviews; 45 bounded Python tests passed (25 policy, 20 dependency snapshot/inventory/writer), all 12 extracted Helm command bodies and effective shell behavior matched, and all 16 relevant routing combinations retained a single RocksDB host plus Struct fallback. Exact-head CI passed Helm-in-preflight and RocksDB-in-PD. We did not independently execute the complete Maven reactor or rerun external CI; trusted-master baseline submission/comparison still needs end-to-end evidence. This score applies only to the linked revision, not future commits.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The exact-head Store/RAT finding in review 5560237642 remains unresolved; this review adds a separate failure on the first push that creates a matching release/test/TinkerPop branch. Evidence: exact-head Store job 117506515037 and the static push-policy/action path.

AFTER_SHA: ${{ github.event.pull_request.head.sha || github.event.after || github.sha }}
run: |
for revision in "$BEFORE_SHA" "$AFTER_SHA"; do
if [[ ! "$revision" =~ ^[0-9a-f]{40}$ || "$revision" =~ ^0+$ ]]; then

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

⚠️ Important. Blocking: no. Summary: On the first push that creates a matching release-*, test-* or tinkerpop-* branch, github.event.before is the all-zero SHA. The coordinator's diff fails and falls back to mainline checks, so this action runs but this guard rejects the before SHA before comparison. Please handle the created-ref case by comparing from the new commit's parent or skip the comparison with an explicit notice; add a regression for the zero before value. Evidence: the exact-head server-ci.yml push filter includes these branches, and ci-policy.py:create_plan() diffs against event.before then falls back to mainline if that diff fails. GitHub documents before as the pre-push SHA, and Git documents the all-zero old OID for a newly created ref: https://docs.github.com/en/webhooks/webhook-events-and-payloads#push and https://git-scm.com/docs/githooks/2.46.0.html#_pre_receive.

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