e2e: API endpoint contract suite + status-aware blocking coverage gate#1661
Merged
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a blocking per-status-code API endpoint coverage gate and an in-process identity stub to the bronze-to-API E2E test framework, splits the E2E CI workflow into independent build/api/metrics lanes each with dedicated coverage gates, adds a new ChangesAPI Coverage Gate and Split E2E Lanes
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Test as API test
participant Client as Analytics client
participant Analytics as AnalyticsProcess
participant Ledger as observed_endpoints.json
participant Gate as api_coverage gate
participant Spec as OpenAPI spec
Test->>Client: HTTP request
Client->>Analytics: forwarded request
Analytics-->>Client: response
Client->>Ledger: record_response (method, path, status)
Note over Ledger: on pytest_sessionfinish
Gate->>Ledger: load observed
Gate->>Spec: load spec_operations
Gate->>Gate: build CoverageReport (covered/missing/uncovered)
Gate-->>Gate: PASS/FAIL exit code
sequenceDiagram
participant Build
participant ApiLane as e2e-api
participant MetricsLane as e2e-metrics
participant ApiGate as api-endpoint-coverage-gate
participant MetricGate as metric-coverage-gate
participant Suite as e2e-suite
Build->>ApiLane: shared runner image
Build->>MetricsLane: shared runner image
ApiLane->>ApiGate: coverage-inputs-api
MetricsLane->>MetricGate: coverage-inputs-metrics
ApiGate-->>Suite: pass/fail
MetricGate-->>Suite: pass/fail
Build-->>Suite: pass/fail
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ktursunov
force-pushed
the
api-coverage-gate
branch
3 times, most recently
from
July 6, 2026 17:04
6adc0c2 to
0aca453
Compare
ktursunov
force-pushed
the
api-coverage-gate
branch
4 times, most recently
from
July 8, 2026 08:07
8a3a6e7 to
af82758
Compare
2 tasks
added 11 commits
July 8, 2026 15:40
The gears host binds its HTTP listener only after MariaDB connect + sea-orm migrations + catalog seed + boot probes; on a cold container that pre-bind work can exceed a tight health gate, surfacing as an opaque "connection refused" with no logs. Raise the ceiling to 120s (env-overridable via E2E_API_HEALTH_TIMEOUT_S) and stream the child's stdout/stderr to a temp file so the startup log is surfaced on a timeout (a blocking read on a live PIPE would hang while the process is still alive, which is why the old path showed nothing). The wait still returns the instant /health answers, so the warm path costs nothing. Ported from the pre-rename api-coverage-gate branch (165fdfc) onto the gears-host lib/analytics.py. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Record every (method, path, status) the e2e suite exercises via an httpx response hook on the rig's single client chokepoint (lib/api_coverage.py, attached in AnalyticsProcess.client()); conftest.pytest_sessionfinish dumps the ledger to .artifacts/observed_endpoints.json (shipped to CI inside the existing coverage-inputs artifact). lib/api_coverage.py diffs it against the committed docs/components/backend/analytics/openapi.json — the universe, kept accurate by the analytics OpenAPI drift gate (openapi_spec_matches_committed golden test + openapi-specs.yml) — and reports covered / baseline-skipped / missing per documented operation, with SKIP_LIST hygiene (redundant or stale skips FAIL). Observability-only (non-blocking): a read-only metric suite touches few routes (most are write/admin), so a pass/fail CI gate would be ~all skip-list. ./e2e.sh gates prints it after the blocking metric-coverage gate. Restores the endpoint-coverage half scoped out of the metric-coverage PR (19e3f7f), adapted to the analytics rename and the offline-generated spec: no live-spec snapshot or drift job here anymore — the spec is drift-gated at its source. Verified against the committed spec: 2/20 operations exercised, 18 baseline-skipped, 0 missing; /health (now served by the api-gateway host gear, off the spec) reports as informational-unmatched. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…ric gate The collab Messaging tests (constructorfabric#1527) assert the response metric_key the unified-metrics path returns — the full dotted catalog key (collab_person_counter_daily.messages_sent) — but the gate's resolver only understood bare column keys mapped by suffix, so both messaging keys were flagged MISSING and the metric-coverage gate has been red since constructorfabric#1580 merged. A dotted assertion now matches the catalog directly; bare keys keep the suffix mapping. Verified: 46/108 validated, 0 missing. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…e blocking One module per path group under api/, one test per (path, method, status-code) case, resources from fixtures (api/conftest.py: scratch_metric, scratch_threshold, admin_threshold_row — created through the recording client, removed in teardown, so the metric catalog is never touched and soft-deleted scratch metrics stay invisible to GET /v1/metrics): test_catalog.py POST /v1/catalog/get_metrics test_metrics.py metric CRUD + single/batch query (13 cases) test_metric_thresholds.py legacy threshold CRUD (8 cases) test_admin_thresholds.py admin threshold CRUD (7 cases) test_columns.py column catalog (2 cases) test_persons.py person lookup (rig contract: canonical 500) All 20 spec operations are exercised plus every error code reachable in the rig (400 validation, 404 unknown/soft-deleted, 500 identity-unconfigured); 401/403/409/429 are unreachable by design (auth disabled, no rate limiting). With total coverage the endpoint gate stops being observability-only: SKIP_LIST is empty, ./e2e.sh gates exits non-zero on a miss, and a new api-endpoint-coverage-gate CI job analyses the uploaded ledger — a spec operation added without a contract test now fails CI. Verified: full suite 89 passed; api-only 38 passed in 3.2s; metric gate PASS 46/108 (0 missing); endpoint gate PASS 20/20 (0 skips, 0 missing). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The e2e job now runs two named steps — 'E2E suite — api' (endpoint contract tests, seconds) and 'E2E suite — metrics' (the yaml fixture rig) — instead of one monolithic run. meta/ (the rig's own framework self-tests) no longer runs in CI: it tests the harness, not the product; ./e2e.sh test meta/ stays for local use. !cancelled() on the metrics step keeps the two signals independent inside the job. Two pytest sessions means two sessionfinish dumps, so dump_observed now MERGES into an existing observed_endpoints.json (statuses unioned per method+path) instead of overwriting — a plain overwrite would keep only the last suite's traffic and fail the endpoint gate. CI starts from a clean checkout; locally, delete .artifacts/ for a from-scratch measurement. Verified CI-shaped locally (rm .artifacts && api && metrics && gates): 38 + 52 passed; metric gate PASS 46/108 (0 missing); endpoint gate PASS 20/20 on the merged ledger. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…ngs)
An independent QA review of the api/ contract suite produced 10 findings;
this lands the fixes:
- Endpoint gate is now STATUS-AWARE: an operation only counts as covered when
it was seen answering a declared 2xx (EXPECTED_STATUS overrides the persons
op, whose rig contract is the canonical 500); ops only ever observed
erroring, and stale overrides, fail the gate. Previously the gate was pure
route-reachability and its report implied more than it checked.
- /v1/columns tests were vacuous (no seed migration → empty set → all([]) is
True). A seeded_columns fixture now inserts two NULL-tenant rows directly
into MariaDB (the only write path that exists), so the per-table filter is
asserted against data, including cross-table exclusion.
- 9 new per-case tests: admin create 400 unknown-metric + 409 duplicate,
admin PUT/DELETE 404 unknown, catalog 400 unknown-field
(deny_unknown_fields), single-query 400 bad $orderby (injection guard),
batch partial-failure envelope (per-item RFC-9457 Problem inside a 200),
threshold create 404 unknown-metric, threshold PUT 400 bad operator.
api suite: 33 → 42 contract cases.
- match_observed prefers literal path segments over {param} at equal arity, so
a future literal route cannot be mis-attributed by spec ordering.
- Partial-update assertions now pin that absent fields survive (threshold
operator/field_name, metric is_enabled); CI resets .artifacts/ before the
suites so the merging ledger can't inherit stale coverage on a reused
workspace; corrected the invalid-query_ref docstring (fails on missing
SELECT...FROM, not a non-SELECT check).
Verified CI-shaped: api 47 passed (3.7s), metrics 52 passed, metric gate PASS
46/108 (0 missing), endpoint gate PASS 20/20 (0 skips, 0 missing) with the
status requirement active.
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Every assertion in api/test_session_smoke.py is proven implicitly on every run: the compose/migration fixtures fail the session in seconds if ClickHouse or MariaDB is down, the analytics fixture health-gates /health 200 before any test executes, and GET /v1/metrics is pinned harder by the contract case api/test_metrics.py::test_list_metrics_200. Its stale claims (rig compiles analytics via cargo; /v1/metrics response shape 'not locked yet') predate the baked runner binary and the typed OpenAPI contract. api/ is now purely the per-case contract suite (42 cases); the smoke marker stays for the remaining meta/ framework tests (local-only, CI skips meta/). Verified: api suite 42 passed; both gates PASS. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…eview Two symmetric guards on the status-aware rules: - UNCONSTRAINED: an op declaring no 2xx and carrying no EXPECTED_STATUS override would have been accepted on ANY status — now it fails until the expectation is stated explicitly (latent; no current op triggers it). - REDUNDANT EXPECTED_STATUS: an overridden op that now answers a declared 2xx (e.g. persons gaining a real identity backend) fails so the override gets actualized instead of silently masking that op's success-status regression guard — mirrors the redundant-skip rule. Both FAIL paths proven on synthetic inputs; real gates unchanged: metric PASS 46/108 (0 missing), endpoint PASS 20/20 (0 skips). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…t xfails — no product changes in this PR The contract suite found two analytics bugs; per review scope this PR carries NO Rust changes, so the tests pin them instead of fixing them: - constructorfabric#1663 (thresholds.value DECIMAL vs f64 — every read of a non-empty table 500s): test_create_threshold_201 is a strict xfail; the other success-path threshold cases xfail via the scratch_threshold fixture; temporary EXPECTED_STATUS overrides accept the pinned 400/404 error contracts for the four legacy threshold operations. - constructorfabric#1664 (duplicate admin create → 500 internal + drift alarm instead of the declared 409): test_create_409_duplicate is a strict xfail pinning the spec'd 409. Self-cleaning by construction: when either fix lands, the strict xfail XPASSes (fails CI) and, for constructorfabric#1663, the first observed 2xx makes the redundant-override hygiene fail the endpoint gate — both force removal of this scaffolding with the fix. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…+ CI lanes
Reworks the api-endpoint coverage gate and extends the api/ contract suite,
test-only (zero src/backend diff — the committed OpenAPI spec stays the
.standard_errors boilerplate and its inaccuracies are filed as bugs, not fixed
here):
- Operation-level gate (lib/api_coverage.py): the gate blocks only when a
documented operation is exercised by NO test (a new endpoint) or a SKIP_LIST
entry rots. Per-status-code coverage is REPORTED as a percentage — an
endpoints x registered-codes matrix (observed / declared-but-unobserved /
excluded) — not enforced. coverable(op) = declared - {>=500} -
UNIVERSAL_BOILERPLATE{401,429} - BLOCKED[op]; BLOCKED absorbs the boilerplate
over-declaration, and a now-observed excluded code is a non-blocking advisory.
- POST /v1/metric-results (api/test_metric_results.py): the unified-metric
compute endpoint added by the feat/unified-metrics merge (constructorfabric#1656) is covered on
its deterministic error paths — 400 (empty / bad-period / unknown-key, which
is a 400 via `unavailable`, not 404) and 415. Its 200 needs seeded observation
data and reports as a coverage gap. All 21 spec operations exercised.
- CI (e2e-bronze-to-api.yml): the suite runs as two lanes, renamed to `api` and
`metrics`, each feeding its own coverage gate, aggregated by an umbrella
`Run E2E suite` job that supplies the `main` branch-protection required check.
Contract suite (api/): per-(path,method,status) cases incl. 415 wrong
content-type and 400 path-parse; constructorfabric#1663/constructorfabric#1664 pinned as strict xfails with their
codes BLOCKED; constructorfabric#1670 (off-schema body should be canonical 400) pinned as strict
xfails.
Docs: README updated to the operation-level model; the bronze-to-api-e2e
PRD/DESIGN/DECOMPOSITION specs have the (never-shipped) temporal skip-list
feature removed. Bugs filed: constructorfabric/insight constructorfabric#1663 constructorfabric#1664 constructorfabric#1669 constructorfabric#1670.
Verified: endpoint gate PASS 21/21 (97.9% registered-code coverage) on the
merged ledger; cfs validate-toc clean.
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…structorfabric#1691) GET /v1/persons/{email} previously exercised only its no-backend 500: the rig stood up no Identity service, so the handler short-circuited and the op's 200 (found) and 404 (unknown) paths were BLOCKED as unreachable. Wire a loopback in-process Identity stub (lib/identity_stub.py) into the analytics config (APP__gears__analytics__config__identity_url) instead of the real service (kept out of v1 scope per PRD §4.2). It resolves one seeded email → 200 with a canned Person and 404s every other email. AnalyticsProcess takes an identity_url; a session-scoped `identity_stub` fixture starts it before `analytics` so the URL is known when the gear reads identity_url at init. test_persons.py now pins both documented outcomes (200 with the verbatim Person body; canonical 404 not_found carrying the email), and BLOCKED["GET /v1/persons/{email}"] drops to just the .standard_errors boilerplate {400,403,409} (constructorfabric#1669) — its required set {200,404} is now fully covered. Still test-only: zero src/backend diff. PRD/DESIGN/FEATURE updated to the stub model. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
force-pushed
the
api-coverage-gate
branch
from
July 8, 2026 12:45
2bb6c55 to
3319c8c
Compare
ktursunov
marked this pull request as ready for review
July 8, 2026 13:02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-bronze-to-api.yml:
- Line 172: In the workflow jobs that only produce or consume artifacts, update
each actions/checkout@v4 step to disable persisted credentials so the GitHub
token is not written into .git/config before running PR-controlled tests or
artifact handling. Make this change consistently in all affected jobs in the
e2e-bronze-to-api workflow, keeping the checkout behavior otherwise the same
while ensuring credentials are not retained.
- Around line 84-90: Update the stale branch-protection note in the workflow
comment block around the E2E lane jobs: the guidance currently says no job emits
“Run E2E suite,” but the workflow now includes an umbrella job with that exact
status-check name. Revise the wording near the lane/job setup so it reflects the
current behavior and no longer instructs readers to re-add or reconcile a
missing umbrella job; keep the references to the lane names (“api”, “metrics”)
and the branch protection/ruleset guidance accurate.
In `@docs/domain/bronze-to-api-e2e/specs/feature-api-coverage-gate/FEATURE.md`:
- Line 8: The feature ID in FEATURE.md has a typo in the shared naming
convention. Update the ID string in the feature entry for
cpt-bronze-to-api-e2e-featstatus-api-coverage-gate so it matches the standard
pattern used elsewhere in the docs, and keep the rest of the FEATURE.md entry
unchanged.
In `@src/ingestion/tests/e2e/api/test_admin_thresholds.py`:
- Around line 162-177: `test_create_403_locked_broader_scope` has unreachable
defensive cleanup after the `assert r.status_code == 403`, so the created row
would leak if the endpoint unexpectedly returns 201. Move the cleanup in the
test body so it runs before the status assertion (using the response from
`api.post` in `test_create_403_locked_broader_scope`), and keep the `api.delete`
fallback tied to the returned metric-threshold id rather than placing it behind
the assertion.
In `@src/ingestion/tests/e2e/conftest.py`:
- Around line 235-248: Register the identity_stub function as a pytest fixture
so it can be injected into analytics(..., identity_stub: IdentityStub) and other
tests. Add the pytest fixture decorator to identity_stub in conftest.py, keeping
its setup/teardown behavior (IdentityStub(), start(), yield, stop()) unchanged
so pytest can resolve the dependency instead of raising fixture not found.
In `@src/ingestion/tests/e2e/lib/analytics.py`:
- Around line 207-214: The startup path in analytics() launches a subprocess via
_proc and immediately calls _wait_healthy(), but if that health check raises
before yielding, the cleanup path never runs and the spawned process/log handle
can leak. Update the analytics startup flow to wrap the Popen/_wait_healthy
sequence with failure cleanup in the analytics context manager, ensuring the
subprocess is stopped and the log file is closed whenever _wait_healthy() fails;
use the existing _proc and proc.stop() cleanup logic as the reference point.
In `@src/ingestion/tests/e2e/lib/api_coverage.py`:
- Around line 322-327: The gate logic in passed currently ignores missing
coverable status coverage because it only blocks on missing, redundant_skips,
and stale_skips. Update the ApiCoverage gating path so self.uncovered is treated
as a blocking condition as well, and surface those entries from
gate_violations() so failures name the affected status codes and indicate
whether they need tests or justified exclusions. Keep the change localized to
the passed property and gate_violations() in api_coverage.py.
In `@src/ingestion/tests/e2e/lib/metric_coverage.py`:
- Around line 223-230: Preserve exact matching for dotted assertions in metric
coverage handling: the current lookup in the assertion loop still allows a
dotted `bare` key to fall through to `by_suffix` matching, which can incorrectly
mark the wrong catalog entry as covered. Update the matching logic in
`metric_coverage.py` so the assertion loop in `self.asserted` only uses suffix
resolution for bare column keys, while dotted keys must resolve strictly through
`self.universe` (and otherwise remain in `self.unknown_asserted`), keeping the
existing `covered`/`unknown_asserted` bookkeeping in `self.covered`.
In `@src/ingestion/tests/e2e/README.md`:
- Around line 123-136: Update the API endpoint coverage gate description in the
README to reflect the new status-aware blocking behavior instead of saying only
operation-level gaps are enforced and status-code coverage is merely reported.
Revise the explanation around `api-endpoint-coverage-gate`,
`lib/api_coverage.py`, and the `coverable(op)`/coverage table language so CI
users understand status-code misses can fail the gate, then reconcile the listed
API operation count here with the count used in the other PR docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4816f2b8-3ca8-42df-8f44-a91446ab6f64
📒 Files selected for processing (24)
.github/workflows/e2e-bronze-to-api.ymldocs/domain/bronze-to-api-e2e/specs/DECOMPOSITION.mddocs/domain/bronze-to-api-e2e/specs/DESIGN.mddocs/domain/bronze-to-api-e2e/specs/PRD.mddocs/domain/bronze-to-api-e2e/specs/feature-api-coverage-gate/FEATURE.mdsrc/ingestion/tests/e2e/README.mdsrc/ingestion/tests/e2e/api/__init__.pysrc/ingestion/tests/e2e/api/conftest.pysrc/ingestion/tests/e2e/api/endpoint_helpers.pysrc/ingestion/tests/e2e/api/test_admin_thresholds.pysrc/ingestion/tests/e2e/api/test_catalog.pysrc/ingestion/tests/e2e/api/test_columns.pysrc/ingestion/tests/e2e/api/test_metric_results.pysrc/ingestion/tests/e2e/api/test_metric_thresholds.pysrc/ingestion/tests/e2e/api/test_metrics.pysrc/ingestion/tests/e2e/api/test_persons.pysrc/ingestion/tests/e2e/api/test_session_smoke.pysrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/e2e.shsrc/ingestion/tests/e2e/lib/analytics.pysrc/ingestion/tests/e2e/lib/api_coverage.pysrc/ingestion/tests/e2e/lib/identity_stub.pysrc/ingestion/tests/e2e/lib/metric_coverage.pysrc/ingestion/tests/e2e/pytest.ini
💤 Files with no reviewable changes (1)
- src/ingestion/tests/e2e/api/test_session_smoke.py
Comment on lines
+84
to
+90
| # NB: each lane job reports its `name:` as the status-check context — now | ||
| # "api" and "metrics" (was "E2E suite — api"/"E2E suite — metrics"). Heads-up | ||
| # for whoever wires branch protection: the `main` ruleset ("Soft Protection") | ||
| # still REQUIRES a check named "Run E2E suite" — the pre-split job name — which | ||
| # no job here emits anymore. That required check must be reconciled (point the | ||
| # ruleset at the "api"/"metrics" lanes and/or the coverage gates, or re-add an | ||
| # umbrella "Run E2E suite" job) before this branch can merge. If `build` fails, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale branch-protection note.
Line 88 says no job emits Run E2E suite, but lines 443-444 now add the umbrella job with exactly that check name.
Suggested update
- # still REQUIRES a check named "Run E2E suite" — the pre-split job name — which
- # no job here emits anymore. That required check must be reconciled (point the
- # ruleset at the "api"/"metrics" lanes and/or the coverage gates, or re-add an
- # umbrella "Run E2E suite" job) before this branch can merge.
+ # still REQUIRES a check named "Run E2E suite" — re-supplied by the umbrella
+ # `e2e-suite` job below. Keep that job name in sync with branch protection.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # NB: each lane job reports its `name:` as the status-check context — now | |
| # "api" and "metrics" (was "E2E suite — api"/"E2E suite — metrics"). Heads-up | |
| # for whoever wires branch protection: the `main` ruleset ("Soft Protection") | |
| # still REQUIRES a check named "Run E2E suite" — the pre-split job name — which | |
| # no job here emits anymore. That required check must be reconciled (point the | |
| # ruleset at the "api"/"metrics" lanes and/or the coverage gates, or re-add an | |
| # umbrella "Run E2E suite" job) before this branch can merge. If `build` fails, | |
| # NB: each lane job reports its `name:` as the status-check context — now | |
| # "api" and "metrics" (was "E2E suite — api"/"E2E suite — metrics"). Heads-up | |
| # for whoever wires branch protection: the `main` ruleset ("Soft Protection") | |
| # still REQUIRES a check named "Run E2E suite" — re-supplied by the umbrella | |
| # `e2e-suite` job below. Keep that job name in sync with branch protection. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-bronze-to-api.yml around lines 84 - 90, Update the
stale branch-protection note in the workflow comment block around the E2E lane
jobs: the guidance currently says no job emits “Run E2E suite,” but the workflow
now includes an umbrella job with that exact status-check name. Revise the
wording near the lane/job setup so it reflects the current behavior and no
longer instructs readers to re-add or reconcile a missing umbrella job; keep the
references to the lane names (“api”, “metrics”) and the branch
protection/ruleset guidance accurate.
| working-directory: src/ingestion/tests/e2e | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials in artifact-producing/consuming jobs.
These jobs do not push back to the repo, so keep the GitHub token out of .git/config before running PR-controlled tests or handling artifacts.
Suggested hardening
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: falseAlso applies to: 278-278, 418-418
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 172-177: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-bronze-to-api.yml at line 172, In the workflow jobs
that only produce or consume artifacts, update each actions/checkout@v4 step to
disable persisted credentials so the GitHub token is not written into
.git/config before running PR-controlled tests or artifact handling. Make this
change consistently in all affected jobs in the e2e-bronze-to-api workflow,
keeping the checkout behavior otherwise the same while ensuring credentials are
not retained.
Source: Linters/SAST tools
|
|
||
| # Feature: API Endpoint Contract Suite & Per-Status-Code Coverage Gate | ||
|
|
||
| - [ ] `p1` - **ID**: `cpt-bronze-to-api-e2e-featstatus-api-coverage-gate` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the feature ID typo.
featstatus breaks the shared naming convention and will make cross-references drift from the rest of the docs.
Suggested fix
-- [ ] `p1` - **ID**: `cpt-bronze-to-api-e2e-featstatus-api-coverage-gate`
+- [ ] `p1` - **ID**: `cpt-bronze-to-api-e2e-feature-api-coverage-gate`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [ ] `p1` - **ID**: `cpt-bronze-to-api-e2e-featstatus-api-coverage-gate` | |
| - [ ] `p1` - **ID**: `cpt-bronze-to-api-e2e-feature-api-coverage-gate` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/domain/bronze-to-api-e2e/specs/feature-api-coverage-gate/FEATURE.md` at
line 8, The feature ID in FEATURE.md has a typo in the shared naming convention.
Update the ID string in the feature entry for
cpt-bronze-to-api-e2e-featstatus-api-coverage-gate so it matches the standard
pattern used elsewhere in the docs, and keep the rest of the FEATURE.md entry
unchanged.
Comment on lines
+162
to
+177
| def test_create_403_locked_broader_scope(api, locked_tenant_threshold: str) -> None: | ||
| """A role-scope create for a metric whose tenant scope is locked is refused | ||
| with 403 threshold_locked (the broader lock shadows the narrower write).""" | ||
| r = api.post( | ||
| "/v1/admin/metric-thresholds", | ||
| json={ | ||
| "metric_id": locked_tenant_threshold, | ||
| "scope": "role", | ||
| "role_slug": "e2e-analyst", | ||
| "good": 0.0, | ||
| "warn": 0.0, | ||
| }, | ||
| ) | ||
| assert r.status_code == 403, f"status={r.status_code} body={r.text}" | ||
| if r.status_code != 403: # defensive cleanup only if a row unexpectedly landed | ||
| api.delete(f"/v1/admin/metric-thresholds/{r.json().get('id')}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Defensive cleanup in test_create_403_locked_broader_scope is dead code.
Line 175 asserts r.status_code == 403, so the if r.status_code != 403 on line 176 is either always False (assertion passes) or unreachable (assertion raises). If the endpoint ever returns 201 instead of 403, the role-scope row leaks — and purge_tenant_admin_rows only cleans tenant-scope rows, so it wouldn't be recovered on re-run.
🔧 Move cleanup before the assertion
def test_create_403_locked_broader_scope(api, locked_tenant_threshold: str) -> None:
"""A role-scope create for a metric whose tenant scope is locked is refused
with 403 threshold_locked (the broader lock shadows the narrower write)."""
r = api.post(
"/v1/admin/metric-thresholds",
json={
"metric_id": locked_tenant_threshold,
"scope": "role",
"role_slug": "e2e-analyst",
"good": 0.0,
"warn": 0.0,
},
)
- assert r.status_code == 403, f"status={r.status_code} body={r.text}"
- if r.status_code != 403: # defensive cleanup only if a row unexpectedly landed
- api.delete(f"/v1/admin/metric-thresholds/{r.json().get('id')}")
+ if r.status_code == 201: # defensive cleanup only if a row unexpectedly landed
+ api.delete(f"/v1/admin/metric-thresholds/{r.json()['id']}")
+ assert r.status_code == 403, f"status={r.status_code} body={r.text}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_create_403_locked_broader_scope(api, locked_tenant_threshold: str) -> None: | |
| """A role-scope create for a metric whose tenant scope is locked is refused | |
| with 403 threshold_locked (the broader lock shadows the narrower write).""" | |
| r = api.post( | |
| "/v1/admin/metric-thresholds", | |
| json={ | |
| "metric_id": locked_tenant_threshold, | |
| "scope": "role", | |
| "role_slug": "e2e-analyst", | |
| "good": 0.0, | |
| "warn": 0.0, | |
| }, | |
| ) | |
| assert r.status_code == 403, f"status={r.status_code} body={r.text}" | |
| if r.status_code != 403: # defensive cleanup only if a row unexpectedly landed | |
| api.delete(f"/v1/admin/metric-thresholds/{r.json().get('id')}") | |
| def test_create_403_locked_broader_scope(api, locked_tenant_threshold: str) -> None: | |
| """A role-scope create for a metric whose tenant scope is locked is refused | |
| with 403 threshold_locked (the broader lock shadows the narrower write).""" | |
| r = api.post( | |
| "/v1/admin/metric-thresholds", | |
| json={ | |
| "metric_id": locked_tenant_threshold, | |
| "scope": "role", | |
| "role_slug": "e2e-analyst", | |
| "good": 0.0, | |
| "warn": 0.0, | |
| }, | |
| ) | |
| if r.status_code == 201: # defensive cleanup only if a row unexpectedly landed | |
| api.delete(f"/v1/admin/metric-thresholds/{r.json()['id']}") | |
| assert r.status_code == 403, f"status={r.status_code} body={r.text}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/api/test_admin_thresholds.py` around lines 162 - 177,
`test_create_403_locked_broader_scope` has unreachable defensive cleanup after
the `assert r.status_code == 403`, so the created row would leak if the endpoint
unexpectedly returns 201. Move the cleanup in the test body so it runs before
the status assertion (using the response from `api.post` in
`test_create_403_locked_broader_scope`), and keep the `api.delete` fallback tied
to the returned metric-threshold id rather than placing it behind the assertion.
Comment on lines
+235
to
+248
| def identity_stub(): | ||
| """In-process loopback Identity stub (lib.identity_stub). | ||
|
|
||
| The rig runs no Identity service, so GET /v1/persons/{email} would 500 | ||
| ("identity not configured"). This stub resolves one seeded email (→ 200) and | ||
| 404s the rest, so the persons endpoint exercises its real 200/404 contract | ||
| (#1691). Started before `analytics` (which depends on it) so its URL is known | ||
| when the binary boots — the analytics IdentityClient reads identity_url once | ||
| at gear init. | ||
| """ | ||
| stub = IdentityStub() | ||
| stub.start() | ||
| yield stub | ||
| stub.stop() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Register identity_stub as a pytest fixture.
analytics(..., identity_stub: IdentityStub) depends on this name, but without the decorator pytest will report fixture 'identity_stub' not found.
Suggested fix
+@pytest.fixture(scope="session")
def identity_stub():📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def identity_stub(): | |
| """In-process loopback Identity stub (lib.identity_stub). | |
| The rig runs no Identity service, so GET /v1/persons/{email} would 500 | |
| ("identity not configured"). This stub resolves one seeded email (→ 200) and | |
| 404s the rest, so the persons endpoint exercises its real 200/404 contract | |
| (#1691). Started before `analytics` (which depends on it) so its URL is known | |
| when the binary boots — the analytics IdentityClient reads identity_url once | |
| at gear init. | |
| """ | |
| stub = IdentityStub() | |
| stub.start() | |
| yield stub | |
| stub.stop() | |
| `@pytest.fixture`(scope="session") | |
| def identity_stub(): | |
| """In-process loopback Identity stub (lib.identity_stub). | |
| The rig runs no Identity service, so GET /v1/persons/{email} would 500 | |
| ("identity not configured"). This stub resolves one seeded email (→ 200) and | |
| 404s the rest, so the persons endpoint exercises its real 200/404 contract | |
| (`#1691`). Started before `analytics` (which depends on it) so its URL is known | |
| when the binary boots — the analytics IdentityClient reads identity_url once | |
| at gear init. | |
| """ | |
| stub = IdentityStub() | |
| stub.start() | |
| yield stub | |
| stub.stop() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/conftest.py` around lines 235 - 248, Register the
identity_stub function as a pytest fixture so it can be injected into
analytics(..., identity_stub: IdentityStub) and other tests. Add the pytest
fixture decorator to identity_stub in conftest.py, keeping its setup/teardown
behavior (IdentityStub(), start(), yield, stop()) unchanged so pytest can
resolve the dependency instead of raising fixture not found.
Comment on lines
207
to
+214
| self._proc = subprocess.Popen( | ||
| [str(self.binary), "-c", str(config_path), "run"], | ||
| env=env, | ||
| stdout=subprocess.PIPE, | ||
| stdout=self._log_fh, | ||
| stderr=subprocess.STDOUT, | ||
| text=True, | ||
| ) | ||
| # The gears host inits every system gear + runs migrations/probes, so | ||
| # allow a little more startup headroom than the old bare axum server. | ||
| self._wait_healthy(timeout_s=60.0) | ||
| self._wait_healthy(timeout_s=_HEALTH_TIMEOUT_S) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up the spawned process when startup health-check fails.
If _wait_healthy() raises, analytics() has not yielded yet, so proc.stop() is never reached; the live analytics process and temp log can leak.
Suggested fix
- self._proc = subprocess.Popen(
- [str(self.binary), "-c", str(config_path), "run"],
- env=env,
- stdout=self._log_fh,
- stderr=subprocess.STDOUT,
- text=True,
- )
- self._wait_healthy(timeout_s=_HEALTH_TIMEOUT_S)
+ try:
+ self._proc = subprocess.Popen(
+ [str(self.binary), "-c", str(config_path), "run"],
+ env=env,
+ stdout=self._log_fh,
+ stderr=subprocess.STDOUT,
+ text=True,
+ )
+ self._wait_healthy(timeout_s=_HEALTH_TIMEOUT_S)
+ except Exception:
+ self.stop()
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self._proc = subprocess.Popen( | |
| [str(self.binary), "-c", str(config_path), "run"], | |
| env=env, | |
| stdout=subprocess.PIPE, | |
| stdout=self._log_fh, | |
| stderr=subprocess.STDOUT, | |
| text=True, | |
| ) | |
| # The gears host inits every system gear + runs migrations/probes, so | |
| # allow a little more startup headroom than the old bare axum server. | |
| self._wait_healthy(timeout_s=60.0) | |
| self._wait_healthy(timeout_s=_HEALTH_TIMEOUT_S) | |
| try: | |
| self._proc = subprocess.Popen( | |
| [str(self.binary), "-c", str(config_path), "run"], | |
| env=env, | |
| stdout=self._log_fh, | |
| stderr=subprocess.STDOUT, | |
| text=True, | |
| ) | |
| self._wait_healthy(timeout_s=_HEALTH_TIMEOUT_S) | |
| except Exception: | |
| self.stop() | |
| raise |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 207-207: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/lib/analytics.py` around lines 207 - 214, The startup
path in analytics() launches a subprocess via _proc and immediately calls
_wait_healthy(), but if that health check raises before yielding, the cleanup
path never runs and the spawned process/log handle can leak. Update the
analytics startup flow to wrap the Popen/_wait_healthy sequence with failure
cleanup in the analytics context manager, ensuring the subprocess is stopped and
the log file is closed whenever _wait_healthy() fails; use the existing _proc
and proc.stop() cleanup logic as the reference point.
Comment on lines
+322
to
+327
| @property | ||
| def passed(self) -> bool: | ||
| # Gate blocks ONLY on a documented operation that no test exercises (a | ||
| # new endpoint), plus SKIP_LIST rot. Per-status-code coverage and | ||
| # excluded-set hygiene are REPORTED (advisories), never enforced. | ||
| return not (self.missing or self.redundant_skips or self.stale_skips) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the gate actually block on missing coverable statuses.
self.uncovered is only advisory, so deleting a 400/404 contract while leaving a 200 test still passes the gate. That does not satisfy the status-aware coverage objective.
Suggested direction
- return not (self.missing or self.redundant_skips or self.stale_skips)
+ return not (
+ self.missing
+ or self.redundant_skips
+ or self.stale_skips
+ or self.uncovered
+ )Also add self.uncovered entries to gate_violations() so the failure explains which status codes need tests or justified exclusions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @property | |
| def passed(self) -> bool: | |
| # Gate blocks ONLY on a documented operation that no test exercises (a | |
| # new endpoint), plus SKIP_LIST rot. Per-status-code coverage and | |
| # excluded-set hygiene are REPORTED (advisories), never enforced. | |
| return not (self.missing or self.redundant_skips or self.stale_skips) | |
| `@property` | |
| def passed(self) -> bool: | |
| # Gate blocks ONLY on a documented operation that no test exercises (a | |
| # new endpoint), plus SKIP_LIST rot. Per-status-code coverage and | |
| # excluded-set hygiene are REPORTED (advisories), never enforced. | |
| return not ( | |
| self.missing | |
| or self.redundant_skips | |
| or self.stale_skips | |
| or self.uncovered | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/lib/api_coverage.py` around lines 322 - 327, The gate
logic in passed currently ignores missing coverable status coverage because it
only blocks on missing, redundant_skips, and stale_skips. Update the ApiCoverage
gating path so self.uncovered is treated as a blocking condition as well, and
surface those entries from gate_violations() so failures name the affected
status codes and indicate whether they need tests or justified exclusions. Keep
the change localized to the passed property and gate_violations() in
api_coverage.py.
Comment on lines
223
to
230
| for bare in self.asserted: | ||
| full = by_suffix.get(bare) | ||
| # A fully-qualified dotted key (the response `metric_key` of the | ||
| # unified-metrics path, e.g. `collab_person_counter_daily.messages_sent`) | ||
| # matches the catalog directly; bare column keys resolve via the | ||
| # unique suffix. Without the direct match, dotted assertions were | ||
| # miscounted as unasserted and their keys flagged MISSING. | ||
| full = bare if bare in self.universe else by_suffix.get(bare) | ||
| (self.covered if full else self.unknown_asserted).add(full or bare) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve exact matching for dotted assertions.
full = bare if bare in self.universe else by_suffix.get(bare) still lets a dotted key fall through to suffix matching. That can turn an unknown dotted assertion into a false PASS against the wrong catalog entry.
Suggested fix
- full = bare if bare in self.universe else by_suffix.get(bare)
+ full = bare if bare in self.universe else (
+ by_suffix.get(bare) if "." not in bare else None
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for bare in self.asserted: | |
| full = by_suffix.get(bare) | |
| # A fully-qualified dotted key (the response `metric_key` of the | |
| # unified-metrics path, e.g. `collab_person_counter_daily.messages_sent`) | |
| # matches the catalog directly; bare column keys resolve via the | |
| # unique suffix. Without the direct match, dotted assertions were | |
| # miscounted as unasserted and their keys flagged MISSING. | |
| full = bare if bare in self.universe else by_suffix.get(bare) | |
| (self.covered if full else self.unknown_asserted).add(full or bare) | |
| for bare in self.asserted: | |
| # A fully-qualified dotted key (the response `metric_key` of the | |
| # unified-metrics path, e.g. `collab_person_counter_daily.messages_sent`) | |
| # matches the catalog directly; bare column keys resolve via the | |
| # unique suffix. Without the direct match, dotted assertions were | |
| # miscounted as unasserted and their keys flagged MISSING. | |
| full = bare if bare in self.universe else ( | |
| by_suffix.get(bare) if "." not in bare else None | |
| ) | |
| (self.covered if full else self.unknown_asserted).add(full or bare) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/lib/metric_coverage.py` around lines 223 - 230,
Preserve exact matching for dotted assertions in metric coverage handling: the
current lookup in the assertion loop still allows a dotted `bare` key to fall
through to `by_suffix` matching, which can incorrectly mark the wrong catalog
entry as covered. Update the matching logic in `metric_coverage.py` so the
assertion loop in `self.asserted` only uses suffix resolution for bare column
keys, while dotted keys must resolve strictly through `self.universe` (and
otherwise remain in `self.unknown_asserted`), keeping the existing
`covered`/`unknown_asserted` bookkeeping in `self.covered`.
Comment on lines
+123
to
+136
| ## API endpoint coverage gate | ||
|
|
||
| The suite records which analytics routes it exercises: an httpx response hook on the rig's single client chokepoint (`AnalyticsProcess.client()`) accumulates `(method, path) → {status codes}`, and `conftest.pytest_sessionfinish` dumps the ledger to `.artifacts/observed_endpoints.json` (shipped to CI inside the `coverage-inputs-api` artifact, produced by the `e2e-api` lane). The `api-endpoint-coverage-gate` job diffs it against the committed OpenAPI spec (`docs/components/backend/analytics/openapi.json` — kept accurate by the analytics OpenAPI drift gate: the `openapi_spec_matches_committed` golden test + the `openapi-specs` workflow) via [`lib/api_coverage.py`](lib/api_coverage.py). **Blocking is at the operation level**: the gate fails only when a documented operation is exercised by NO test — a new endpoint added to the spec without a contract test — or when a `SKIP_LIST` entry rots (now exercised, or gone from the spec). It does **not** fail on an individual unobserved status code. | ||
|
|
||
| Locally, after a run: | ||
|
|
||
| ```bash | ||
| ./e2e.sh test api/ # runs the api/ suite (emits both .artifacts files; only observed_endpoints.json feeds this gate) | ||
| ./e2e.sh gates api # endpoint gate only, against .artifacts/ (in the runner image; no DB) | ||
| ``` | ||
|
|
||
| Per-status-code coverage is **reported, not enforced**: the report renders an endpoints × registered-status-codes table (`✓` observed · `✗` declared but not yet observed · `·` excluded · blank = not declared) and an overall coverage percentage. A code is *coverable* — and so counts toward the percentage — only if a black-box rig can produce it: `coverable(op) = declared(op) − {codes ≥ 500} − UNIVERSAL_BOILERPLATE{401,429} − BLOCKED[op]`. `BLOCKED` absorbs the committed spec's `.standard_errors` over-declaration (#1669) plus pinned rig/product limits, and a `·` code that becomes observed (or a `BLOCKED` op dropped from the spec) is surfaced as a non-blocking advisory so the list stays honest. | ||
|
|
||
| The [`api/`](api/) contract suite covers all 21 spec operations — one module per path group (`test_metrics.py`, `test_metric_thresholds.py`, `test_admin_thresholds.py`, `test_catalog.py`, `test_columns.py`, `test_persons.py`, `test_metric_results.py`), one test per (path, method, status-code) case, from self-cleaning fixtures (`api/conftest.py`) — so `SKIP_LIST` is empty and adding a spec operation without a test fails the gate as MISSING. Spec/product gaps are pinned by **strict xfails** rather than fixed here: #1663 (legacy threshold reads 500 once a row exists), #1664 (duplicate admin create answers 500 instead of the declared 409), and #1670 (off-schema legacy body answers a non-canonical 422, not the intended 400). `POST /v1/metric-results` — the unified-metric compute endpoint added by the `feat/unified-metrics` merge (#1656) — is covered on its deterministic error paths (400 empty/bad-period/unknown-key, 415 wrong content-type); its 200 happy-path needs seeded unified-metric observation data and shows as a reported `✗` gap until that fixture lands. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the API gate wording.
This section still says the endpoint gate is operation-level only and that per-status-code coverage is just reported. The new gate is status-aware and blocking, so the README will mislead CI users. Please also reconcile the operation count here with the figure used elsewhere in the PR docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tests/e2e/README.md` around lines 123 - 136, Update the API
endpoint coverage gate description in the README to reflect the new status-aware
blocking behavior instead of saying only operation-level gaps are enforced and
status-code coverage is merely reported. Revise the explanation around
`api-endpoint-coverage-gate`, `lib/api_coverage.py`, and the
`coverable(op)`/coverage table language so CI users understand status-code
misses can fail the gate, then reconcile the listed API operation count here
with the count used in the other PR docs.
The api-coverage gate and constructorfabric#1691 stub accreted essay-length comment blocks that restate the design docs (bronze-to-api-e2e specs) rather than state constraints the code can't. Compress the api_coverage.py module docstring and the constructorfabric#1669/ BLOCKED preamble, the api/__init__.py tail, the thrice-stated identity_url note, and the identity_stub docstring — keeping the load-bearing bits (per-entry BLOCKED code tags, the stub's caller-header/401 rationale, the Person field- match constraint) and correcting a stale EXPECTED_STATUS→BLOCKED reference. Comments only; no logic change (gate still PASS 98% / 0 missing, 21 BLOCKED). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
aleksdotbar
approved these changes
Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Every operation in the committed analytics OpenAPI spec is now exercised by an e2e contract test, and the endpoint-coverage gate is blocking and status-aware — a spec operation added without a test, or a route that stops answering its expected status, fails CI. Test/CI only: this PR contains no product (Rust) changes.
API endpoint contract suite (
src/ingestion/tests/e2e/api/)test_metrics,test_metric_thresholds,test_admin_thresholds,test_catalog,test_columns,test_persons), one test per (path, method, status-code) case — 42 cases, ~3s wall-clock.api/conftest.py:scratch_metric,scratch_threshold,admin_threshold_row,seeded_columns) — the metric catalog is never touched and the yaml rig is undisturbed.query_ref,$orderbyinjection guard,deny_unknown_fields, smuggledtenant_id), 404 unknown/soft-deleted, batch partial-failure envelope (per-item RFC-9457 Problem inside a 200), and the persons op's canonical 500 (no identity service in the rig). 401/403/429 are unreachable by design (auth disabled, no rate limiting).Two product bugs found — pinned, not fixed here
The suite surfaced two analytics bugs, filed separately and pinned by strict xfails so this PR stays test-only:
valuecolumn vsf64entity decode). Success-path threshold cases xfail; temporaryEXPECTED_STATUSoverrides accept the pinned 400/404 error contracts for the four legacy threshold operations.test_create_409_duplicatexfails, pinning the spec'd 409.Self-cleaning: when either fix lands, the strict xfail XPASSes (fails CI) and, for #1663, the first observed 2xx trips the gate's redundant-override hygiene — both force the scaffolding out with the fix.
Coverage gates
api-endpoint-coverage-gate(new CI job): every spec operation must be exercised and seen answering an expected status (declared 2xx by default; explicitEXPECTED_STATUSoverrides otherwise, with redundant/stale/unconstrained hygiene).SKIP_LISTis empty.pytest_sessionfinishdumps a ledger that merges across suite sessions.metric_keyassertions now resolve against the catalog directly (previously miscounted as MISSING).CI layout
The
e2ejob runs two named suites — api and metrics — as separate steps;meta/(rig framework self-tests) is local-only..artifacts/is reset before the suites so the merging ledger can't inherit stale coverage.e2e rig fix
Analytics health-wait is cold-start tolerant: 120s env-overridable ceiling (
E2E_API_HEALTH_TIMEOUT_S) and the child's log streams to a file that is surfaced on a timeout or startup exit (previously an opaque "connection refused" with no logs).Validation
Against the unfixed binary (upstream main), CI-shaped local run (
rm .artifacts && ./e2e.sh test api/ && ./e2e.sh test metrics/ && ./e2e.sh gates):Summary by CodeRabbit
New Features
Bug Fixes
Documentation