Skip to content

ci: make Cargo surfaces and the merge gate fail closed#30

Open
Nelson Spence (Fieldnote-Echo) wants to merge 4 commits into
codex/manifest-compat-foundationfrom
codex/ci-merge-gate
Open

ci: make Cargo surfaces and the merge gate fail closed#30
Nelson Spence (Fieldnote-Echo) wants to merge 4 commits into
codex/manifest-compat-foundationfrom
codex/ci-merge-gate

Conversation

@Fieldnote-Echo

@Fieldnote-Echo Fieldnote-Echo commented Jul 25, 2026

Copy link
Copy Markdown
Member

No description provided.

@Fieldnote-Echo
Nelson Spence (Fieldnote-Echo) changed the base branch from feat/ordvec-0.7-deterministic-manifest to codex/manifest-compat-foundation July 25, 2026 18:28

Copy link
Copy Markdown
Member Author

Codex (@codex) review

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

CI: fail-closed Cargo surfaces + merge-gate required-check contract

✨ Enhancement ⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Add a single inventory of all independently locked Cargo graphs and derive CI matrices from it.
• Enforce dependency/lockfile invariants (exact git revs, feature policies, min versions) and
 require --locked everywhere.
• Replace required-check aggregation with a fail-closed merge gate that accepts only an expected job
 set.
Diagram

