Skip to content

Commit e71ccdb

Browse files
committed
fix(bootstrap): address Reviewer findings on PR #74 (RAN-47)
All 8 review findings on the bootstrap PR addressed in one commit on the same branch — squash-merge stays clean. Findings → fixes: 1. pom.xml: dependency-check:check was configured (failBuildOnCVSS=7) but not bound to a Maven phase, so `mvn verify` never ran the gate. Added an `<execution>` binding `check` to `verify` (RAN-46 AC #5). 2. shared/runbooks/release.md §3: the runbook said "push v* tag → workflow runs", but `release-java.yml` is `workflow_dispatch` only and the workflow itself creates and pushes the tag. Rewrote §3 to use `gh workflow run release-java.yml -f version=X.Y.Z` and to describe the actual deploy → tag → GH Release order. Direct tag-push without the workflow does not publish. 3. scripts/setup-git-signed.sh: removed the hard-coded "Amit Kumar" / "ak.nitrr13@gmail.com" defaults. Identity now resolves from env vars, then `git config --global` (user.name / user.email / user.signingkey), and the script errors out (rc=4) with a clear remediation message if neither is set. No more silent maintainer-misattribution. 4. shared/runbooks/first-time-setup.md §2: replaced the invalid `git verify-commit --raw -` (which expects a commit id, not stdin) with a working two-step pattern that captures the signed object and verifies it via `git verify-commit "$sig_commit"` + `git log -1 --pretty=%G?`. 5. shared/runbooks/first-time-setup.md §3 quick-loop: dropped the contradictory `-DskipTests test` (which skipped every test). Now uses `-Dspotbugs.skip=true -Ddependency-check.skip=true` to keep the inner loop fast WITHOUT skipping tests, and adds a note explaining the prior draft was wrong. 6. shared/runbooks/first-time-setup.md §5: removed Scorecard from the "required PR-green check" list — Scorecard runs on push-to-main + weekly cron, never on pull_request, and is intentionally non-gating per engineering-standards.md §1. Replaced "signed-commits status check" with the correct framing (branch-protection rejects unsigned commits, not a separate status check). 7. SECURITY.md: replaced the stale `.github/workflows/codeql.yml` link (workflow removed in 35762b1) with a description of the repo-level CodeQL default setup that supersedes it. Also clarified that the workflow-driven codeql.yml was attempted and removed because of the default-setup SARIF-upload conflict. 8. shared/runbooks/release.md §2 pre-release checklist: dropped the "OSV-Scanner workflow latest run green" line (no such workflow). The dependency audit gate is now the bound `mvn verify` from fix #1, with a Dependabot security-tab cross-check. Refs RAN-47 (Reviewer findings comment 5a572640).
1 parent 9b9665c commit e71ccdb

5 files changed

Lines changed: 73 additions & 31 deletions

File tree

SECURITY.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ Out of scope:
5757
- [`shared/runbooks/engineering-standards.md`](shared/runbooks/engineering-standards.md) — CVE policy and quality gates.
5858
- [`shared/runbooks/rollback.md`](shared/runbooks/rollback.md) §6 — secret rotation flow.
5959
- `.github/workflows/scorecard.yml` — OpenSSF Scorecard supply-chain checks.
60-
- `.github/workflows/codeql.yml` — code scanning (SARIF in the Security tab).
61-
- `.github/dependabot.yml` — automated dependency / GHA / Docker bumps.
60+
- GitHub repo-level **CodeQL default setup** (java-kotlin + javascript-typescript + actions) — code scanning, SARIF in the Security tab. Configured under repo Settings → Code security → Code scanning, not via a workflow file (a workflow-driven `codeql.yml` was tried and removed because GitHub rejects duplicate SARIF uploads when default setup is on for the same language).
61+
- `.github/dependabot.yml` — automated dependency / GHA / npm bumps.
6262

6363
## Changelog
6464

pom.xml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,18 @@
410410
<!-- Fail build on High/Critical CVEs (CVSS >= 7) per security.md -->
411411
<failBuildOnCVSS>7</failBuildOnCVSS>
412412
</configuration>
413+
<executions>
414+
<!-- Bind dependency-check:check into the verify phase so
415+
`mvn -B -ntp clean verify` enforces the CVSS>=7 gate
416+
(RAN-46 AC #5 / Reviewer finding #1). -->
417+
<execution>
418+
<id>dependency-check-on-verify</id>
419+
<phase>verify</phase>
420+
<goals>
421+
<goal>check</goal>
422+
</goals>
423+
</execution>
424+
</executions>
413425
</plugin>
414426

415427
<plugin>

scripts/setup-git-signed.sh

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,30 @@ fi
2323

2424
cd "$repo_root"
2525

26-
# Defaults can be overridden by env vars so the same script works for forks.
27-
GIT_USER_NAME=${GIT_USER_NAME:-"Amit Kumar"}
28-
GIT_USER_EMAIL=${GIT_USER_EMAIL:-"ak.nitrr13@gmail.com"}
29-
GIT_SIGNING_KEY=${GIT_SIGNING_KEY:-"$HOME/.ssh/id_ed25519.pub"}
26+
# Identity is taken from env vars first, then from the user's GLOBAL git
27+
# config — never hard-coded to the maintainer. This avoids silently
28+
# misattributing every contributor's signed commits to the maintainer
29+
# (Reviewer finding #3).
30+
GIT_USER_NAME=${GIT_USER_NAME:-$(git config --global --get user.name 2>/dev/null || true)}
31+
GIT_USER_EMAIL=${GIT_USER_EMAIL:-$(git config --global --get user.email 2>/dev/null || true)}
32+
GIT_SIGNING_KEY=${GIT_SIGNING_KEY:-$(git config --global --get user.signingkey 2>/dev/null || echo "$HOME/.ssh/id_ed25519.pub")}
33+
34+
if [ -z "$GIT_USER_NAME" ] || [ -z "$GIT_USER_EMAIL" ]; then
35+
cat >&2 <<'EOF'
36+
error: contributor identity not set.
37+
38+
This script does not assume a default identity. Set yours either:
39+
1. Globally (recommended):
40+
git config --global user.name "Your Name"
41+
git config --global user.email "you@example.com"
42+
2. Per-invocation:
43+
GIT_USER_NAME="Your Name" GIT_USER_EMAIL="you@example.com" \
44+
scripts/setup-git-signed.sh
45+
46+
Then re-run this script. Signed commits will use the identity you set.
47+
EOF
48+
exit 4
49+
fi
3050

3151
# The signing key path must exist before we wire up signing — otherwise every
3252
# commit will fail and the local config will be silently broken.

shared/runbooks/first-time-setup.md

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,19 @@ Sanity-check the config:
6060
```bash
6161
git config --local --get user.signingkey # should print a path ending in .pub
6262
git config --local --get commit.gpgsign # should print "true"
63-
echo test | git commit-tree HEAD^{tree} -m test --gpg-sign \
64-
| git verify-commit --raw - # should report "Good signature"
63+
64+
# Produce a throwaway signed commit object (no refs touched) and verify it.
65+
sig_commit=$(echo "verify-signing" | git commit-tree HEAD^{tree} -S)
66+
git verify-commit "$sig_commit" # expect "Good ... signature"
67+
git log -1 --pretty=%G? "$sig_commit" # expect: G
6568
```
6669

70+
`git verify-commit` operates on a commit object id, not stdin — capturing the
71+
output of `git commit-tree -S` first and then verifying that id is the right
72+
shape. If the verification line errors with "no principal matched", point git
73+
at an `allowed_signers` file: see `scripts/setup-git-signed.sh` output for the
74+
canonical template.
75+
6776
---
6877

6978
## 3. Build, test, run
@@ -79,11 +88,14 @@ This runs the full pipeline: unit tests, integration tests, jacoco coverage gate
7988
For a faster inner loop while iterating:
8089

8190
```bash
82-
mvn -B -ntp -DskipTests test # unit + integration, no plugins
83-
mvn -B -ntp -Dtest=SomeDetectorTest test # single test
84-
mvn -B -ntp -DskipTests=true package # JAR only
91+
mvn -B -ntp test \
92+
-Dspotbugs.skip=true -Ddependency-check.skip=true # unit + integration, no static analysis / CVE plugins
93+
mvn -B -ntp -Dtest=SomeDetectorTest test # single test class
94+
mvn -B -ntp -DskipTests=true package # JAR only, no tests
8595
```
8696

97+
The first command **does run tests** — earlier drafts incorrectly passed `-DskipTests` here, which would have skipped them. Use `-Dspotbugs.skip` / `-Ddependency-check.skip` to keep the inner loop fast without dropping test coverage.
98+
8799
Smoke-test the CLI end-to-end against this repo:
88100

89101
```bash
@@ -129,8 +141,9 @@ gh pr create --fill --base main
129141

130142
Branch protection on `main` requires:
131143
- A Codex review approval from TechLead (or delegate).
132-
- CI green: `ci-java.yml`, Sonar Quality Gate, Scorecard workflow, signed-commits status check.
133-
- All commits in the PR signed.
144+
- CI green on the PR: `ci-java.yml` (build + jacoco 85% + dependency-check + Sonar), the repo-level CodeQL default-setup checks (`Analyze (java-kotlin)`, `Analyze (javascript-typescript)`, `Analyze (actions)`), Socket Security, SonarCloud Code Analysis.
145+
- All commits in the PR signed (branch protection rejects unsigned commits — there is no separate "signed-commits" status check).
146+
- OpenSSF Scorecard runs on push-to-`main` and a weekly cron, **not** on PRs, and is intentionally non-gating per [`engineering-standards.md`](engineering-standards.md) §1.
134147

135148
Force-push to `main` is disabled. Direct pushes are disabled. Squash-merge is the default and only path.
136149

shared/runbooks/release.md

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ Run BEFORE creating the tag:
2929
1. `main` is green: `gh run list --branch main --workflow ci-java.yml --limit 1``success`.
3030
2. SonarCloud Quality Gate: `gh api /repos/RandomCodeSpace/codeiq/actions/runs?branch=main --jq '.workflow_runs[0].conclusion'` and SonarCloud project page both green.
3131
3. Coverage ≥ 85% (jacoco rule + Sonar new-code ≥ 80% — see [`engineering-standards.md`](engineering-standards.md)).
32-
4. Dependency audit clean: `mvn dependency-check:check -DfailBuildOnCVSS=7` exits 0; OSV-Scanner workflow latest run green.
32+
4. Dependency audit clean: `mvn -B -ntp clean verify` exits 0 (the OWASP `dependency-check:check` goal is bound to `verify` and fails the build on CVSS ≥ 7 — see `pom.xml`). Cross-check with the Dependabot security tab for any open advisories.
3333
5. SpotBugs clean: `mvn spotbugs:check` exits 0.
3434
6. CHANGELOG entry drafted under `[Unreleased]` and ready to promote.
3535
7. Working copy of `main` is clean (`git status --porcelain` empty).
@@ -39,33 +39,30 @@ Run BEFORE creating the tag:
3939

4040
## 3. Cut a release (canonical path)
4141

42-
Driven by `release-java.yml` triggered on a `v*` tag push.
42+
Driven by `release-java.yml` via **manual `workflow_dispatch`** with a `version` input. The workflow itself bumps `pom.xml`, deploys to Maven Central, then creates and pushes the `vX.Y.Z` tag and the GitHub Release.
4343

4444
```bash
45-
# 1. Promote CHANGELOG
45+
# 1. Promote CHANGELOG on main
4646
$EDITOR CHANGELOG.md # move [Unreleased] → [X.Y.Z] - YYYY-MM-DD
47-
48-
# 2. Bump pom.xml version
49-
mvn versions:set -DnewVersion=X.Y.Z -DgenerateBackupPoms=false
50-
51-
# 3. Commit and push
52-
git add pom.xml CHANGELOG.md
47+
git add CHANGELOG.md
5348
git commit -S -m "chore(release): X.Y.Z"
5449
git push origin main
5550

56-
# 4. Tag (annotated + ssh-signed) and push
57-
git tag -s vX.Y.Z -m "codeiq X.Y.Z"
58-
git push origin vX.Y.Z
51+
# 2. Trigger the release workflow with the target version
52+
gh workflow run release-java.yml --ref main -f version=X.Y.Z
53+
54+
# 3. Watch it run (workflow handles versions:set, deploy, tag, GH Release)
55+
gh run watch $(gh run list --workflow release-java.yml --limit 1 --json databaseId --jq '.[0].databaseId')
5956
```
6057

6158
`release-java.yml` then:
62-
1. Builds with `mvn -B -ntp clean verify` (full test suite + jacoco gate).
63-
2. Signs artifacts with `MAVEN_GPG_*` secrets.
64-
3. Publishes to Maven Central via `central-publishing-maven-plugin`.
65-
4. Uploads `code-iq-X.Y.Z-cli.jar` to a new GitHub Release.
66-
5. Records SBOM (CycloneDX) and provenance (SLSA) as Release assets.
59+
1. Sets the pom version to `X.Y.Z` via `mvn versions:set`.
60+
2. Deploys to Sonatype Central with the `release` profile (`mvn -P release -B clean deploy`) — runs the full quality gate on the way (jacoco 85% + SpotBugs + dependency-check).
61+
3. Signs artifacts with `MAVEN_GPG_*` secrets.
62+
4. Creates the annotated tag `vX.Y.Z` and pushes it (the workflow has `contents: write`).
63+
5. Cuts a GitHub Release from the tag and uploads `code-iq-X.Y.Z-cli.jar`, with auto-generated release notes.
6764

68-
Track it: `gh run watch $(gh run list --workflow release-java.yml --limit 1 --json databaseId --jq '.[0].databaseId')`.
65+
If you prefer to drive the tag yourself (fork or downstream cut), `release-java.yml`'s `workflow_dispatch` is still the canonical entrypoint — push the tag manually only after the deploy succeeds. Direct tag-push without the workflow does **not** publish.
6966

7067
---
7168

0 commit comments

Comments
 (0)