Skip to content

test: gate measured coverage and Observatory contracts (#998) - #1111

Merged
iamgp merged 23 commits into
mainfrom
agent/issue-998-testing
Oct 11, 2026
Merged

iamgp merged 23 commits into
mainfrom
agent/issue-998-testing

Conversation

@iamgp

@iamgp iamgp commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Scope

Refs #998, including transferred #1013 checks. Retains the authenticated Node runtime and all forwarding and backend-boundary negatives.

The main-targeted head 3b0f81e655 preserves published history and incorporates actual main through landed PR1106. Earlier frozen prerequisite branches remain unchanged. No unlanded changes are imported.

Verification

Exact-head hosted validation completed successfully: all 25 jobs passed, including measured coverage, required integration, source/file contracts, browser behaviour and pr / required. The synthetic merge candidate has the actual main and this head as its parents.

  • Fresh canonical coverage: 6,620 non-integration passes, four reported existing skips, then seven required disposable-PostgreSQL passes with zero skips. All 40 source-tree line and branch floors pass unchanged. The downloaded hosted canonical coverage artifact independently passes the same floor checker.
  • Full required local integration: eight suites, 122 tests, zero failures or skips. Required PostgreSQL unavailability fails with exit 1 and zero skips. Independent line and branch regression probes each fail with exit 1.
  • Fresh real FastAPI OpenAPI comparison: all 111 automatically discovered production call variants pass, with no removed variants. Retains IncidentView.id rename proof, actual JSON-text helper and inner-schema checks without default-annotation shortcuts, and explicit CSV, NDJSON and no-content contracts.
  • Final make check, frontend 125 tests, production build and inspected browser startup, query result and permission-denial states pass. Browser denial removes the previous result. Caller identity and staging forwarding are asserted.

Corrections without weaker checks

Full-history checkout fixes the pinned hygiene-baseline failure. Public behaviour tests cover previously missed provider branches. Canonical coverage now measures PostgreSQL operation controls on a required real provider instead of excluding them. A runs-envelope test now isolates ambient evidence and checks both absent and populated pagination cursors. Production responses and all coverage thresholds are unchanged.

Real Parquet/DuckDB benchmark tests found a singleton key tail represented by PyIceberg EqualTo rather than In. The benchmark-only writer now handles it; all 11 worker/report checks pass. Production quality/Iceberg behaviour and performance claims are unchanged.

The regression-test-or-justification item remains in the single landed PR1109 template. No duplicate policy is imported.

Limits

See docs/contributing/testing-gates.md for commands and the 66-handler ownership inventory. Browser smoke runs the real built Node application with controlled HTTP, not live Trino or complete product acceptance. Broader product and unrestricted quality acceptance gaps remain with their owning issues. No deployment, production write, settings change or closure of incomplete issues is included.

@iamgp
iamgp changed the base branch from agent/issue-998-contract-base to issue-1035-contract-base October 11, 2026 01:06
iamgp added 4 commits October 11, 2026 01:07
Preserve created follow-up identity and nullable dates. Normalize persisted replay dates through the wire model so the first HTTP response and replay match; prove the declared required fields against disposable PostgreSQL responses.
@iamgp
iamgp changed the base branch from issue-1035-contract-base to issue-1035-text-contract-base October 11, 2026 01:36
@iamgp
iamgp changed the base branch from issue-1035-text-contract-base to issue-1035-effect-contract-base October 11, 2026 09:31
@iamgp
iamgp changed the base branch from issue-1035-effect-contract-base to main October 11, 2026 11:22
@iamgp
iamgp marked this pull request as ready for review October 11, 2026 13:19
@coldtea-pr-lens

Copy link
Copy Markdown

Note

The title starts with test:, so PR Lens left this pull request undrawn. Comment @pr-lens draw to draw it

github.comment.notice: false in .github/pr-lens.yml turns this note off

@iamgp
iamgp added this pull request to the merge queue Oct 11, 2026
@phlo-agent

phlo-agent Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Reviewed the full diff at 3b0f81e (42 files, 2,094 additions) against #998's remaining scope. The gates line up with the issue: measured per-tree line/branch floors with a published report, a real unavailable-service negative test, injected-event replacements for the removed sleeps, integration-path marker enforcement, a per-suite skip summary, the built-app browser smoke, and the method/response drift gate. Two issues in the new Observatory gates are worth fixing before merge.

1. scripts/adapter-boundary.mjs — the shared-credential and provider rules are bypassable

The credential rule only matches a property/element access whose whole text begins with process.env. or process.env[ (L65-L74):

if ((ts.isPropertyAccessExpression(node) || ts.isElementAccessExpression(node)) &&
    /^process\.env(?:\.|\[)/.test(node.getText(tree))) { ... }

const { PHLO_SERVICE_TOKEN } = process.env produces no error: the only relevant node is process.env, whose text is exactly process.env and does not match. const env = process.env; env.PHLO_SERVICE_TOKEN passes the same way. Both read a shared server credential from inside an adapter (or anywhere under src), while the two forms the negative test covers — process.env.PHLO_SERVICE_TOKEN and process.env['PHLO_SERVICE_TOKEN'] — are rejected. Neither bypass has a test case.

Outside src/lib/data/api/ the provider/database rule is a denylist whose alternation must be followed by / or end-of-string (L21-L24), and only the adapter directory gets the allow-list. So import axios from 'axios' (or undici, knex, mysql2, redis, @trinodb/client, @duckdb/node-api) in src/lib/… or src/routes/… is accepted; fetch, new WebSocket and dynamic import() are caught, so the hole is for statically imported clients. Adding the package to package.json satisfies knip, which only checks that imports are declared.

No current file exploits either path, so nothing fails today. The consequence is that a regression this guard exists to stop — reading a shared token, or reaching a provider outside the adapters — lands green. The credential gap is a one-line fix (process.env itself must be rejected outside the transport); the provider gap needs a wider denylist or the adapter allow-list applied across src.

2. boundary.test.mjs can pass without inspecting any Observatory source

expect(checkTree('src')).toEqual([]) (L10) resolves src against the vitest process cwd. That is correct under npm test and CI, which run from the package directory. Run vitest from the repository root instead (npx vitest run --config packages/phlo-observatory/src/phlo_observatory/vitest.config.ts) and checkTree('src') walks the core Python tree, which contains no .ts/.tsx files, so it returns [] — a green guard that inspected nothing. client-contracts.mjs already derives its directory from import.meta.url; the same here removes the failure mode. contracts.test.mjs shares the assumption in execFileSync('uv', …, { cwd: '../../../..' }) (L9-L20).

Minor

npm test now shells out to uv run --locked python from contracts.test.mjs, so it requires a synced Python workspace. The verification matrix still lists npm test as the targeted Observatory frontend check with no such prerequisite, and docs/contributing/testing-gates.md documents the gate without it, so a contributor bootstrapped with only make setup-js gets a failure unrelated to their change.

Validation

This review is static — I executed nothing. The exact-head hosted run is green (pr / required succeeded at 13:17:37Z), and the local counts in the description (6,620 non-integration passes, 125 frontend tests, 122 integration tests) are author-reported rather than independently reproduced. The browser smoke drives the built app against a controlled HTTP fixture, so it is not live FastAPI/Trino acceptance, as the description states. The coverage floors are derived from the same command they gate, so their value is future regressions; they also encode the current four optional skips in that lane, and a skip that flips will move the measurement.

Merged via the queue into main with commit ec77555 Oct 11, 2026
29 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