Skip to content

Follow-ups from PR #21: Postgres ordering, --scan-id, inventory fixes, CLAUDE.md audit - #22

Merged
rchildress87 merged 7 commits into
mainfrom
followups-pr21
Oct 3, 2026
Merged

rchildress87 merged 7 commits into
mainfrom
followups-pr21

Conversation

@rchildress87

Copy link
Copy Markdown
Contributor

What this changes

This PR works through the follow-ups that PR #21 (sdkinv-provider-neutral) left outside its scope. Each phase was reviewed adversarially and committed on its own.

Phase 1: store correctness (deb3012f)

  • Postgres bug. ListScans, LatestCompleteScan, LatestIncompleteScan and ListCheckRuns ordered by rowid, which Postgres does not have, so they failed there with 42703. They now use newestFirst(): started_at DESC, then rowid on SQLite (so latest stays the row created last) and id on Postgres.
  • Second Postgres bug. PersistCheckRun sent an unrebound ? placeholder to Postgres (42601).
  • Test coverage. Both bugs are covered by withDialects tests that run against Postgres.
  • --scan-id. It now matches discovered_by OR verified_by, so a scan that only re-verified rows lists them, and the listing agrees with scans.resource_count. All mentions of the old --scan-as are gone.
  • Checkpoints removed. The dead checkpoint API is deleted and migration 020 drops scan_checkpoints on both dialects. The --resume help text now describes what resume actually does.
  • Flaky test fixed. TestGraphBlast_PrincipalAutoFallback failed because pflag's Changed state leaked between tests.

Phase 2: help text (2daf1994)

  • Help text and the README's jq examples used snake_case JSON keys left over from before the v0.18.0 camelCase migration. They now use the real keys.
  • Also fixed: a few stale comments, and the plural in "1 SDK extractor warnings".

Phase 3: inventory correctness (4d101f4b)

  • Azure module keys. Azure modules are now keyed <rp>/<armX>. resources/ and solutions/ both ship armmanagedapplications, and with basename keys one module's ops overwrote the other's.
  • New optional interface. LabelModuler maps a module key back to its label prefix.
  • GCP scope match. The query-parameter scope match now compares sdkinv.Ident stems.
  • Azure paged union. It now has a fixture test.

