Skip to content

docs: correct the workflow inventory and the collector client split - #382

Closed
sh4mbhavi wants to merge 2 commits into
Hardhat-Enterprises:mainfrom
sh4mbhavi:docs/correct-workflow-and-collector-docs
Closed

sh4mbhavi wants to merge 2 commits into
Hardhat-Enterprises:mainfrom
sh4mbhavi:docs/correct-workflow-and-collector-docs

Conversation

@sh4mbhavi

Copy link
Copy Markdown
Contributor

What is wrong

Two pages describe the repository as it was, not as it is, and both errors send a contributor the wrong way.

docs/DevSecOps/workflow-documentation.md named ten of fourteen workflows

ci.compliance.yml, ci.gitleaks.yml, ops.branch-cleanup.yml and pr.size-warning.yml appeared nowhere on the page, which nevertheless generalised over the workflows as though the list were complete.

Four of its statements were wrong:

Page said Actually
ci.grype.yml — "fail-build is false so findings are surfaced for review rather than blocking merges" fail-build: true, severity-cutoff: critical. A critical finding fails the check.
ops.collector.yml "runs the audit engine collector on the engine-development branch" Push trigger commented out, job carries if: false. It runs nothing, deliberately.
ops.workflow-cleanup.yml "runs on a schedule to delete old workflow run history" The scheduled step is --dryRun=true. Deletion needs a manual dispatch with dry_run=false and confirm=DELETE. Schedule is Sundays 00:00 UTC, not Saturdays 23:32.
ci.validate-alerts.yml "does not do anything meaningful in its current state" It runs promtool check rules and fails the build on a syntax error.
pr.size-warning.yml — undocumented, and the obvious reading is wrong It does not comment on the PR. It emits a core.warning annotation and a job summary.

The fail-build one is the costly error: a contributor who believes the page is surprised by a red check they were told could not block them.

ops.workflow-cleanup.yml turned out to be worse than out of date. It is not a retention policy at all: the workflow passes RETENTION_DAYS: 30 and its dispatch input is described as "Delete runs older than this many days", but tools/workflow-cleanup/cleanup-workflows.js never reads that variable. It selects by how long a run took — under two minutes is deleted, two minutes or more is kept — across a single unpaginated page of 20 runs. A real deletion run would remove recent short runs and keep old long ones. Nothing here prunes by age, so this workflow should not be cited as retention evidence.

Two facts are now written down that were not:

  • ci.opa-eval.yml evaluates engine/legacy/ and uploads a PDF and a JSON artifact. No policy verdict from it can fail a build — aggregator.py records a non-zero opa exit into the report JSON rather than raising — though a crash in either script still fails the job. Nothing in .github/workflows/ runs opa check, opa test or opa eval against the CIS or Essential Eight policies.
  • ops.short-test.yml filters on .github/workflows/short-test.yml while the file is .github/workflows/ops.short-test.yml. Commit 155f82aa applied the ops. prefix and left the filter behind, so editing the canary no longer runs it.

engine/collectors/README.md said GraphClient was "shared across all Microsoft collectors"

It is not, and the gap is nearly half the registry. Resolving each DATA_COLLECTORS entry to the client its collect method declares gives 26 GraphClient and 22 PowerShellClient out of 48. Every Exchange Online and SharePoint Online setting AutoAudit reads comes through a cmdlet, because Graph does not expose it — so the page sent every new Exchange, Teams or SharePoint collector to the wrong client.

The page now leads with picking a client, shows a PowerShell worked example beside the Graph one, and states the mechanism that actually decides: engine/worker/tasks.py selects the client from the collector's registered ID prefix, before the class is instantiated, so a PowerShell collector registered under an entra. ID is handed a GraphClient and fails. That rule and the collect annotations agree for all 48 today. exchange.dns.dns_security_records is the carve-out and is explained.

Four smaller corrections on the same page: the imports in the examples are rooted at collectors., not engine.collectors.; the folder tree showed an m365/exchange|sharepoint|teams layout that does not exist; _pending/ — which holds the five Teams and two Purview collectors — was not mentioned; and two GraphClient guarantees were overstated. The client returns its cached token with no expiry check, so it is caching and not refresh, and get_all_pages stops at max_pages (default 100) and returns what it has rather than raising.

Verification

$ ls .github/workflows/
ci.backend-api.yml   ci.compliance.yml    ci.engine.yml        ci.frontend.yml
ci.gitleaks.yml      ci.grype.yml         ci.opa-eval.yml      ci.security.yml
ci.validate-alerts.yml
ops.branch-cleanup.yml  ops.collector.yml  ops.short-test.yml  ops.workflow-cleanup.yml
pr.size-warning.yml

workflows on disk : 14
named by the page : 14
on disk, unnamed  : none
named, not on disk: none
$ python3 -  # resolve every DATA_COLLECTORS entry to its collect() client
DATA_COLLECTORS entries: 48
  GraphClient: 26
  PowerShellClient: 22

The collector split is resolved by importing collectors.registry and reading each class's collect signature, not by counting git grep hits.

Merge order

The inventory is stated against this branch's base, which is the preview-deploy removal PR in this series: fourteen workflow files, after the three pr.preview-* files are gone. Merge that one first.

Caveat (GRC-D03)

Demonstrates policy-decision correctness against fixtures. No live tenant has been
collected, so this is not evidence of any organisation's control posture.

`pr.preview-deploy.yml` starts the infrastructure from `docker-compose.yml` and
then starts the backend against a hardcoded connection string:

    -e DATABASE_URL=postgresql+asyncpg://autoaudit:autoaudit_dev_password@db:5432/autoaudit

