feat(a4): persist session-bound search issues - #398
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds a session-scoped A4 Issue forwarding route, a coordinator-only governance endpoint, replay-safe A4 evidence persistence, stricter internal authentication and proof verification, bounded A4 search responses, and extensive integration and persistence tests. A4 Issue Flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Coordinator
participant GovernanceAPI
participant ProofRegistry
participant IssueStore
Client->>Coordinator: Submit session-scoped A4 issue draft
Coordinator->>Coordinator: Authenticate and validate binding
Coordinator->>GovernanceAPI: Forward trusted A4 request
GovernanceAPI->>ProofRegistry: Verify proof and snapshot
GovernanceAPI->>IssueStore: Create or replay issue
IssueStore-->>GovernanceAPI: Issue and replay status
GovernanceAPI-->>Coordinator: Issue response
Coordinator-->>Client: Sanitized response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements the S4-C backend slice of the a4-semantic-search-model-qa OpenSpec change: it adds an end-to-end path for turning a trusted A4 semantic-search result row into a persisted governance Issue. A new coordinator-only route (/api/governance/issues/from-a4-search/for-session/{session_id}) reauthorizes the current session/principal, strips browser-supplied authority, and forwards to a new governance route (/api/internal/a4/issues/from-search) that verifies a signed row proof and atomically persists immutable A4 evidence. It fits into the existing coordinator→governance loopback boundary and the scoped-only A4 search design, keeping the production-mounted mutation fail-closed (503) while the authentic lease capability remains lab_unverified.
Changes:
- Proof hardening: embeds authenticated expiry + snapshot-hash claims inside the opaque proof id (restart/rotation-safe verification), adds per-binding/per-principal quotas, and JSON-wire-stable numeric normalization so Python↔Node round-trips preserve signed bytes.
- Persistence + replay: new additive
a4_issue_evidencetable with three digests (snapshot_hash,proof_digest,creation_request_hash), single-transaction create, constant-time exact-replay that returns the original Issue even after key retirement, and authorization-before-digest ordering to avoid a proof-existence oracle. - Surface hiding + bounded responses: generic Issue list/detail/transition fail closed on
a4_searchrecords; search responses are size-bounded (row projection, honest truncation, proof attachment budgeting) and share printable-ASCII internal-token validation.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
governance-service/search/proofs.py |
Embedded-claim proof ids, quota accounting/release, restart-safe verify, wire-stable snapshot normalization, discard() |
governance-service/search/engine.py |
Response byte budgeting, bounded value projection, proof attachment loop, corrected degraded_to_deterministic semantics |
governance-service/search/internal_auth.py |
New shared fail-closed internal-token validation helpers |
governance-service/search/api.py |
Uses shared internal_auth; removes duplicated token logic |
governance-service/issues/store.py |
a4_issue_evidence schema, atomic create_a4_issue, find_a4_issue_replay, A4 hiding in list_issues |
governance-service/issues/api.py |
Hides A4 records from generic get/transition endpoints |
governance-service/issues/a4_api.py |
New coordinator-only A4 Issue creation route with snapshot/binding validation |
governance-service/issues/__init__.py |
Exports new A4 exceptions |
governance-service/app.py |
Mounts the A4 Issue router |
bim-review-coordinator/src/routes/a4IssueRoutes.ts |
New scoped session route: authorization, draft sanitization, trusted-context injection |
bim-review-coordinator/src/routes/a4SearchRoutes.ts |
Exports forwardTrustedA4/GovernanceTimeoutBudget; adds exact-echo allowlist and ASCII token check |
bim-review-coordinator/src/app.ts |
Wires the A4 Issue route before the generic proxy |
governance-service/README.md, bim-review-coordinator/README.md |
Document the new route and token constraints |
openspec/changes/.../tasks.md |
Marks tasks 4.1–4.4/4.7 complete with a scope note |
governance-service/tests/*, bim-review-coordinator/tests/* |
Extensive new/updated coverage (persistence, replay, quotas, budgeting, coordinator forwarding) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@bim-review-coordinator/src/routes/a4IssueRoutes.ts`:
- Around line 353-362: Remove collectExactStringEchoes from the
allowedResponseEchoes construction in the route handling the draft value, and
retain only the explicit title, description, and assignee strings. Ensure no
other browser-controlled a4_evidence_snapshot fields are added to the echo
allowlist, while preserving the existing normalization and optional-field
behavior.
🪄 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 Plus
Run ID: 60345739-1020-4ca6-802c-acd8c2505ab9
📒 Files selected for processing (20)
bim-review-coordinator/README.mdbim-review-coordinator/src/app.tsbim-review-coordinator/src/routes/a4IssueRoutes.tsbim-review-coordinator/src/routes/a4SearchRoutes.tsbim-review-coordinator/tests/governance-issue-from-a4-session.test.tsbim-review-coordinator/tests/governance-search-for-session.test.tsgovernance-service/README.mdgovernance-service/app.pygovernance-service/issues/__init__.pygovernance-service/issues/a4_api.pygovernance-service/issues/api.pygovernance-service/issues/store.pygovernance-service/search/api.pygovernance-service/search/engine.pygovernance-service/search/internal_auth.pygovernance-service/search/proofs.pygovernance-service/tests/test_a4_issues.pygovernance-service/tests/test_search_handoff_api.pygovernance-service/tests/test_search_model.pyopenspec/changes/a4-semantic-search-model-qa/tasks.md
monkey1sai
left a comment
monkey1sai
left a comment
There was a problem hiding this comment.
Final risk-loop review
Verdict: Accept — no unresolved blocking findings.
- The response-echo allowlist concern was fixed in
dc20f52: browser-controlled snapshot strings are no longer recursively allowlisted; only normalized title, description, and assignee echoes remain. - Regression coverage now proves a path-like snapshot value is sanitized to
502; the CodeRabbit thread is resolved. - Local gates on the final head: coordinator build + 65 files / 724 tests; targeted Issue suite 7/7; PR preflight passed;
git diff --checkpassed. - GitHub required checks on
dc20f52are green; PR is mergeable and clean.
Non-blocking gaps: Ruff is unavailable in this environment; browser/Kit/live-model/design gates were not applicable to this backend-only slice, so full user-facing completion is not claimed. The broad PR remains GitNexus CRITICAL by scope, covered by the recorded explicit sign-off; the repair-specific delta is LOW.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8bf0412d9
ℹ️ 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 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 address that feedback".
| elif accepted_prim is not None or row_prim is not None or body.usd_prim_path is not None: | ||
| raise _error(422, "a4_issue_mapping_mismatch") |
There was a problem hiding this comment.
When a search runs with element_mapping_path missing or rejected as fake, search/engine.py only records path_missing/rejected_fake in top-level evidence_refs; the signed row snapshot still has mapping_observed=false and null prims. This branch then lets that snapshot fall through as valid, so the new A4 Issue route can persist A4 provenance for rows whose mapping source was not actually accepted. Include the mapping-join status in the signed snapshot and reject non-accepted mapping states before creating the Issue.
AGENTS.md reference: governance-service/AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
| deps, | ||
| "/api/internal/a4/issues/from-search", | ||
| "deterministic", | ||
| { ...draft.value, a4_trusted_context: trusted }, |
There was a problem hiding this comment.
If the primary lease or active stage binding changes after resolveSession/snapshotMatchesCurrentBinding but before the governance call commits, this forwards the stale trusted object and governance has no way to know it is no longer current. In that race, an A4 Issue can be persisted for a proof tied to a previous artifact/revision/principal authorization; re-resolve and compare the binding after the upstream verification window, or make the create conditional on a fresh current binding.
AGENTS.md reference: bim-review-coordinator/AGENTS.md:L18-L19
Useful? React with 👍 / 👎.
| model_version_id=None, | ||
| kind=None, | ||
| *, | ||
| include_a4: bool = False, |
There was a problem hiding this comment.
Because include_a4 now defaults to false at the store layer, existing internal callers such as /api/bcf/export and /api/diffs/{diff_id}/issue-impact also silently drop confirmed A4 records even though they are stored as formal kind='issue' rows with ifc_guid and model_version_id. If the goal is only to hide generic list/detail/transition until session-authorized lifecycle routes exist, pass an explicit include flag or add authorized read paths for BCF export and diff impact; otherwise user-confirmed A4 issues will be missing from governance outputs.
AGENTS.md reference: governance-service/AGENTS.md:L14-L16
Useful? React with 👍 / 👎.
* docs(evidence): record PR 398 test-deploy risk verification * docs(evidence): complete PR 398 test deploy verification * docs(evidence): clarify PR 398 verification scope
Summary
lab_unverified.Scope and completion
This is the S4-C backend slice of
a4-semantic-search-model-qa. OpenSpec tasks 4.1-4.4 and 4.7 are complete. Task 3.8 remains open because the production resolver cannot yet produce an authentic verified lease capability. Task 4.5 still needs browser-memory draft recovery/recheck UI, and task 4.6 still needs the expiry-plus-clock-skew key-retirement operational contract.Full completion claimed: no.
AI Coding Governance
a4-semantic-search-model-qa; no standalone GitHub issuedetect_changes risk=CRITICAL: 20 files, 224 symbols, 165 execution flows on final head; explicit user sign-off recorded; correctness and security reviewers found no P1/P2lab_unverifiedDeploy Path Verification
npm run verifyinbim-review-coordinator;python -m pytest tests/ -q -p no:cacheprovideringovernance-servicef8bf041anddc20f52; affected tests underbim-review-coordinator/tests/andgovernance-service/tests/Validation
npm run verify: TypeScript build passed; 65 test files and 724 tests passed.npx openspec validate a4-semantic-search-model-qa --strict: passed.npx openspec validate --all --strict: 64 passed, 0 failed.git diff --checkand staged trailing-whitespace/credential scans: passed; production credential signatures: 0.governanceProxy.ts, viewer, Kit Manager, and streaming scopes: zero diff.dc20f52: response exact-echo allowlist is restricted to title/description/assignee; snapshot-only path-like strings now fail closed.Known Risks
No module named ruff), so no Ruff result is claimed.Summary by CodeRabbit