ErikBPF commented on PR #7311:
URL:
https://github.com/apache/datafusion-comet/pull/7311#issuecomment-6499777678
Reviewed `f2e5fa60c8d943af8daa9384a1db1baa7a9d8015`.
### P2 — `ci.yml:341` — Preserve cumulative coverage for pushes to fork
`main`
A PR from the fork’s default `main` takes the incremental `before..after`
path. A native-code push followed by a README-only push therefore sets
`build_linux_full=false`, although the PR still changes native code. Upstream
now skips that tier too, and branch-keyed concurrency can cancel the unfinished
code run. The actual workflow shell reproduced the selection failure.
**Evidence**
> elif [[ "$REPOSITORY" != "apache/datafusion-comet" && "$PUSH_REF" !=
"refs/heads/$DEFAULT_BRANCH" ]]; then
**Action:** Use cumulative coverage or full PR-tier selection for
fork-default-branch pushes. Add a code-then-docs regression test.
### P2 — `ci.yml:350` — Preserve upstream changes when the fork baseline is
stale
Let fork `main` remain at A while upstream advances to B with a native-file
change. A feature branch based on B that reverts that file now matches A, so
this diff omits it. The reproduction selected only `README.md` and returned
`build_linux_full=false`, while the upstream PR diff also contained
`native/core/src/lib.rs`. Syncing fork `main` avoids this case, but the
implementation promises conservative handling of stale baselines.
**Evidence**
> git diff --name-only "origin/$DEFAULT_BRANCH"..."$PUSH_AFTER" >
changed_files.txt
**Action:** Preserve upstream-relative changed paths or fall back
conservatively. Add a stale-baseline/native-revert regression test. Correct the
“never fewer” claim alongside the implementation.
### P2 — `docs/source/contributor-guide/ci.md:123` — Describe the queue
coverage actually provided
The new documentation presents complete queue coverage as the reason fork
results can remain advisory. The queue still selects only applicable queue-tier
suites. Nightly-only and label-only suites remain excluded, and path filters
still apply.
**Evidence**
> The merge queue, which also stays here, runs every suite on the merge
result before anything lands.
**Action:** Replace “every suite” with “the applicable queue-tier suites”
here and in the matching workflow and README explanations.
## Inherited security findings
These predate this PR. Track the repairs separately and link the issues.
### P1 — `dev/release/publish-to-maven.sh:165` — Stop logging publishing
authentication
The Nexus close request combines `curl -u` with verbose output. Curl writes
the recoverable Basic Authorization header to stderr, which the surrounding
command substitution does not capture. A loopback test with dummy credentials
reproduced the disclosure. Historical log exposure remains unverified.
**Evidence**
> -H "Content-Type:application/xml" -v \
**Action:** Remove `-v`. Have the release owner privately inspect retained
stderr or combined transcripts and revoke credentials if exposure is confirmed.
### P2 — `publish_snapshot.yml:219–220` — Keep publishing credentials out of
dry runs
`dry_run` skips Maven settings creation and changes the goal to `install`,
but both Nexus secrets still enter the build environment. Maven and its plugins
can read them before any publication is needed. A dummy Maven stub confirmed
both variables remained accessible with `GOAL=install`.
**Evidence**
> NEXUS_USER: ${{ secrets.NEXUS_USER }}
>
> NEXUS_PW: ${{ secrets.NEXUS_PW }}
**Action:** Make dry runs credential-free. Build without publishing secrets
and perform real publication in a fresh restricted job.
### P2 — `docs.yaml:89` — Separate dependency execution from
repository-write access
The docs caller grants `contents: write`, and both checkouts persist
authentication before dependency installation and Sphinx/Maven execution. A
compromised selected dependency can use the same writer intended for the final
website push. The main-ref guard blocks direct fork execution, but does not
contain dependency compromise during an authorized build.
**Evidence**
> pip install -r docs/requirements.txt
**Action:** Build docs in a read-only job with non-persisted checkout
credentials. Publish static output from a separate minimal write-scoped job.
This isolates credentials, but does not establish that a compromised builder
produced safe content.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