Phase 4: provider neutrality (4329a0c1)

  • Scaffold stub. Each provider now supplies its own disco-scaffold stub scanner through coverage.ScaffoldStubber, so cmd no longer keeps a per-provider signature table.
  • AWS absent-model warning. AWS now warns for a linked aws-sdk-go-v2/service/* module that has no model at the pinned ref (absentModels), like Azure's absentModules.

Phase 5: spike, measured and deferred (3b16282f)

  • Azure x-ms-arm-id refs would add 127 refs within the walk depth, on 2.6% of resource candidates.
  • The cost is a 64–214 MB source plus picking the API version per module.
  • The numbers and the decision are recorded in azureinventory/CLAUDE.md.

Final: CLAUDE.md pass (cad70519, a7a1360f)

  • Wrong facts fixed. Every claim was checked against the code. For example:
    • Placeholders do carry reference_only.
    • CI does not run check-migrations.
    • The EventBridge Rego path is Rule, not rule.
  • Code comments. Two wrong comments in pairing.go and store/dialect.go are corrected.
  • Duplication. Each repeated fact now lives in one owning file, with pointers from the others.
  • AWS guide. internal/providers/aws/CLAUDE.md is regrouped under seven parent headings. Every original body line is kept except one cross-reference, which now names its target section.
  • Release notes. The release-pipeline explanation moved to docs/release.md; the root keeps the rules that must not break.

Checklist

  • Branched from main and targeting main
  • CGO_ENABLED=0 go test ./... passes (full run with -tags grpcnotrace; Postgres subtests ran via docker, not skipped)
  • go vet ./... and golangci-lint run are clean (golangci-lint: 0 issues)
  • Formatting: golangci-lint fmt --diff is empty (the linter formats with gofumpt + goimports; see the root CLAUDE.md change)
  • make check-migrations passes (migration 020 added on both dialects)
  • No new scanners or resolvers
  • No CGO dependency introduced

Notes for reviewers

  • Migration 020 drops scan_checkpoints. Nothing ever wrote to it. disco-saas guards the table with to_regclass, so it needs no change.
  • Postgres tie order. On a same-second tie, Postgres orders by the random hex id. That order is deterministic but is not creation order. Scan ids stay random on purpose: users paste 8-character prefixes, and time-ordered ids (UUIDv7) would make scans from the same ~65 seconds share a prefix.
  • Not done, found during the audit:
    • When a scanner re-verifies a row identical to a placeholder, the update path never clears reference_only. This is unlikely and untested; it is recorded in store/CLAUDE.md.
    • Adding make check-migrations to CI is a separate decision.
  • Not committed: .claude/rules/testing.instructions.md was corrected locally, but .claude/ is gitignored, so that fix is not in this PR.

🤖 Generated with Claude Code

rchildress87 and others added 7 commits September 30, 2026 09:48
…-scan-id

Store correctness follow-ups from PR #21.

- Newest-first ordering: ListScans, LatestCompleteScan, ListCheckRuns
  tie-broke on SQLite's rowid, which does not exist on Postgres, so every
  one of them failed there. They and LatestIncompleteScan (which had no
  tiebreak) now order by newestFirst(): rowid on SQLite, keeping creation
  order for `latest`, and id on Postgres. Ids stay random hex because
  time-ordered ids would collide on the 8-char prefixes users paste.
- PersistCheckRun sent an unrebound `?` INSERT through tx.Exec, so
  `disco check --persist` failed on Postgres (42601). Findings tests now
  run under withDialects.
- `--scan-id` (resources, summary, tag-coverage) matches
  discovered_by OR verified_by, so a re-verify-only scan lists the rows
  `disco scans` counts instead of none. ResourceFilter.DiscoveredBy is
  renamed SeenBy. The removed --scan-as flag is gone from the docs;
  `disco diff` is the "new this run" view.
- Remove the checkpoint API, which never had a writer, and drop the
  scan_checkpoints table (migration 020, both dialects). --resume help now
  says what it does: reuse the scan id and re-list every service.
- Correct the ScanDiff comment and clear pflag Changed in resetGraphFlags
  (TestGraphBlast_PrincipalAutoFallback failed on every -count>1 run).

Release note: migration 020 raises the schema version, so read commands
against an existing database ask for `disco scan` to upgrade it, as with
any migration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Help text and docs that still named keys from before the v0.18.0 camelCase
wire migration, so copied jq filters returned null:

- summary and `scans show` JSON envelope help, and their jq examples
- README summary and graph jq examples (`.native_id` printed an empty
  column for every row)
- snapshot help and verify messages/flag help naming manifest fields
  (tool_version, generated_at, scan_ids -> toolVersion, generatedAt, scans)
- diff jsonl comment (change_type -> changeType; the CSV header keeps
  change_type)

Also: `scans show` no longer claims the record carries the binary
version (nothing writes it); summary notes accountName is omitted when
unknown; the extractor-warning count agrees in number; the Diagnostic
severity comment lists what is emitted ("info" | "warn"); all.go names
the real per-provider files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… paged union

SDK inventory correctness follow-ups from PR #21.

- Azure: resources/armmanagedapplications and solutions/armmanagedapplications
  share a basename, so keyed by it one module's ops answered for the
  other's imports. ImportKey and OpKey now key by "<rp>/<armX>". Labels keep
  the "armX:" prefix; a new optional pairing.LabelModuler maps a key back,
  used by moduleAbsent, the only core site that compares a label's module
  prefix. Live effect: pairings, unpaired and `coverage services` output
  unchanged (both modules map to the same candidates); 14 sdk-skew messages
  now name the rp/armX key. Fixture gadgetry/armwidgets beside
  widgets/armwidgets pins the case where the candidates differ.
- GCP: a required query parameter names a tenancy root by Ident stem, so a
  plural ?projects= scopes to project. Live output unchanged; fixture
  widgets/shelves pins it.
- Azure: fixture method Refresh reaches the list builder after the pager,
  guarding `b.paged = b.paged || paged`, which no test covered.
- sdk-skew message no longer doubles the module; docs record the key
  scheme, the optional resolver hooks and why idx.other stays
  single-valued.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dules without a model

- The stub scanner disco-scaffold emits (imports, serviceEntry fn
  signature, TODO body) moved from a provider-keyed table in
  cmd/disco-scaffold to an optional coverage.ScaffoldStubber each
  provider's coverageProvider implements. Scaffold output is
  byte-identical for every provider, registered or not. Tests now fail if
  a registered provider drops its stub, or if a stub's signature drifts
  from that provider's serviceEntry.fn type (which used to yield a
  scaffold that parsed but did not compile).
- AWS absentModels warns for each linked aws-sdk-go-v2/service module
  whose package has no Smithy model at SDKRef, the analogue of Azure's
  absentModules. Existence only; a per-module version compare would warn
  whenever go.mod lags the pin. Live: 0 of 312 linked service packages
  absent; coverage output unchanged. linkedModules moved to
  sdkinv.LinkedModules for both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Measured whether azure-rest-api-specs arm-id annotations would add refs
the shape rule misses. Joined at each SDK client's api-version, with
common-types resolved, the element's own id skipped and discriminator
subtypes walked: 127 new within-depth refs on 64 of 2,478 resource
candidates (2.6%), while 88% of today's shape refs carry no annotation.
Not worth a 64-214 MB source plus per-module version selection; the
numbers, the join requirements and the corrected first measurement are
in azureinventory/CLAUDE.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-md-improver pass over the repo's CLAUDE.md files, with each
claim checked against the code.

Wrong facts corrected:
- store: placeholders do carry a marker (reference_only, 011); the
  InsertResourcesIfAbsent caller list missed three GCP resolvers
  (now a grep pointer); check-migrations is not run in CI;
  resource_count is computed in SQL (scanResourceCountExpr), not
  passed in; UpsertQuotas is a withWriteRetry writer; dimension_key
  came in 019.
- cmd: the EventBridge wrapper key is "Rule", not "rule" (a Rego rule
  copied from the doc matched nothing); service-filter examples come
  from each provider's ServiceFilterExemplar; incomplete --output lists.
- root: the linter formats with gofumpt + goimports; there is no
  project gofmt config. Use golangci-lint fmt.
- Leftovers from this branch: the Azure paged-union rule is now
  covered, x-ms-arm-id is deferred rather than "the follow-up",
  ScaffoldStubber was missing from coverage and the sdkinv README, the
  GCP scope rule now compares Ident stems, and absentModels and
  absentModules were attributed to the wrong files.
- providers: three design rules, not two; gcp's pairing guard also
  accepts explained unpaired reasons; sdkinv README's --with-refs row.

Code comments: pairing.Diagnostic.Kind listed a kind nothing emits
and omitted sdk-skew/sdk-module-absent; store/dialect.go's
placeholder() doc comment sat on newestFirst().

Duplication: each fact now has one owner (store owns newestFirst,
SeenBy, the JSON contract and the ErrStoreWrite rationale; providers
owns slim wiring and redaction rules; inventory files own their drop
counts and live sizes), with pointers elsewhere. Incident narratives
and frozen counts were cut; the x-ms-arm-id spike keeps its decision
numbers (full numbers in 3b16282).

internal/providers/aws/CLAUDE.md: ~75 flat sections regrouped under
seven parent headings. Every original body line is kept verbatim except one
cross-reference reworded to name its target section; one
pair of sections merged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… Postgres section

The root CLAUDE.md carried ~25 lines of release-engineering narrative
(SBOM generation, the govulncheck gate, binary vs source mode). The
full explanation now lives in docs/release.md. The root keeps only
the rules an edit to the Makefile, release.yaml or go.mod must not
break: SBOMs from the raw binary before compression, pinned tool
versions kept in sync, never vulncheck a stripped binary, and bump
the Dockerfile GO_VERSION with the go directive.

store/CLAUDE.md: the row-level-security requirements for an embedder
widening the relationships key moved from the Tables bullet to the
Postgres backend section, as a numbered list. The text maps each
missed condition to its failure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rchildress87
rchildress87 merged commit a11d3ef into main Oct 3, 2026
2 checks passed
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