graph TD
  Inv["cargo-surfaces.json"] --> CSP["cargo_surface_policy.py"] --> Mat["Build/Audit matrices"] --> CI["ci.yml / audit.yml jobs"] --> Gate{"merge-gate"} --> Req["Required check"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Runtime discovery instead of explicit inventory
  • ➕ Less JSON to maintain; surfaces discovered from repo structure automatically
  • ➕ Avoids duplication of commands between inventory and workflows
  • ➖ Harder to make governance intent explicit (feature selectors, publishability, deny config)
  • ➖ Greater risk of accidental surface changes going unnoticed; harder to fail-closed on omissions
2. Use GitHub branch protection only (no merge-gate contract)
  • ➕ No custom merge-gate scripting; relies on native protections
  • ➕ Simpler workflow graph (no aggregator job)
  • ➖ Required-check sets are harder to keep stable across workflow evolution without explicit validation
  • ➖ Does not inherently fail-closed on unexpected/missing jobs unless ruleset is updated in lockstep
3. Replace Python validators with a dedicated CI-policy tool (Rust/Go)
  • ➕ Potentially faster execution and stronger typing
  • ➕ Easier distribution as a single binary for local/CI usage
  • ➖ Higher implementation/maintenance cost than Python in-repo scripts
  • ➖ Bootstrap complexity (toolchain install) and less direct iteration during policy changes

Recommendation: The chosen approach (explicit inventory + validator-driven matrices + fail-closed merge-gate) is the most robust for governance: it makes the set of independently locked graphs and their exact feature/command semantics reviewable, enforces immutable dependency identities, and provides a stable required-check contract without relying on out-of-band ruleset mutations.

Files changed (17) +3113 / -92

Enhancement (2) +1060 / -7
cargo_surface_policy.pyAdd fail-closed Cargo surface validator and matrix emitter +1027/-0

Add fail-closed Cargo surface validator and matrix emitter

• Implements repository policy enforcement for five independently locked Cargo graphs: validates inventory shape, tracked lock/manifests bijection, deny.toml invariants, git dependency allowlists + immutable revs, ordvec identity pinning, serde_json feature policy (declared + production selectors), and minimum dependency versions. Emits build/audit matrices for GitHub Actions and can run surface build commands verbatim.

scripts/cargo_surface_policy.py

release_crate_package_smoke.shHarden package smoke ordering and lock install +33/-7

Harden package smoke ordering and lock install

• Detects the manifest-policy crate name (one of two approved) from cargo metadata and stages it first when present. Adds --locked to cargo install and restructures package ordering to avoid inventing placeholder crates during integration.

scripts/release_crate_package_smoke.sh

Tests (1) +764 / -0
test_ci_policy.pyAdd unit tests for CI policy, surface policy, and merge-gate validators +764/-0

Add unit tests for CI policy, surface policy, and merge-gate validators

• Adds a comprehensive unittest suite covering merge-gate acceptance criteria, Cargo surface policy enforcement (git pinning, feature policy, minimum versions, placeholder rejection), and workflow-policy parsing/validation edge cases.

tests/test_ci_policy.py

Documentation (1) +31 / -0
CARGO_SURFACES.mdDocument Cargo surface inventory and dependency policy rules +31/-0

Document Cargo surface inventory and dependency policy rules

• Adds rationale and operational rules for the cargo-surfaces inventory, including ordvec pinning modes, serde_json feature policy expectations, and fail-closed lock discovery behavior.

.github/CARGO_SURFACES.md

Other (13) +1258 / -85
cargo-surfaces.jsonIntroduce machine-readable inventory for five locked Cargo surfaces +333/-0

Introduce machine-readable inventory for five locked Cargo surfaces

• Defines five Cargo roots (workspace + four consumers) with their manifests/locks, build/audit/metadata commands, feature selectors, publishability, deny config, and supported targets. Encodes dependency policy for ordvec identity, allowed git sources, serde_json feature constraints, and minimum crossbeam-epoch version.

.github/cargo-surfaces.json

ci-policy-requirements.txtPin CI-policy parser dependency with hashes +2/-0

Pin CI-policy parser dependency with hashes

• Adds a hashed requirements file (PyYAML) used by workflow-policy validation in CI.

.github/ci-policy-requirements.txt

merge-gate-expected.jsonDeclare the exact CI job set required by merge-gate +17/-0

Declare the exact CI job set required by merge-gate

• Adds a sorted, machine-readable list of job IDs that merge-gate must observe and require as successful.

.github/merge-gate-expected.json

actionlint.ymlAdd workflow-policy validation to actionlint workflow +7/-0

Add workflow-policy validation to actionlint workflow

• Bootstraps Python + CI policy dependencies and runs scripts/validate_workflow_policy.py after actionlint to fail closed on workflow contract violations.

.github/workflows/actionlint.yml

audit.ymlGenerate cargo-deny matrix from surface inventory and assert immutability +33/-30

Generate cargo-deny matrix from surface inventory and assert immutability

• Adds an inventory job that validates surfaces and emits an audit matrix, then runs cargo-deny per-surface with explicit args/config and --locked. Adds git diff assertions to ensure audits do not modify tracked files.

.github/workflows/audit.yml

ci.ymlAdd surface inventory/build jobs, iOS target checks, and fail-closed merge-gate +134/-51

Add surface inventory/build jobs, iOS target checks, and fail-closed merge-gate

• Introduces inventory-driven build matrices (cargo-surface-*), updates clippy and maturin invocations to use --locked, and converts dependency-policy auditing to a per-surface matrix. Adds macOS iOS target compilation checks and replaces prior aggregators with a merge-gate job that validates an exact success-only needs set.

.github/workflows/ci.yml

nightly-real-embeddings.ymlMake nightly maturin build use --locked +1/-1

Make nightly maturin build use --locked

• Adds --locked to the maturin build invocation to prevent drifting dependency resolution in nightly runs.

.github/workflows/nightly-real-embeddings.yml

release-crates.ymlLock downstream smoke run in release workflow +1/-1

Lock downstream smoke run in release workflow

• Adds --locked to the downstream consumer cargo run to enforce lockfile-based resolution during release validation.

.github/workflows/release-crates.yml

release-pypi.ymlLock sdist build in PyPI release workflow +1/-1

Lock sdist build in PyPI release workflow

• Adds --locked to maturin sdist args so release artifacts are built against the intended Cargo.lock graph.

.github/workflows/release-pypi.yml

deny.tomlDisable implicit all-features in cargo-deny graph configuration +6/-0

Disable implicit all-features in cargo-deny graph configuration

• Sets graph.all-features=false and documents that feature selection is driven per surface by the inventory, enabling explicit BEIR no-default-features auditing.

deny.toml

pi_arm64_profile.shLock downstream smoke run in Pi profiling script +1/-1

Lock downstream smoke run in Pi profiling script

• Adds --locked to the downstream smoke cargo run used in arm64 profiling to keep dependency resolution stable.

scripts/pi_arm64_profile.sh

validate_merge_gate.pyAdd merge-gate validator enforcing exact job set and success-only results +74/-0

Add merge-gate validator enforcing exact job set and success-only results

• Validates that toJSON(needs) contains exactly the expected job keys and that every result is 'success', failing closed on missing/unexpected jobs or non-success outcomes.

scripts/validate_merge_gate.py

validate_workflow_policy.pyAdd structural workflow policy validator (locked commands, stable contexts) +648/-0

Add structural workflow policy validator (locked commands, stable contexts)

• Parses workflow YAML with duplicate-key detection and validates that resolving cargo/maturin invocations use real --locked arguments, recognized actions are pinned by full SHA, immutability jobs end with 'git diff --exit-code HEAD --', and merge-gate needs covers all CI jobs and matches the expected set.

scripts/validate_workflow_policy.py

@qodo-code-review

qodo-code-review Bot commented Jul 25, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Smokes share interpreter state ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New
Description
blocked_egress_smoke.py runs all adapter smoke scripts via runpy in one Python process, so
sys.modules and any framework-level global state persist across scripts. This reduces isolation
versus separate-process runs and can make the blocked-egress guard miss behaviors that only happen
on a fresh import/init per script.
Code

examples/python_adapters/blocked_egress_smoke.py[R82-86]

+    with block_socket_egress():
+        for script in SMOKES:
+            path = ROOT / "examples" / "python_adapters" / script
+            runpy.run_path(str(path), run_name="__main__")
+
Evidence
The runner loops over smoke scripts and executes them with runpy inside a single `with
block_socket_egress()` scope, meaning all scripts share one interpreter and module cache. In
contrast, CI runs each smoke script as a separate Python process, indicating the typical/expected
isolation model for these examples.

examples/python_adapters/blocked_egress_smoke.py[76-86]
.github/workflows/ci.yml[201-213]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`examples/python_adapters/blocked_egress_smoke.py` executes multiple framework smoke scripts in-process via `runpy.run_path(...)`. Because the same interpreter is reused, `sys.modules` caching and any process-global state (framework singletons, global config, monkeypatches, background threads) can leak between scripts, reducing the reliability/coverage of the blocked-egress guard.

### Issue Context
In CI, the edge smokes are also run as separate `python ... script.py` invocations, but the new blocked-egress runner currently does not mirror that isolation.

### Fix Focus Areas
- examples/python_adapters/blocked_egress_smoke.py[76-86]

### Recommended fix
Change the runner to execute each smoke script in a fresh Python subprocess while still applying the socket/DNS block inside that subprocess (e.g., invoke `python -c` that imports a small blocker snippet + `runpy.run_path(script, run_name='__main__')`). This preserves isolation per script while keeping the egress block effective.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Swallowed egress unnoticed ✓ Resolved 🐞 Bug ☼ Reliability
Description
blocked_egress_smoke.py only fails the CI step if BlockedNetworkError propagates out of the adapter
scripts; if a dependency catches the exception, the smoke run can still exit successfully even
though a socket/DNS attempt occurred. This creates false negatives for the new “blocked egress” CI
guard.
Code

examples/python_adapters/blocked_egress_smoke.py[R30-86]

+def _blocked(*args, **kwargs):
+    raise BlockedNetworkError(f"network egress blocked during adapter smoke: args={args!r}")
+
+
+@contextlib.contextmanager
+def block_socket_egress() -> Iterator[None]:
+    original_socket = socket.socket
+    original_create_connection = socket.create_connection
+    original_getaddrinfo = socket.getaddrinfo
+    original_gethostbyname = socket.gethostbyname
+    original_gethostbyname_ex = socket.gethostbyname_ex
+    original_gethostbyaddr = socket.gethostbyaddr
+    original_getnameinfo = socket.getnameinfo
+    original_getfqdn = socket.getfqdn
+
+    class BlockedSocket(original_socket):
+        def connect(self, address):
+            _blocked(address)
+
+        def connect_ex(self, address):
+            _blocked(address)
+
+        def sendto(self, *args, **kwargs):
+            _blocked(*args, **kwargs)
+
+    socket.socket = BlockedSocket
+    socket.create_connection = _blocked
+    socket.getaddrinfo = _blocked
+    socket.gethostbyname = _blocked
+    socket.gethostbyname_ex = _blocked
+    socket.gethostbyaddr = _blocked
+    socket.getnameinfo = _blocked
+    socket.getfqdn = _blocked
+    try:
+        yield
+    finally:
+        socket.socket = original_socket
+        socket.create_connection = original_create_connection
+        socket.getaddrinfo = original_getaddrinfo
+        socket.gethostbyname = original_gethostbyname
+        socket.gethostbyname_ex = original_gethostbyname_ex
+        socket.gethostbyaddr = original_gethostbyaddr
+        socket.getnameinfo = original_getnameinfo
+        socket.getfqdn = original_getfqdn
+
+
+def main() -> None:
+    os.environ.setdefault("HAYSTACK_TELEMETRY_ENABLED", "False")
+    os.environ.setdefault("HAYSTACK_DISABLE_TELEMETRY", "1")
+    os.environ.setdefault("POSTHOG_DISABLED", "1")
+    os.environ.setdefault("AGNO_TELEMETRY", "false")
+
+    with block_socket_egress():
+        for script in SMOKES:
+            path = ROOT / "examples" / "python_adapters" / script
+            runpy.run_path(str(path), run_name="__main__")
+
Evidence
The runner’s blocking shim only raises a custom exception and the main loop does not record or
assert that no blocked calls occurred, so any blocked call that is caught internally will not fail
the process.

examples/python_adapters/blocked_egress_smoke.py[30-32]
examples/python_adapters/blocked_egress_smoke.py[34-74]
examples/python_adapters/blocked_egress_smoke.py[76-86]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`examples/python_adapters/blocked_egress_smoke.py` relies on uncaught exceptions to fail when network is attempted. If an adapter example or a dependency catches the raised exception, the smoke runner may still succeed while egress was attempted.

## Issue Context
The smoke runner is intended to “catch accidental socket use”, but it currently has no accounting of attempted calls; it only fails on uncaught exceptions.

## Fix Focus Areas
- examples/python_adapters/blocked_egress_smoke.py[30-86]

## Suggested fix
- Add a mutable counter/list (e.g., `attempts: list[tuple[str, tuple, dict]]`) in `block_socket_egress()`.
- In `_blocked()`, append attempt details before raising.
- After each `runpy.run_path(...)` (or after the loop), assert `attempts` is empty; if not, raise an AssertionError summarizing which APIs/targets were attempted.
- (Optional) raise an `OSError`-family exception to better match typical network failures, but keep the attempt recording so swallowed exceptions are still surfaced at the end.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Marker parses huge sidecar ✓ Resolved 🐞 Bug ➹ Performance
Description
_json_sidecar_marker_has_schema() fully json.load()s documents.json to detect a marker, but
documents.json contains the entire documents payload; this can cause large, avoidable I/O and memory
use in adapter initialization just to decide whether to call AdapterStore.load(). Because sidecars
are written with sort_keys=True, the schema_version key comes after the full "documents" map, so
this check must deserialize the whole file to verify the marker.
Code

ordinaldb-python/python/ordinaldb/adapters/_common.py[R65-71]

+def _json_sidecar_marker_has_schema(path: Path, schema_version: str) -> bool:
+    try:
+        with path.open("r", encoding="utf-8") as handle:
+            payload = json.load(handle)
+    except (OSError, UnicodeDecodeError, json.JSONDecodeError):
+        return False
+    return isinstance(payload, dict) and payload.get("schema_version") == schema_version
Evidence
The marker helper unconditionally deserializes JSON to check schema_version, while the sidecar
writer persists the entire documents mapping into documents.json and serializes with sort_keys=True,
which places the large "documents" field before "schema_version"; therefore marker checking can
require reading/parsing the full sidecar contents.

ordinaldb-python/python/ordinaldb/adapters/_common.py[65-71]
ordinaldb-python/python/ordinaldb/adapters/_common.py[46-62]
ordinaldb-python/python/ordinaldb/adapters/_common.py[466-483]
ordinaldb-python/python/ordinaldb/adapters/_common.py[789-798]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`adapter_store_markers_exist()` falls back to `_json_sidecar_marker_has_schema()` which does a full `json.load()` of `documents.json` / `metadata.json`. `documents.json` can be very large (it contains the full document text mapping), so this adds unnecessary I/O + memory overhead on adapter construction.

## Issue Context
Marker detection should be cheap; if the goal is “fail closed” (avoid creating a new store on top of a partial/corrupt store), it’s sufficient to treat the presence of known sidecar filenames as a marker without fully parsing their contents.

## Fix Focus Areas
- ordinaldb-python/python/ordinaldb/adapters/_common.py[46-72]
- ordinaldb-python/python/ordinaldb/adapters/_common.py[466-494]
- ordinaldb-python/python/ordinaldb/adapters/_common.py[789-798]

### Suggested fix direction
- Replace the schema-parsing marker check with a cheap existence check (e.g., `return (root / DOCUMENTS_FILE).is_file() or (root / METADATA_FILE).is_file()`), or at minimum avoid parsing `documents.json` (treat it as a marker if it exists).
- Update `test_adapter_store_markers_detect_partial_existing_store` expectations accordingly (it currently asserts `False` for invalid JSON sidecars).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. Sidecar-only store undetected ✓ Resolved 🐞 Bug ☼ Reliability
Description
adapter_store_markers_exist() ignores documents.json/metadata.json, so an adapter directory that
only has those valid sidecars (e.g., partial copy/extract) will be treated as empty and adapters
will create a new store instead of failing closed. A later AdapterStore.save() can replace the
directory and discard those existing sidecars.
Code

ordinaldb-python/python/ordinaldb/adapters/_common.py[R46-58]

+def adapter_store_markers_exist(path: str | os.PathLike[str] | None) -> bool:
+    if path is None:
+        return False
+    root = Path(path)
+    return any(
+        (root / marker).exists()
+        for marker in (
+            ADAPTER_FILE,
+            ID_MAP_FILE,
+            INDEX_DIR,
+            ADAPTER_STORE_FILE,
+        )
+    )
Evidence
The repo documents documents.json/metadata.json as standard adapter-store artifacts and
AdapterStore.save() replaces the full directory, so mis-detecting a partially present directory
can lead to later replacement that discards those sidecars.

docs/persistence.md[66-84]
ordinaldb-python/python/ordinaldb/adapters/_common.py[32-58]
ordinaldb-python/python/ordinaldb/adapters/_common.py[453-481]
ordinaldb-python/python/ordinaldb/adapters/_common.py[897-913]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`adapter_store_markers_exist()` currently only checks for `adapter.json`, `id_map.json`, `index.odb/`, and `adapter.redb`. This can misclassify a partially-present adapter directory as “empty” when it contains only `documents.json`/`metadata.json` sidecars, which are part of the official adapter-store layout.

## Issue Context
- Adapter directories officially include `documents.json` and `metadata.json` sidecars.
- `AdapterStore.save()` replaces the entire target directory atomically (rename + delete backup), so initializing a fresh store against a partially-present directory can later discard those sidecars.
- The existing unit test intentionally avoids treating arbitrary `metadata.json` / `documents.json` files as markers; keep that goal by only treating them as markers when they look like *OrdinalDB adapter sidecars* (schema_version matches).

## Fix Focus Areas
- ordinaldb-python/python/ordinaldb/adapters/_common.py[46-58]
- ordinaldb-python/python/ordinaldb/adapters/_common.py[727-745]
- ordinaldb-python/tests/test_adapters_common.py[60-73]

## Suggested fix approach
1. Extend `adapter_store_markers_exist()` to also consider `documents.json` and `metadata.json` **only if** they parse as JSON objects and their `schema_version` equals `DOCUMENTS_SCHEMA_VERSION` / `METADATA_SCHEMA_VERSION`.
2. Update/add tests:
  - Keep the existing assertions that random/invalid `metadata.json` (`{}`) and `documents.json` (`[]`) do **not** trigger detection.
  - Add a new assertion that correctly-shaped `documents.json`/`metadata.json` (with proper `schema_version`) **does** trigger detection.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

5. DNS blocking incomplete ✓ Resolved 🐞 Bug ≡ Correctness
Description
blocked_egress_smoke.py is documented as blocking “DNS entry points”, but it only patches
getaddrinfo/gethostbyname/gethostbyname_ex and leaves other resolver APIs unpatched (e.g.,
gethostbyaddr/getnameinfo). This reduces CI’s ability to catch accidental DNS/network lookups in
adapter examples.
Code

examples/python_adapters/blocked_egress_smoke.py[R34-56]

+@contextlib.contextmanager
+def block_socket_egress() -> Iterator[None]:
+    original_socket = socket.socket
+    original_create_connection = socket.create_connection
+    original_getaddrinfo = socket.getaddrinfo
+    original_gethostbyname = socket.gethostbyname
+    original_gethostbyname_ex = socket.gethostbyname_ex
+
+    class BlockedSocket(original_socket):
+        def connect(self, address):
+            _blocked(address)
+
+        def connect_ex(self, address):
+            _blocked(address)
+
+        def sendto(self, *args, **kwargs):
+            _blocked(*args, **kwargs)
+
+    socket.socket = BlockedSocket
+    socket.create_connection = _blocked
+    socket.getaddrinfo = _blocked
+    socket.gethostbyname = _blocked
+    socket.gethostbyname_ex = _blocked
Evidence
Documentation claims DNS entry points are blocked, but the smoke runner only patches a subset of
socket DNS/resolution functions, leaving gaps in what it can detect.

docs/edge-deployment.md[126-129]
examples/python_adapters/blocked_egress_smoke.py[34-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The blocked-egress smoke is described as blocking “Python socket connection and DNS entry points”, but the implementation only patches a subset of socket resolver functions. This can allow some DNS-related lookups to slip through the smoke test.

## Issue Context
Current patches:
- `socket.socket`, `socket.create_connection`
- `socket.getaddrinfo`, `socket.gethostbyname`, `socket.gethostbyname_ex`

Not patched (examples):
- `socket.gethostbyaddr`
- `socket.getnameinfo`
- (optionally) `socket.getfqdn` (can trigger reverse lookups via gethostbyaddr)

## Fix Focus Areas
- examples/python_adapters/blocked_egress_smoke.py[34-65]
- docs/edge-deployment.md[126-129]

## Suggested fix approach
1. Patch and restore additional resolver APIs (`gethostbyaddr`, `getnameinfo`, optionally `getfqdn`) in `block_socket_egress()`.
2. Alternatively (or additionally), narrow the documentation wording if you intentionally only want to block forward-lookup entry points.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 0fe9ed0

Results up to commit 137345b


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Sidecar-only store undetected ✓ Resolved 🐞 Bug ☼ Reliability
Description
adapter_store_markers_exist() ignores documents.json/metadata.json, so an adapter directory that
only has those valid sidecars (e.g., partial copy/extract) will be treated as empty and adapters
will create a new store instead of failing closed. A later AdapterStore.save() can replace the
directory and discard those existing sidecars.
Code

ordinaldb-python/python/ordinaldb/adapters/_common.py[R46-58]

+def adapter_store_markers_exist(path: str | os.PathLike[str] | None) -> bool:
+    if path is None:
+        return False
+    root = Path(path)
+    return any(
+        (root / marker).exists()
+        for marker in (
+            ADAPTER_FILE,
+            ID_MAP_FILE,
+            INDEX_DIR,
+            ADAPTER_STORE_FILE,
+        )
+    )
Evidence
The repo documents documents.json/metadata.json as standard adapter-store artifacts and
AdapterStore.save() replaces the full directory, so mis-detecting a partially present directory
can lead to later replacement that discards those sidecars.

docs/persistence.md[66-84]
ordinaldb-python/python/ordinaldb/adapters/_common.py[32-58]
ordinaldb-python/python/ordinaldb/adapters/_common.py[453-481]
ordinaldb-python/python/ordinaldb/adapters/_common.py[897-913]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`adapter_store_markers_exist()` currently only checks for `adapter.json`, `id_map.json`, `index.odb/`, and `adapter.redb`. This can misclassify a partially-present adapter directory as “empty” when it contains only `documents.json`/`metadata.json` sidecars, which are part of the official adapter-store layout.

## Issue Context
- Adapter directories officially include `documents.json` and `metadata.json` sidecars.
- `AdapterStore.save()` replaces the entire target directory atomically (rename + delete backup), so initializing a fresh store against a partially-present directory can later discard those sidecars.
- The existing unit test intentionally avoids treating arbitrary `metadata.json` / `documents.json` files as markers; keep that goal by only treating them as markers when they look like *OrdinalDB adapter sidecars* (schema_version matches).

## Fix Focus Areas
- ordinaldb-python/python/ordinaldb/adapters/_common.py[46-58]
- ordinaldb-python/python/ordinaldb/adapters/_common.py[727-745]
- ordinaldb-python/tests/test_adapters_common.py[60-73]

## Suggested fix approach
1. Extend `adapter_store_markers_exist()` to also consider `documents.json` and `metadata.json` **only if** they parse as JSON objects and their `schema_version` equals `DOCUMENTS_SCHEMA_VERSION` / `METADATA_SCHEMA_VERSION`.
2. Update/add tests:
  - Keep the existing assertions that random/invalid `metadata.json` (`{}`) and `documents.json` (`[]`) do **not** trigger detection.
  - Add a new assertion that correctly-shaped `documents.json`/`metadata.json` (with proper `schema_version`) **does** trigger detection.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational
2. DNS blocking incomplete ✓ Resolved 🐞 Bug ≡ Correctness
Description
blocked_egress_smoke.py is documented as blocking “DNS entry points”, but it only patches
getaddrinfo/gethostbyname/gethostbyname_ex and leaves other resolver APIs unpatched (e.g.,
gethostbyaddr/getnameinfo). This reduces CI’s ability to catch accidental DNS/network lookups in
adapter examples.
Code

examples/python_adapters/blocked_egress_smoke.py[R34-56]

+@contextlib.contextmanager
+def block_socket_egress() -> Iterator[None]:
+    original_socket = socket.socket
+    original_create_connection = socket.create_connection
+    original_getaddrinfo = socket.getaddrinfo
+    original_gethostbyname = socket.gethostbyname
+    original_gethostbyname_ex = socket.gethostbyname_ex
+
+    class BlockedSocket(original_socket):
+        def connect(self, address):
+            _blocked(address)
+
+        def connect_ex(self, address):
+            _blocked(address)
+
+        def sendto(self, *args, **kwargs):
+            _blocked(*args, **kwargs)
+
+    socket.socket = BlockedSocket
+    socket.create_connection = _blocked
+    socket.getaddrinfo = _blocked
+    socket.gethostbyname = _blocked
+    socket.gethostbyname_ex = _blocked
Evidence
Documentation claims DNS entry points are blocked, but the smoke runner only patches a subset of
socket DNS/resolution functions, leaving gaps in what it can detect.

docs/edge-deployment.md[126-129]
examples/python_adapters/blocked_egress_smoke.py[34-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The blocked-egress smoke is described as blocking “Python socket connection and DNS entry points”, but the implementation only patches a subset of socket resolver functions. This can allow some DNS-related lookups to slip through the smoke test.

## Issue Context
Current patches:
- `socket.socket`, `socket.create_connection`
- `socket.getaddrinfo`, `socket.gethostbyname`, `socket.gethostbyname_ex`

Not patched (examples):
- `socket.gethostbyaddr`
- `socket.getnameinfo`
- (optionally) `socket.getfqdn` (can trigger reverse lookups via gethostbyaddr)

## Fix Focus Areas
- examples/python_adapters/blocked_egress_smoke.py[34-65]
- docs/edge-deployment.md[126-129]

## Suggested fix approach
1. Patch and restore additional resolver APIs (`gethostbyaddr`, `getnameinfo`, optionally `getfqdn`) in `block_socket_egress()`.
2. Alternatively (or additionally), narrow the documentation wording if you intentionally only want to block forward-lookup entry points.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 5e6fa95


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Marker parses huge sidecar ✓ Resolved 🐞 Bug ➹ Performance
Description
_json_sidecar_marker_has_schema() fully json.load()s documents.json to detect a marker, but
documents.json contains the entire documents payload; this can cause large, avoidable I/O and memory
use in adapter initialization just to decide whether to call AdapterStore.load(). Because sidecars
are written with sort_keys=True, the schema_version key comes after the full "documents" map, so
this check must deserialize the whole file to verify the marker.
Code

ordinaldb-python/python/ordinaldb/adapters/_common.py[R65-71]

+def _json_sidecar_marker_has_schema(path: Path, schema_version: str) -> bool:
+    try:
+        with path.open("r", encoding="utf-8") as handle:
+            payload = json.load(handle)
+    except (OSError, UnicodeDecodeError, json.JSONDecodeError):
+        return False
+    return isinstance(payload, dict) and payload.get("schema_version") == schema_version
Evidence
The marker helper unconditionally deserializes JSON to check schema_version, while the sidecar
writer persists the entire documents mapping into documents.json and serializes with sort_keys=True,
which places the large "documents" field before "schema_version"; therefore marker checking can
require reading/parsing the full sidecar contents.

ordinaldb-python/python/ordinaldb/adapters/_common.py[65-71]
ordinaldb-python/python/ordinaldb/adapters/_common.py[46-62]
ordinaldb-python/python/ordinaldb/adapters/_common.py[466-483]
ordinaldb-python/python/ordinaldb/adapters/_common.py[789-798]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`adapter_store_markers_exist()` falls back to `_json_sidecar_marker_has_schema()` which does a full `json.load()` of `documents.json` / `metadata.json`. `documents.json` can be very large (it contains the full document text mapping), so this adds unnecessary I/O + memory overhead on adapter construction.

## Issue Context
Marker detection should be cheap; if the goal is “fail closed” (avoid creating a new store on top of a partial/corrupt store), it’s sufficient to treat the presence of known sidecar filenames as a marker without fully parsing their contents.

## Fix Focus Areas
- ordinaldb-python/python/ordinaldb/adapters/_common.py[46-72]
- ordinaldb-python/python/ordinaldb/adapters/_common.py[466-494]
- ordinaldb-python/python/ordinaldb/adapters/_common.py[789-798]

### Suggested fix direction
- Replace the schema-parsing marker check with a cheap existence check (e.g., `return (root / DOCUMENTS_FILE).is_file() or (root / METADATA_FILE).is_file()`), or at minimum avoid parsing `documents.json` (treat it as a marker if it exists).
- Update `test_adapter_store_markers_detect_partial_existing_store` expectations accordingly (it currently asserts `False` for invalid JSON sidecars).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 84c6b11


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Swallowed egress unnoticed ✓ Resolved 🐞 Bug ☼ Reliability
Description
blocked_egress_smoke.py only fails the CI step if BlockedNetworkError propagates out of the adapter
scripts; if a dependency catches the exception, the smoke run can still exit successfully even
though a socket/DNS attempt occurred. This creates false negatives for the new “blocked egress” CI
guard.
Code

examples/python_adapters/blocked_egress_smoke.py[R30-86]

+def _blocked(*args, **kwargs):
+    raise BlockedNetworkError(f"network egress blocked during adapter smoke: args={args!r}")
+
+
+@contextlib.contextmanager
+def block_socket_egress() -> Iterator[None]:
+    original_socket = socket.socket
+    original_create_connection = socket.create_connection
+    original_getaddrinfo = socket.getaddrinfo
+    original_gethostbyname = socket.gethostbyname
+    original_gethostbyname_ex = socket.gethostbyname_ex
+    original_gethostbyaddr = socket.gethostbyaddr
+    original_getnameinfo = socket.getnameinfo
+    original_getfqdn = socket.getfqdn
+
+    class BlockedSocket(original_socket):
+        def connect(self, address):
+            _blocked(address)
+
+        def connect_ex(self, address):
+            _blocked(address)
+
+        def sendto(self, *args, **kwargs):
+            _blocked(*args, **kwargs)
+
+    socket.socket = BlockedSocket
+    socket.create_connection = _blocked
+    socket.getaddrinfo = _blocked
+    socket.gethostbyname = _blocked
+    socket.gethostbyname_ex = _blocked
+    socket.gethostbyaddr = _blocked
+    socket.getnameinfo = _blocked
+    socket.getfqdn = _blocked
+    try:
+        yield
+    finally:
+        socket.socket = original_socket
+        socket.create_connection = original_create_connection
+        socket.getaddrinfo = original_getaddrinfo
+        socket.gethostbyname = original_gethostbyname
+        socket.gethostbyname_ex = original_gethostbyname_ex
+        socket.gethostbyaddr = original_gethostbyaddr
+        socket.getnameinfo = original_getnameinfo
+        socket.getfqdn = original_getfqdn
+
+
+def main() -> None:
+    os.environ.setdefault("HAYSTACK_TELEMETRY_ENABLED", "False")
+    os.environ.setdefault("HAYSTACK_DISABLE_TELEMETRY", "1")
+    os.environ.setdefault("POSTHOG_DISABLED", "1")
+    os.environ.setdefault("AGNO_TELEMETRY", "false")
+
+    with block_socket_egress():
+        for script in SMOKES:
+            path = ROOT / "examples" / "python_adapters" / script
+            runpy.run_path(str(path), run_name="__main__")
+
Evidence
The runner’s blocking shim only raises a custom exception and the main loop does not record or
assert that no blocked calls occurred, so any blocked call that is caught internally will not fail
the process.

examples/python_adapters/blocked_egress_smoke.py[30-32]
examples/python_adapters/blocked_egress_smoke.py[34-74]
examples/python_adapters/blocked_egress_smoke.py[76-86]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`examples/python_adapters/blocked_egress_smoke.py` relies on uncaught exceptions to fail when network is attempted. If an adapter example or a dependency catches the raised exception, the smoke runner may still succeed while egress was attempted.

## Issue Context
The smoke runner is intended to “catch accidental socket use”, but it currently has no accounting of attempted calls; it only fails on uncaught exceptions.

## Fix Focus Areas
- examples/python_adapters/blocked_egress_smoke.py[30-86]

## Suggested fix
- Add a mutable counter/list (e.g., `attempts: list[tuple[str, tuple, dict]]`) in `block_socket_egress()`.
- In `_blocked()`, append attempt details before raising.
- After each `runpy.run_path(...)` (or after the loop), assert `attempts` is empty; if not, raise an AssertionError summarizing which APIs/targets were attempted.
- (Optional) raise an `OSError`-family exception to better match typical network failures, but keep the attempt recording so swallowed exceptions are still surfaced at the end.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread .github/ci-policy-requirements.txt Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6dfa627d72

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread scripts/validate_workflow_policy.py
Comment thread scripts/validate_workflow_policy.py Outdated
Comment thread scripts/release_crate_package_smoke.sh

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 442b120693

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread scripts/validate_workflow_policy.py
Comment thread scripts/validate_workflow_policy.py
Comment thread scripts/cargo_surface_policy.py Outdated
Comment thread scripts/cargo_surface_policy.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a77fa6cd9a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread scripts/validate_workflow_policy.py
Comment thread scripts/cargo_surface_policy.py Outdated
Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
@Fieldnote-Echo

Copy link
Copy Markdown
Member Author

Corrected CI/governance layer pushed at c05ae00655107610f56415542f90caa446984ea4, rebased on PR #31 head 1bd4c19d3eb56d2291a5002256da9e56d4082ef8.

The latest review findings are fixed: GitHub-expression command construction fails closed, legitimate repeated Cargo options are representable, and the earlier workflow/package-policy bypasses remain covered. Local proof: 76 policy tests, workflow validator, five-surface inventory validator, exact ordvec 0.6.0 source policy, Ruff, formatting, and diff checks all pass.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c05ae00655

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread .github/workflows/actionlint.yml
Comment thread scripts/cargo_surface_policy.py Outdated
Signed-off-by: Nelson Spence <nelson@projectnavi.ai>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c712baa35a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment thread scripts/reconcile_sdist_lock.py
Comment thread scripts/validate_workflow_policy.py
Comment thread scripts/validate_workflow_policy.py Outdated
Comment thread scripts/cargo_surface_policy.py Outdated
Signed-off-by: Nelson Spence <nelson@projectnavi.ai>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 176451e89c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread scripts/validate_workflow_policy.py
Comment thread scripts/validate_workflow_policy.py Outdated
Signed-off-by: Nelson Spence <nelson@projectnavi.ai>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0fe9ed01da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

if not isinstance(steps, list) or not steps:
fail(f"{path.name}: protected job {job_id!r} must have a non-empty step list")
payload = json.dumps(
_plain_yaml(steps),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Bind inventory outputs to the generated matrices

Include each protected inventory job's outputs mapping in this digest or validate it separately. Because only steps are hashed and no later check validates producer outputs, a workflow change can hardcode build_matrix and audit_matrix to a single valid surface while leaving the reviewed inventory steps untouched; validation still checks all five surfaces, but the build/audit consumers receive only the narrowed matrix and the merge gate can remain green.

Useful? React with 👍 / 👎.

Comment on lines +2097 to +2098
errors.extend(_protected_job_step_sequence_errors(path, job_id, job))
if "if" in job:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject dependencies on stable required-context jobs

Require the actionlint and zizmor jobs to omit needs (or validate their complete dependency chain). Fresh evidence beyond the earlier job-level if fix is that adding needs: blocker leaves this validation and the protected step digest unchanged; if blocker fails or is skipped, GitHub skips the required-context job before any audit or policy-validator step runs, allowing the required context to conclude as skipped rather than enforcing the checks.

Useful? React with 👍 / 👎.

Comment on lines +2175 to +2179
if isinstance(steps, list) and any(
isinstance(step, MarkedDict) and "continue-on-error" in step
for step in steps
):
errors.append(f"CI job {job_id!r} contains a continue-on-error step")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require the exact dependency-policy validator

Structurally require an unconditional execution of tests/dependency_policy.py and protect its release-smoke wrapper. The exact TURBOVEC_REVISION and comparator feature boundary are enforced only by that script, but neither the lint nor package-integration proof step is protected and this global pass rejects only continue-on-error; a PR can add if: ${{ false }} to both invocations, change BEIR's turbovec dependency to any other full SHA from the allowlisted repository, and still pass the Cargo-surface validator, audits, and merge gate.

Useful? React with 👍 / 👎.

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