Skip to content

feat(a4): 加入 session-scoped search proxy - #384

Merged
monkey1sai merged 4 commits into
mainfrom
feat/a4-s4b-session-search-proxy
Jul 23, 2026
Merged

monkey1sai merged 4 commits into
mainfrom
feat/a4-s4b-session-search-proxy

Conversation

@monkey1sai

@monkey1sai monkey1sai commented Jul 23, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • 新增 coordinator-owned A4 session/partial-confirmation/IFC-ready search proxy;browser 不再取得 host path、mapping root 或 governance credential,generic browser search 維持停用。
  • Search authority 綁定 authenticated principal、current primary lease 與 exact active stage binding;model、artifact、source、mapping 均由 server resolve,host-kit transport 使用 exact-origin allowlist、server-only token 與 read-only artifact mount。
  • PR fix(viewer): preserve A4 scoped search compatibility #386 已先合併 caller compatibility prerequisite;本 PR 再加入 mode-aware layered timeout chain:viewer deterministic 15s、semantic/auto/omitted 150s;coordinator deterministic/partial 12s、semantic/auto/omitted 135s,hard clamp 140s;governance LLM 120s 後才進入最多 10s IFC scan。
  • S4-C Issue 與 S4-D UI 不在此 PR;Full completion claimed: no。

AI Coding Governance

Item Result
Change lane G
Behavior contract changed yes
Linked issue OpenSpec a4-semantic-search-model-qa S4-B;無獨立 GitHub issue
Requirement source docs/plans
CODEOWNERS / owner review 兩位 frozen-diff read-only reviewers 對 current local head 均為 CLEAN,0 P1/P2;GitHub current-head review/checks 在 push 後重新執行
GitNexus evidence Pre-change route impact LOW;createCoordinatorApp CRITICAL 為 graph over-attribution,已有編輯前 sign-off;final detect_changes 重試仍為 Transport closed,所以 risk 誠實記為 UNKNOWN,不宣稱 pass
Browser E2E evidence Fresh isolated host-native governance :49103 + coordinator :8005,strict real-IFC e2e/a4-closeout.spec.ts 4/4 PASS;cold first search 8.8s,四次 internal search 均為 HTTP 200
Agent workflow changed? no;只更新既有 NOW/OpenSpec S4-B closeout evidence
Required checks expected CI、Agent Governance、PR Review Agent、affected coordinator/viewer、compose/deploy/secret、design visual/semantic checks

Deploy Path Verification

Item Result
Affects runtime / docker / Kit / viewer / ports / env? yes;host-kit coordinator→host-native governance transport、viewer caller timeout、env 與 artifact mount
Canonical deploy path updated? scripts/deploy.ps1 updated;host root normalization、token validation/fingerprint 與 web-plane refresh wiring
New root script added? no
Deploy dry-run command PowerShell 7 與 Windows PowerShell 5.1 sequential scripts/tests/test-deploy-dryrun.ps1 均 PASS;涵蓋短/長 token fail-closed、secret-safe signature 與 unchanged/changed effective-config regression
Full deploy tested 尚未;PR merge 後從 freshly fetched origin/main 執行 canonical hybrid lab smoke
Verify command PS7 + PS5.1 scripts/tests/test-deploy-governance-static.ps1 PASS;docker compose -f compose.runtime-manager.yml -f compose.host-kit.yml --env-file .env.web-plane.host-kit.example config --quiet PASS
Frontend URL verified http://127.0.0.1:8005/ui/#semantic-search
Evidence path docs/plans/NOW.md S4-B closeout;openspec/changes/a4-semantic-search-model-qa/tasks.md

Frontend Verification