`f7981302` externalised that password, so the database no longer has it and the
backend cannot connect. The last run that was not skipped, on PR Hardhat-Enterprises#306 on
2026-08-31, failed at "Wait for backend health (includes Alembic migrations)"
after the two-minute timeout; every run since has been skipped because nobody
applies the `deploy-preview` label any more.

Beyond not working, the stack holds a hosted runner for up to six hours per
preview, publishes mutable `pr-<number>` image tags to GHCR -- the only registry
push in the repository -- and exposes the runner through a Cloudflare quick
tunnel. `pr.preview-instructions.yml` and `pr.preview-teardown.yml` exist only to
advertise and stop it.

`git log -- .github/workflows/pr.preview-deploy.yml` has the original if previews
are ever revived.
Two pages described the repository as it was, not as it is, and both errors
send a contributor the wrong way.

**`docs/DevSecOps/workflow-documentation.md` named ten of fourteen workflows**
and generalised over them as if the list were complete. `ci.compliance.yml`,
`ci.gitleaks.yml`, `ops.branch-cleanup.yml` and `pr.size-warning.yml` appeared
nowhere. The page now names all fourteen, with one table of triggers and path
filters, so a missing workflow shows up as a gap rather than as silence.

Five of its statements were wrong:

- **`ci.grype.yml`: "fail-build is false so findings are surfaced for review
  rather than blocking merges."** It is `fail-build: true` with
  `severity-cutoff: critical`. A critical finding fails the check.
- **`ops.collector.yml` "runs the audit engine collector on the
  engine-development branch".** Its push trigger is commented out and the job
  carries `if: false`; it runs nothing, deliberately, and its header records the
  two conditions for re-enabling it.
- **`ops.workflow-cleanup.yml` "runs on a schedule to delete old workflow run
  history".** The scheduled step is `--dryRun=true`; deletion needs a manual
  dispatch with `dry_run=false` and `confirm=DELETE`. Its schedule is Sundays
  00:00 UTC, not the Saturdays 23:32 the table claimed. And it is not a
  retention policy at all: the workflow passes `RETENTION_DAYS: 30` and its
  input is described as "Delete runs older than this many days", but
  `tools/workflow-cleanup/cleanup-workflows.js` never reads that variable. It
  selects by how long a run took -- anything under two minutes is deleted, two
  minutes or more is kept -- over a single unpaginated page of 20 runs. A real
  deletion run would remove recent short runs and keep old long ones.
- **`ci.validate-alerts.yml` "does not do anything meaningful in its current
  state".** It runs `promtool check rules` and fails the build on a syntax
  error. What it does not do is now stated instead: it globs five filename
  patterns non-recursively, so a new rule file under another name escapes it;
  promtool is installed unpinned with `apt-get`; and a syntax check does not
  test that a threshold fires or that an alert reads a metric the application
  emits.
- **`pr.size-warning.yml`,** newly documented, does not comment on a pull
  request as the obvious reading suggests. It emits a `core.warning` annotation
  and a job summary.

Two things worth knowing are now written down: `ci.opa-eval.yml` evaluates
`engine/legacy/` and `aggregator.py` records a non-zero `opa` exit into the
report JSON rather than raising, so no policy verdict from it can fail a build,
and nothing in `.github/workflows/` runs `opa check`, `opa test` or `opa eval`
against the CIS or Essential Eight policies; and `ops.short-test.yml` filters on
`.github/workflows/short-test.yml` while the file is `ops.short-test.yml`,
because `155f82aa` applied the `ops.` prefix and left the filter behind.

**`engine/collectors/README.md` said `GraphClient` was "shared across all
Microsoft collectors (Entra, M365 services)".** It is not, and the gap is nearly
half the registry: resolving each `DATA_COLLECTORS` entry to the client its
`collect` method declares gives 26 `GraphClient` and 22 `PowerShellClient` out of
48. Every Exchange Online and SharePoint Online setting AutoAudit reads comes
through a cmdlet, because Graph does not expose it.

The page now leads with picking a client, shows a PowerShell worked example
beside the Graph one, and states the mechanism that actually decides:
`engine/worker/tasks.py` selects the client from the collector's registered ID
prefix, so a PowerShell collector registered under an `entra.` ID is handed a
`GraphClient` and fails. `exchange.dns.dns_security_records` is the carve-out
and is explained.

Four smaller corrections on the same page: the imports in the examples are
rooted at `collectors.`, not `engine.collectors.`; the folder tree showed an
`m365/exchange|sharepoint|teams` layout that does not exist; `_pending/`, which
holds the Teams and Purview collectors, was not mentioned; and two client
guarantees were overstated -- `GraphClient` returns its cached token without an
expiry check, and `get_all_pages` stops at `max_pages` (default 100) and returns
what it has rather than raising.

Verified:

    $ ls .github/workflows/          # 14 files, all named by the page
    $ python3 - (resolve DATA_COLLECTORS to each collect() client)
      DATA_COLLECTORS entries: 48
        GraphClient: 26
        PowerShellClient: 22
@sh4mbhavi
sh4mbhavi requested a review from a team as a code owner September 8, 2026 06:47
@sh4mbhavi

Copy link
Copy Markdown
Contributor Author

Closing. This was stacked on the preview-deploy-removal branch, so its diff carried PR #378 as well as the doc corrections. PR 8's workflow-inventory count depends on #378 landing, so it will be reopened clean once #378 merges. No work lost; the branch is kept.

@sh4mbhavi sh4mbhavi closed this Sep 8, 2026
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.

1 participant