Item Result
Frontend route http://127.0.0.1:8005/ui/#semantic-search
Main button(s) tested Run query at Chromium DPR1 1440×900 and 1920×1080;Issue/session/3D actions remain visibly disabled because S4-C/S4-D are out of scope
Fixture used Real ignored canonical IFC fixture, exactly 89,394,282 bytes;branch-isolated temporary copy and cache were removed after the run
Backend API called Browser scoped POST to coordinator :8005, which called internal governance :49103; no generic browser search, path, mapping root, or credential was sent
Runtime action job_id=ifcready_1784798746123_35b80532; deterministic IfcDoor query resolved against the server-owned active IFC artifact
Visible success state loading: pending Run state is covered by current viewer suite; success: real IfcDoor rows and scopes rendered; failure: unmatched IfcSpaceHeater rendered the explicit terminal no-row state; retry: a subsequent query remained operable and viewer tests cover reset/retry behavior
E2E command E2E_DISABLE_WEBSERVER=1 E2E_COORDINATOR_BASE_URL=http://127.0.0.1:8005 A4_E2E_REQUIRE_REAL=1 A4_E2E_IFC_READY_JOB_ID=ifcready_1784798746123_35b80532 npx playwright test e2e/a4-closeout.spec.ts — 4/4 PASS
Screenshot / trace Ignored local evidence retained under artifacts/e2e/_output/.../*.png and artifacts/e2e/a4-trace/.../trace.zip
Design gate status mixed
Design screen(s) concept.a10.default, concept.a5.default, concept.a6.default, concept.a7.default, concept.a8.default, concept.a9.default, console.home.default, pipeline.default, runtime.ops.default, workspace.a1.default, workspace.a2.default, workspace.a3.default, workspace.a4.default
Reference-missing route(s) / surface(s) #admin, #conv, #gpu, #instances, #issues, #minio, #reports, #review, #sessions, #spec, #viewer
Full completion claimed no
Design reference manifest docs/plans/design-system-reference.manifest.json, SHA-256 64ab4bb42f62d5356f411c2bae35da773d8f0d4e0371771b39f4fbbfb215efe8
Visual fidelity result Current-head required-check output: artifacts/e2e/design-system-visual-result.json; prerequisite PR #386 produced the same 13-screen scope
Visual comparison Required current-head gate: pixel diff <=1% and semantic parity 100%; PR #386 prerequisite evidence was 26 comparisons with max 0.00983796% and semantic 100%
Visual artifacts Required current-head CI artifacts: actual.png and diff.png per screen under artifacts/e2e/design-system-visual/
Known gaps Reference-missing surfaces remain: #admin, #conv, #gpu, #instances, #issues, #minio, #reports, #review, #sessions, #spec, #viewer. No standalone approved golden exists for #semantic-search; session/Issue/3D actions remain disabled; live Ornith and post-merge canonical hybrid smoke are pending; GitNexus final risk is UNKNOWN

Validation

  • npm run verify in bim-review-coordinator: TypeScript build PASS; 64 files / 703 tests PASS.
  • npm test -- governance-search-for-session.test.ts: 18/18 PASS, including a real 5.25s delayed upstream, mode/default/clamp budgets, and exact sanitized timeout 502 behavior.
  • npm run verify in web-viewer-sample: typecheck, production build, full Vitest/DOM suite, and structured-log check PASS.
  • npm test -- governanceClient.test.ts clientTimeout.test.ts: 13/13 PASS; caller-provided signals override the general 15s default and only A4 semantic/auto/omitted receives the 150s browser budget.
  • Fresh branch-isolated strict real-IFC Playwright: 4/4 PASS in 13.1s; first request was cold and no pre-E2E search POST existed.
  • PS7 and Windows PowerShell 5.1 sequential deploy dry-run: PASS. An initial parallel cross-shell attempt collided over shared harness state; both canonical sequential reruns passed.
  • PS7 and Windows PowerShell 5.1 governance static deploy test: PASS.
  • Docker host-kit compose config: PASS.
  • npx --no-install openspec validate a4-semantic-search-model-qa --strict: PASS.
  • npx --no-install openspec validate --all --strict --no-interactive: 63 passed / 0 failed.
  • git diff --check: PASS. High-signal secret scan matching CI: PASS.

Independent Review

  • PR fix(viewer): preserve A4 scoped search compatibility #386 resolved the prior caller-compatibility P2 before this branch was refreshed from origin/main.
  • A first frozen-diff review found one timeout hierarchy P1: the proposed short coordinator/browser deadlines would truncate legitimate model-backed search. The implementation was corrected to the layered budgets documented above.
  • Two independent frozen-diff read-only reviewers then re-reviewed the final six-file timeout/closeout delta and both returned CLEAN with 0 P1/P2, no secret/path leakage, and no scope drift.

Known Risks

  • GitNexus detect_changes is UNKNOWN because the transport closed repeatedly; this is not reported as a pass.
  • Full canonical hybrid deployment smoke, live Ornith, and post-merge runtime observation are intentionally deferred until this PR merges onto freshly fetched origin/main.
  • The response-leak heuristic is fail-closed and can reject legitimate IFC property text that resembles an absolute path or credential key.
  • OpenSpec 3.4 literal auth_scope=local_dev_lab remains unfinished; the current production pending provider fails closed while trusted context retains the existing lab schema.
  • S4-C Issue and S4-D UI are not implemented; full completion is not claimed.

以 coordinator authority 綁定 session、principal、lease 與 active stage,並補上 host-kit 安全 transport 與雙 namespace mapping。

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 23, 2026 05:49
@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@monkey1sai, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 33 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7295c3c3-175b-4535-888f-5229b4c42a18

📥 Commits

Reviewing files that changed from the base of the PR and between 1915d40 and fd6f9b2.

📒 Files selected for processing (10)
  • bim-review-coordinator/src/routes/a4SearchRoutes.ts
  • bim-review-coordinator/tests/config.test.ts
  • bim-review-coordinator/tests/governance-search-for-session.test.ts
  • docs/plans/NOW.md
  • openspec/changes/a4-semantic-search-model-qa/tasks.md
  • scripts/deploy.ps1
  • scripts/tests/test-deploy-dryrun.ps1
  • scripts/tests/test-deploy-governance-static.ps1
  • web-viewer-sample/src/console/governanceClient.test.ts
  • web-viewer-sample/src/console/governanceClient.ts
📝 Walkthrough

Walkthrough

A4 governance search forwarding is added to the coordinator with server-resolved session authority, trusted transport validation, conversion-artifact path containment, deployment token fingerprinting, host-kit configuration, and expanded contract and integration tests.

Changes

A4 search governance flow

Layer / File(s) Summary
Runtime configuration and deployment wiring
.env.web-plane.host-kit.example, bim-review-coordinator/.env.example, bim-review-coordinator/src/config.ts, compose.host-kit.yml, scripts/deploy.ps1, scripts/tests/*
Adds A4 token, governance-origin, and conversion-artifact settings; mounts artifacts read-only; validates token length; and includes token fingerprints in governance runtime signatures.
Active stage authority and context resolution
bim-review-coordinator/src/services/stageBindingAuthorityStore.ts, bim-review-coordinator/src/app.ts, bim-review-coordinator/tests/services/*
Stores complete active binding snapshots and resolves authenticated session or IFC-ready context using active leases, bindings, source IFCs, and contained mapping artifacts.
Trusted A4 route contracts and forwarding
bim-review-coordinator/src/routes/a4SearchRoutes.ts, bim-review-coordinator/src/app.ts, bim-review-coordinator/README.md
Adds session search and partial-confirmation routes, validates browser controls and trusted origins, forwards bounded table-only context, sanitizes responses, and disables generic browser search.
Route, lifecycle, and configuration validation
bim-review-coordinator/tests/governance-search-for-session.test.ts, bim-review-coordinator/tests/config.test.ts, bim-review-coordinator/tests/unit_kitpool.test.ts, openspec/changes/...
Covers forwarding contracts, lifecycle and lease enforcement, artifact mapping, failure paths, response leakage prevention, configuration defaults, and recorded S4-B validation status.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Browser
  participant Coordinator
  participant GovernanceAPI
  Browser->>Coordinator: Submit session-scoped search
  Coordinator->>Coordinator: Authenticate and resolve active lease and artifacts
  Coordinator->>GovernanceAPI: Forward trusted table-only request
  GovernanceAPI-->>Coordinator: Return bounded JSON
  Coordinator-->>Browser: Return sanitized response
Loading

Possibly related PRs

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly matches the main change: adding a session-scoped A4 search proxy.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/a4-s4b-session-search-proxy

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
scripts/tests/test-deploy-dryrun.ps1 (1)

364-376: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Clean up long-token test artifacts in finally.

If the long-token test fails before Line 252, its env file remains under scripts/.run with the generated token. Add its .env, .out.log, and .err.log files to this cleanup list.

Proposed fix
         'deploy-a4-token-short-test.env',
         'deploy-a4-token-short-test.out.log',
-        'deploy-a4-token-short-test.err.log'
+        'deploy-a4-token-short-test.err.log',
+        'deploy-a4-token-long-test.env',
+        'deploy-a4-token-long-test.out.log',
+        'deploy-a4-token-long-test.err.log'
🤖 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 `@scripts/tests/test-deploy-dryrun.ps1` around lines 364 - 376, Extend the
$testArtifact cleanup list in the finally block to include the long-token test’s
.env, .out.log, and .err.log artifacts, using the exact filenames created by the
long-token test near Line 252.

Source: Coding guidelines

🤖 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 `@docs/plans/NOW.md`:
- Line 1: Update the document title area in NOW to include an explicit
document-nature label such as “working note” or “agent boundary,” clearly
distinguishing it from runtime/API specifications and completion evidence.
- Line 119: Update the PR status entry in docs/plans/NOW.md to reflect the
current state of PR `#384`, including its review status, or explicitly label it as
a pre-PR snapshot if that historical context is intentional. Remove the outdated
claim that the branch is uncommitted, unpushed, and has no PR.

In `@scripts/deploy.ps1`:
- Around line 421-424: Update the refresh-detection logic around the
environment-key list and the A4_CONVERSION_ARTIFACTS_HOST_ROOT assignment so
$shouldRefreshWebPlane is not triggered by the value generated during the
current deployment. Compare the original env-file or parent value, or use a
persisted effective-config comparison, while preserving refresh behavior when
the underlying configuration actually changes.

---

Outside diff comments:
In `@scripts/tests/test-deploy-dryrun.ps1`:
- Around line 364-376: Extend the $testArtifact cleanup list in the finally
block to include the long-token test’s .env, .out.log, and .err.log artifacts,
using the exact filenames created by the long-token test near Line 252.
🪄 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: b06060c9-ef74-4420-9b85-b8e72579d39c

📥 Commits

Reviewing files that changed from the base of the PR and between 84bdf5c and 1915d40.

📒 Files selected for processing (17)
  • .env.web-plane.host-kit.example
  • bim-review-coordinator/.env.example
  • bim-review-coordinator/README.md
  • bim-review-coordinator/src/app.ts
  • bim-review-coordinator/src/config.ts
  • bim-review-coordinator/src/routes/a4SearchRoutes.ts
  • bim-review-coordinator/src/services/stageBindingAuthorityStore.ts
  • bim-review-coordinator/tests/config.test.ts
  • bim-review-coordinator/tests/governance-search-for-session.test.ts
  • bim-review-coordinator/tests/services/stageBindingAuthorityStore.test.ts
  • bim-review-coordinator/tests/unit_kitpool.test.ts
  • compose.host-kit.yml
  • docs/plans/NOW.md
  • openspec/changes/a4-semantic-search-model-qa/tasks.md
  • scripts/deploy.ps1
  • scripts/tests/test-deploy-dryrun.ps1
  • scripts/tests/test-deploy-governance-static.ps1

Comment thread docs/plans/NOW.md
Comment thread docs/plans/NOW.md Outdated
Comment thread scripts/deploy.ps1 Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR implements slice S4-B of the A4 semantic-search capability: a coordinator-owned, session-scoped search proxy that sits in front of the frozen governanceProxy.ts. It replaces the previous permissive smoke-test proxy with a hardened boundary where the browser can no longer supply host paths or identity/authority fields. A new a4SearchRoutes.ts module is mounted before the legacy generic proxy so Express first-match routing enforces the boundary without editing the frozen file. The coordinator authenticates the principal, requires the caller's active primary viewer lease and exact active stage binding, and server-resolves model/artifact/source/mapping from its own state. The host-kit transport adds an exact-origin allowlist, a 16–4096 char server-only token, a read-only artifacts mount, and dual coordinator-visible/host-native namespace containment for mapping provenance.

Changes:

  • New a4SearchRoutes.ts route module with strict control sanitization, exact-origin/loopback transport gating, bounded JSON reads, and recursive server-path/credential leak detection (fail-closed); generic browser search returns 404 a4_generic_search_disabled.
  • app.ts/config.ts wiring: A4 principal authentication, server-side session/IFC-ready context resolution, dual-namespace mapping containment, and two new config keys (a4ConversionArtifactsRoot, a4ConversionArtifactsHostRoot); stageBindingAuthorityStore exposes a deep-cloned activeBinding snapshot.
  • Deploy/compose plumbing: token length validation + secret-safe fingerprint in the governance runtime signature, host-native artifacts root resolution, read-only mount, env examples, plus OpenSpec/NOW closeout docs and extensive tests.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
bim-review-coordinator/src/routes/a4SearchRoutes.ts New route module: control validation, transport gating, leak detection
bim-review-coordinator/src/app.ts A4 principal auth, session/IFC-ready resolvers, mapping containment, router mount
bim-review-coordinator/src/config.ts Adds a4ConversionArtifactsRoot/HostRoot config keys
bim-review-coordinator/src/services/stageBindingAuthorityStore.ts Exposes deep-cloned activeBinding snapshot with lease/composition
bim-review-coordinator/tests/governance-search-for-session.test.ts Rewritten contract + integration coverage for the proxy
bim-review-coordinator/tests/config.test.ts Coverage for new config keys (contains duplicated assertions)
bim-review-coordinator/tests/services/stageBindingAuthorityStore.test.ts Verifies activeBinding isolation and principal scoping
bim-review-coordinator/tests/unit_kitpool.test.ts Adds new config fields to the default test config
bim-review-coordinator/README.md / .env.example Documents A4 routes, token, and artifact mounts
scripts/deploy.ps1 Token validation, fingerprint, host-native artifacts root injection
scripts/tests/test-deploy-dryrun.ps1 A4 token/host-root dry-run tests (long-token cleanup omission)
scripts/tests/test-deploy-governance-static.ps1 Static assertions for compose/env wiring
compose.host-kit.yml / .env.web-plane.host-kit.example Origin allowlist, token, read-only artifacts mount
openspec/changes/.../tasks.md, docs/plans/NOW.md S4-B closeout evidence and status updates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread bim-review-coordinator/tests/config.test.ts Outdated
Comment thread scripts/tests/test-deploy-dryrun.ps1 Outdated
Comment on lines +374 to +376
'deploy-a4-token-short-test.env',
'deploy-a4-token-short-test.out.log',
'deploy-a4-token-short-test.err.log'

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 464adbc: added all long-token artifacts to the outer finally cleanup and also covered the new web-plane signature test artifact. Independent PS7 and Windows PowerShell 5.1 dry-run runs leave no matching artifacts.

@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: 1915d4081a

ℹ️ 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".

Comment thread scripts/deploy.ps1
Comment on lines +649 to +650
[Environment]::SetEnvironmentVariable('STREAMING_CONVERSION_ARTIFACTS_ROOT', $resolvedConversionArtifactsRoot, 'Process')
[Environment]::SetEnvironmentVariable('A4_CONVERSION_ARTIFACTS_HOST_ROOT', $resolvedConversionArtifactsRoot, 'Process')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Track artifact-root changes when services are already running

When STREAMING_CONVERSION_ARTIFACTS_ROOT changes while host-native conversion is already running, this new root is exported for the A4 coordinator/container path but it is not part of New-ConversionRuntimeSignature and is not a web-plane refresh input unless A4_CONVERSION_ARTIFACTS_HOST_ROOT is explicitly present in the env file. In that scenario deploy can skip restarting conversion (and, for loopback/no-spectator runs, skip recreating the coordinator), so conversion keeps writing element_mapping.json under the old root while A4 validates/mounts the new root, causing session search to fail with a4_session_mapping_unavailable until a manual restart. Include the resolved artifacts root in the conversion signature and refresh predicate.

AGENTS.md reference: scripts/AGENTS.md:L33-L33

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 464adbc: the resolved artifacts root is now part of New-ConversionRuntimeSignature and the effective web-plane signature, so a root change restarts host-native conversion and reconciles the coordinator mount while unchanged inputs remain idempotent.

Comment on lines +490 to +493
app.post("/api/governance/search/model", (_request, response) => {
response.status(404).json({
error_code: "a4_generic_search_disabled",
detail: "Generic A4 browser search is disabled; use a server-scoped route.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Remove the still-visible A4 path mode

When a user selects the existing A4 path source mode, the frontend still calls governanceClient.searchModel, which posts to /api/governance/search/model; this route now always returns a4_generic_search_disabled, so that visible UI path is guaranteed to fail instead of running a local/dev path search. Since this backend change intentionally disables the generic route in every profile, the path option needs to be removed/disabled or rerouted to one of the server-scoped flows in the same slice.

AGENTS.md reference: bim-review-coordinator/AGENTS.md:L41-L41

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Verified against origin/main and current head: this is a true visible regression. The existing #semantic-search path action still calls the generic route, which this PR now disables in every profile. PR #384 is held from merge; fixing it requires an explicitly authorized minimal UI compatibility slice (or a revised PR sequence), because S4-D UI was declared out of scope.

Comment on lines +574 to +575
const principal = authenticate(request, response, deps);
if (!principal) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Pass browser auth through A4 scoped searches

The existing A4 console defaults to ifc_ready, and its governanceClient.searchModelForIfcReady/searchModelForSession calls only send JSON headers; they never include X-User-Token or Authorization. Because these newly mounted scoped routes now call authenticate() before resolving the job/session, clicking Run query from the default IFC-ready mode, and from session mode, returns 401 a4_authentication_required even in the local-dev flow. Wire the same user token used by the coordinator lease APIs into these A4 client calls, or keep the route compatible with the existing page.

AGENTS.md reference: bim-review-coordinator/AGENTS.md:L41-L41

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Verified against source and mounted integration tests: this is a true visible regression. The scoped clients send no principal carrier, while the new routes authenticate before resolution; an arbitrary token would also fail session lease ownership. PR #384 is held from merge pending an explicitly authorized UI identity/caller compatibility slice or revised sequencing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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: 464adbc847

ℹ️ 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".

Comment thread compose.host-kit.yml
A4_CONVERSION_ARTIFACTS_ROOT: /workspace/a4-conversion-artifacts
# Canonical deploy resolves this to the absolute host namespace used by
# host-native governance; it is never used for container-side file reads.
A4_CONVERSION_ARTIFACTS_HOST_ROOT: ${A4_CONVERSION_ARTIFACTS_HOST_ROOT:-}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Default the A4 host artifact root to the host path

When the web plane is started through scripts/start-web-plane-docker.ps1 or direct compose rather than the canonical deploy.ps1, the example env leaves A4_CONVERSION_ARTIFACTS_HOST_ROOT blank, so the coordinator falls back to its container-visible A4_CONVERSION_ARTIFACTS_ROOT (/workspace/a4-conversion-artifacts). It can validate the read-only mount, but it then forwards that container path as element_mapping_path to the host-native governance service, which cannot read it and silently loses the mapping join/highlight eligibility for otherwise valid A4 session searches. Default this to the same host-side source used by the volume, or have the web-plane starter inject the resolved host path as deploy does.

Useful? React with 👍 / 👎.

@monkey1sai
monkey1sai merged commit 359662f into main Jul 23, 2026
15 checks passed
@monkey1sai
monkey1sai deleted the feat/a4-s4b-session-search-proxy branch July 23, 2026 09:54
monkey1sai added a commit that referenced this pull request Jul 29, 2026
main 版此頁自述為 legacy 相容頁,且 JSX 早已預留 `a4-source-session` 按鈕但
標為 disabled(caption:「legacy route 無法與 active primary viewer lease 共置;
等待 canonical S4-D workspace」)。searchModelForSession 亦早在 #384 進了
governanceClient,只是 UI 沒接。本 commit 就是把它接上。

以 main 為基底疊加(convergence 的對應段落只作參考,未整檔取用):
- 新增 SourceMode 與 session 解析四件組(A4_SESSION_ID_RE / STORAGE_KEY /
  firstValidA4SessionId / initialA4SessionId);selector 只接受語法合法的
  opaque review_session_* id,且不構成 authority
- refreshSources 併入 coordinatorClient.runtimeStatus(),只採 status==="active"
  的 session;已選取者若仍 active 則保留,不被清單順序覆寫
- onRun 依 sourceMode 分派 searchModelForSession / searchModelForIfcReady,
  並保存 A4ResultContext(sourceMode/sessionId/jobId/query/interpretMode)
- 新增 resultContextMatchesCurrent:輸入變更後舊結果不得被當成目前查詢的答案,
  UI 顯示 a4-result-stale-context 警示

安全修正(誠實鐵律/不洩漏):
- onRun 的 catch 由 `setRunErr(String(e))` 改為 a4RequestErrorCopy(e)。原寫法會
  把完整 error message(含 path/upstream detail)直接顯示給使用者;新寫法只輸出
  allowlist code 對應的復原指引
- governanceClient 的兩個 A4 route 加 `{ safeError: true }`,錯誤只帶 status 與
  allowlist code
- response.error_code 經 safeA4DiagnosticCode 白名單化後才顯示

預設模式為 context-aware:進站帶合法 session selector(例如自 #workspace?dock=a4)
→ session;否則維持 ifc_ready,不把 legacy 入口使用者丟進無 session 可選的畫面。
此設計讓既有 13 個 LLM readiness / source window 測試無須改寫即通過。

測試更新(僅一處,反映刻意的行為變更):
`a4-source-session` 的 disabled 斷言由 true 改 false、`runtimeStatus` 由
not.toHaveBeenCalled 改 toHaveBeenCalled。其餘斷言原樣保留。

未涵蓋(仍屬 deferred 母版):Issue draft/create UI 與 client methods、
partial-confirmation 兩個 visible state、3D handoff、7.x evidence gates。

驗證:npx tsc --noEmit exit 0;npx vitest run 69 files / 780 tests 全綠。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016su1Voom8iT8aFYUZ7URbq
monkey1sai added a commit that referenced this pull request Jul 29, 2026
* feat(a4): 移除 A4 dock 偽造證據並收斂 canonical route

a4-console-convergence tasks 3.3/3.4 的第一個切片。這 7 個檔在 merge-base
(5cbfef2) 之後 origin/main 未曾改動,經 git rev-list 逐檔確認,故直接取用
convergence 分支版本無資訊丟失風險;其餘 6 個核心檔(main 有 #384/#386 新工作)
另行逐段調和。

移除的偽造證據(docks.tsx A4Dock):
- 假 toast「POST /api/search/model → 12 hits · 信心度 0.86」
- 假查詢結果 4 筆(fixtures.ts a4Defs)與「不符合 5 / 符合 7」計數
- 假 Evidence Trace「規範條文 · 建築技術規則 76 條 Matched」

改為 data-prov="redirect" 的非操作性導流面板,並誠實說明 A4 live search
為 session-scoped 且 table-only。

canonical route 收斂:
- routing.ts: PRODUCT_CONSOLE_ROUTES 加入 workspace
- docks.tsx: LIVE_LINK_HREF.a4 由 #semantic-search 改為 #workspace?dock=a4
- WorkspacePage.tsx: A4 legend 移除假的「不符合 5 · 符合 7」
- fixtures.ts: 移除 a4Defs 與 a4Ran flag

design-system-semantic-cases.ts 必須同批更新——main 版第 203/232/265 行正在
斷言上述假數據存在(text="符合 7"、text="不符合 5"、confirmToast
"POST /api/search/model");不同步會讓 semantic gate 由「pixel 變動」惡化為
「斷言舊假數據不存在」的自造破壞。

驗證:npx tsc --noEmit exit 0;npx vitest run 69 files / 780 tests 全綠。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016su1Voom8iT8aFYUZ7URbq

* feat(a4): EdgeConsole 辨識 canonical A4 路由並清洗 URL 夾帶內容

EdgeConsole.tsx 在 merge-base(5cbfef2) 之後 origin/main 未曾改動,故直接取用
convergence 版本。

- 新增 A4 session context 解析:依序自 URL search、hash query、history state、
  sessionStorage 取第一個符合 /^review_session_[A-Za-z0-9_-]+$/ 的 opaque
  selector;sessionStorage 讀取以 try/catch 包覆(hardened browser context 可能
  不可用)
- usePageHash 辨識 #workspace?dock=a4:僅 query 恰為 dock=a4 且無 location.search
  時回 workspace-a4,否則回 workspace-a4-scrub,使 URL 夾帶的
  query/proof/prim/handoff material 走清洗路徑

該 selector 本身不構成 authority——coordinator 仍負責 authenticate 與 resolve
session(見 EdgeConsole.tsx 內註解)。

驗證:npx tsc --noEmit exit 0(確認無跨檔依賴);npx vitest run 69 files /
780 tests 全綠。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016su1Voom8iT8aFYUZ7URbq

* feat(a4): governanceClient 補上 A4 safe error surface 與 response 傳輸型別

以 origin/main 為基底逐段疊加 convergence 的 A4 live 型別,不整檔取用。
main 側的 #384/#386/#379 三個 PR 已在此檔實作多項 A4 契約(移除通用
searchModel、X-User-Token principal carrier、A4 專用 150s timeout、
ModelSearchLlmStatus 改為不洩漏 endpoint 的形狀),全部原樣保留。

擋下的回歸:convergence 版把 jsonFetch 的
`signal: init?.signal ?? AbortSignal.timeout(...)` 改回
`signal: AbortSignal.timeout(GOV_FETCH_TIMEOUT_MS)`——那個 `init?.signal ??`
正是 #384 為了讓 a4SearchTimeoutSignal 的 150s 生效才加的,盲目取用會讓
model-capable search 退回 15s 逾時。本 commit 保留 main 版並加註說明。

疊加內容:
- A4SafeErrorCode(17 個 allowlist code)+ A4_SAFE_ERROR_CODES Set + A4GovernanceError;
  錯誤訊息只含 status,不含 path/detail/upstream diagnostics
- jsonFetch 加 options.safeError 分支:只採信 allowlist 內的 error_code,
  其餘一律退回純 status,避免 coordinator/upstream 診斷洩漏到 A4 UI
- ModelSearchResponse 補 model_invocation、session_binding、error_code、
  retryable、stats 細項(returned/mapped/not_matched/total_is_lower_bound/
  scan_complete),search_scope 收窄為 union

範圍界定:partial_* 四個欄位只補傳輸型別(後端已可回傳),本 change 的 UI
不實作 partial-confirmation-required / confirmed partial 兩個 visible state;
A4 Issue draft/create 的 client methods 亦不在本 change——兩者都屬 deferred
母版 a4-semantic-search-model-qa 的其他 Requirement。

驗證:npx tsc --noEmit exit 0;npx vitest run 69 files / 780 tests 全綠。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016su1Voom8iT8aFYUZ7URbq

* feat(a4): A4 Console 接上 canonical session-scoped 查詢路徑

main 版此頁自述為 legacy 相容頁,且 JSX 早已預留 `a4-source-session` 按鈕但
標為 disabled(caption:「legacy route 無法與 active primary viewer lease 共置;
等待 canonical S4-D workspace」)。searchModelForSession 亦早在 #384 進了
governanceClient,只是 UI 沒接。本 commit 就是把它接上。

以 main 為基底疊加(convergence 的對應段落只作參考,未整檔取用):
- 新增 SourceMode 與 session 解析四件組(A4_SESSION_ID_RE / STORAGE_KEY /
  firstValidA4SessionId / initialA4SessionId);selector 只接受語法合法的
  opaque review_session_* id,且不構成 authority
- refreshSources 併入 coordinatorClient.runtimeStatus(),只採 status==="active"
  的 session;已選取者若仍 active 則保留,不被清單順序覆寫
- onRun 依 sourceMode 分派 searchModelForSession / searchModelForIfcReady,
  並保存 A4ResultContext(sourceMode/sessionId/jobId/query/interpretMode)
- 新增 resultContextMatchesCurrent:輸入變更後舊結果不得被當成目前查詢的答案,
  UI 顯示 a4-result-stale-context 警示

安全修正(誠實鐵律/不洩漏):
- onRun 的 catch 由 `setRunErr(String(e))` 改為 a4RequestErrorCopy(e)。原寫法會
  把完整 error message(含 path/upstream detail)直接顯示給使用者;新寫法只輸出
  allowlist code 對應的復原指引
- governanceClient 的兩個 A4 route 加 `{ safeError: true }`,錯誤只帶 status 與
  allowlist code
- response.error_code 經 safeA4DiagnosticCode 白名單化後才顯示

預設模式為 context-aware:進站帶合法 session selector(例如自 #workspace?dock=a4)
→ session;否則維持 ifc_ready,不把 legacy 入口使用者丟進無 session 可選的畫面。
此設計讓既有 13 個 LLM readiness / source window 測試無須改寫即通過。

測試更新(僅一處,反映刻意的行為變更):
`a4-source-session` 的 disabled 斷言由 true 改 false、`runtimeStatus` 由
not.toHaveBeenCalled 改 toHaveBeenCalled。其餘斷言原樣保留。

未涵蓋(仍屬 deferred 母版):Issue draft/create UI 與 client methods、
partial-confirmation 兩個 visible state、3D handoff、7.x evidence gates。

驗證:npx tsc --noEmit exit 0;npx vitest run 69 files / 780 tests 全綠。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016su1Voom8iT8aFYUZ7URbq

* test(a4): 補 session 路徑覆蓋並修正過時的 scope 文案

前一個 commit 改了行為卻只更新既有斷言,等於新功能無測試保障。本 commit 補上。

新增兩個測試:
- routes the canonical session source to the session-scoped search:
  驗證切到 session 來源後只有 status==="active" 的 session 進入清單(closed 的
  不得出現)、job select 消失、且實際呼叫 searchModelForSession 而非
  searchModelForIfcReady
- keeps upstream detail out of the surface when a session search fails:
  以 A4GovernanceError(409, "a4_session_not_active") 驗證畫面只出現 allowlist
  code 對應的復原指引,不得帶出 status code、實際 request path 或
  「A4 governance request failed」原始訊息

修正過時文案:a4-source-scope-note 原本寫「session flow 等 canonical S4-D
workspace 共置 viewer lease 後才啟用」,但 session flow 已於前一個 commit 啟用,
留著即為不實陳述。改為依 sourceMode 分別說明能力邊界——session 模式標
session_table_only 並明講 Issue 需 signed-proof route、3D 需 canonical handoff
且兩者尚未接通;ifc_ready 模式維持 table-only 並指引切換。

驗證:npx tsc --noEmit exit 0;npx vitest run 69 files / 782 tests 全綠
(較前一輪 +2,即本 commit 新增的兩個測試)。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016su1Voom8iT8aFYUZ7URbq

* fix(a4): 補 a4-table-only 標記並對齊 Issue 控制的既有契約

visual gate 的 workspace.a4.default 有三個 semantic DOM expectation 失敗,
逐項查證後分為兩類:

1. a4-table-only-boundary-visible / a4-runtime-table-only-marker
   兩者都要求 [data-testid="a4-table-only"] visible,而該標記是 convergence 版
   有、我上一輪移植時漏掉的。補在結果 Panel 的動作區,並依 sourceMode 分別說明
   table-only 邊界(session 模式指向 proof 與 lease 缺口;ifc_ready 模式指出
   相容入口不具 Issue/3D authority)。

2. a4-no-issue-control 原本期望 [data-testid="a4-create-issues"] count_equals 0,
   但 main 生態以「存在但 disabled」表達 Issue 尚不可用——
   A4SemanticSearchPage.test.tsx:147 與 e2e/a4-closeout.spec.ts:132,150 都斷言
   它 disabled。依「以 main 為基底」原則改 semantic case 的 expectation 為
   disabled,而非為了配合 case 去刪 main 的既有控制項與其三處測試依賴。

注意:pixel diff 仍會 fail(rebaseline 屬 deferred 母版 task 7.1),本 commit
只處理 semantic 契約。

驗證:npx tsc --noEmit exit 0;npx vitest run 69 files / 782 tests 全綠;
SemanticExpectation 型別確認含 "disabled"。

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016su1Voom8iT8aFYUZ7URbq

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

2 participants