refactor(coordinator): migrate governance library diff workflow - #486
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe governance library diff route now delegates tree resolution, diff request construction, upstream forwarding, error classification, and path redaction to ChangesGovernance Library Diff
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GovernanceLibraryRoute
participant GovernanceLibraryWorkflow
participant GovernanceLibraryHttpAdapter
Client->>GovernanceLibraryRoute: POST /api/governance-library/diffs
GovernanceLibraryRoute->>GovernanceLibraryWorkflow: runLibraryDiff(command)
GovernanceLibraryWorkflow->>GovernanceLibraryHttpAdapter: load tree and POST /api/diffs
GovernanceLibraryHttpAdapter-->>GovernanceLibraryWorkflow: upstream response
GovernanceLibraryWorkflow-->>GovernanceLibraryRoute: mapped outcome with redacted body
GovernanceLibraryRoute-->>Client: HTTP response
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 completes the Governance Library Workflow cutover for POST /api/governance-library/diffs in the coordinator, following the already-merged rule-run migration (#401, closing #396). The inline diff logic in the composition root (createCoordinatorApp) is replaced by a delegation to the operation-specific GovernanceLibraryWorkflow.runLibraryDiff, backed by a narrow port method on the production HTTP adapter. The public route/schema, generic governance proxy, and all viewer/deploy/security surfaces are unchanged, and only the now-unreferenced inline helpers are removed.
Changes:
- Add
runLibraryDifftoGovernanceLibraryWorkflow(withGovernanceLibraryDiffBody/GovernanceLibraryDiffCommandtypes and apostDiffport method) that resolves base and target from one tree snapshot, shapes optional fields, forwards opaquely, and redacts server paths. - Add
postDifftoGovernanceLibraryHttpAdapter(POST/api/diffs, no timeout/retry, per-call base resolution). - Rewrite the diff Express route to delegate to the workflow and map closed outcomes to the existing 404/502/forward semantics, and remove the superseded inline helpers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
bim-review-coordinator/src/services/governanceLibraryWorkflow.ts |
Adds diff body/command types, the postDiff port method, and runLibraryDiff with one-tree resolution, optional-field shaping, and path redaction. |
bim-review-coordinator/src/services/governanceLibraryHttpAdapter.ts |
Adds the postDiff HTTP adapter method targeting /api/diffs. |
bim-review-coordinator/src/app.ts |
Delegates the diff route to the workflow and removes the now-unreferenced inline helpers (findLibraryVersionPath, redactServerPathsForBrowser, fetchGovernanceLibraryTree, forwardLibraryPostToGovernance). |
bim-review-coordinator/tests/governance-library-workflow.test.ts |
Adds runLibraryDiff seam tests (one-tree resolution, omission, per-side version-not-found, throwing getter, transport/POST failures). |
bim-review-coordinator/tests/governance-library-routes.test.ts |
Adds diff route integration coverage for upstream status/content-type/opaque-body/path-masking parity, optional-field semantics, and per-side missing versions. |
bim-review-coordinator/tests/governance-library-http-adapter.test.ts |
Extends adapter tests to assert postDiff URL, headers, body, no signal, and per-call base resolution. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![codex-tri-adversarial-bot[bot]](https://avatars.githubusercontent.com/u/311287868?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Tri-Adversarial Bot
Automated tri-adversarial ship-gate (L0 terra triage / L1 tier-routed lens fanout / L2 refute-by-default / L3 sol apex — Codex models).
Mapped event: COMMENT
Codex Tri-Adversarial ship-gate — PR #486
- Repo head:
fix/issue-397-governance-library-diff-workflow@63c293e - Base:
main@4ee3f0c - Files changed: 6
- Engine: four-model tri-adversarial gate on Codex — L0 triage
gpt-5.6-terra/low; L1 lens finders routedgpt-5.6-terra/low →gpt-5.6-luna/medium →gpt-5.5/xhigh (security floorgpt-5.5); L2 refute-by-defaultgpt-5.5/xhigh, top-tier findings refuted bygpt-5.6-sol/xhigh (every refutation cross-model); L3 apexgpt-5.6-sol/max. 誠實聲明：層級與 Claude 三層 gate 同構（terra≈haiku、luna≈sonnet、gpt-5.5≈opus、sol≈fable），但模型池是 Codex 的，非 Anthropic 的。
Verdict
SHIP
- 阻擋門檻 severity:
critical, high - mapped GitHub event:
COMMENT - ℹ️ 判定為 SHIP，但刻意不送 APPROVE：GitHub App 的 approving review 不計入
required_approving_review_count（2026-07-31 實測）。本報告是證據，approving 那一票請由真人帳號投。
Difficulty & routing
- overall:
high(source: terra-triage) - lens tiers: correctness→
gpt-5.5, security→gpt-5.5, simplification→gpt-5.6-luna, test-gap→gpt-5.5
Layer stats
- L1: raw=4 deduped=4 finder_failures=0
- L2: confirmed=1 refuted=3 unverified=0
- L3 final: 1
Findings (final, after apex)
[low] HTTP adapter duplicates identical POST/reply plumbing
- id:
S1lens:simplificationfile:bim-review-coordinator/src/services/governanceLibraryHttpAdapter.tsline:39 - provenance: finder=
gpt-5.6-lunarefuter=gpt-5.5(cross-model) L2=confirmed - evidence: The new
postDiffrepeats the adjacentpostRuleRunimplementation: POST with JSON headers/body, then mapstatus, content-type fallback, andresponse.text(). Only the endpoint and payload type differ. - why: KEEP at low severity. This is genuine duplicated plumbing with two future change sites, but it is not a behavioral defect or merge blocker.
- proposed fix: Extract a private
postJson(path, body)helper and delegate bothpostRuleRunandpostDiffto it.
Killed (did not survive L2/L3)
SEC-1[medium] Client-supplied model version IDs are forwarded without validation or binding to resolved library versions — The diff does not introduce this behavior: the removed app.ts implementation already forwarded both optional client-supplied IDs unchanged after resolving the IFC paths. The refactor preserves that outbound body exactly. Moreover, the IDs are not used here for path resolution or authorization, and tTG-001[medium]invalid_idsbranch for diff route has no adversarial coverage and appears unreachable from new workflow tests — The diff does not establish aninvalid_idscontract forrunLibraryDiff. Static control flow shows that method can return onlyunavailable,version_not_found, orforwarded; therefore the route’sinvalid_idscase is unreachable, likely because it switches over a shared, overly broad outcomS2[low] Port expansion forces unrelated fakes to add dead implementations — The finding is overstated. The diff showspostDiffadded to one shared governance-library workflow port, and the extrapostDiff() { throw ... }implementations are localized test doubles in the same workflow test file. Production has a single cohesive HTTP adapter for the same upstream governanc
Summary
KEEP S1 at low severity: postDiff duplicates postRuleRun request and reply handling. This is a non-blocking cleanup; extracting a small shared helper would prevent the two paths from drifting.
Agent calls
- 10/10 ok, engine wall-clock 281.6s
VERDICT
SHIP
VERDICT: SHIP
monkey1sai-blip
left a comment
monkey1sai-blip
left a comment
There was a problem hiding this comment.
Approved by monkey1sai-blip (the reviewer account pinned by the repo's merge governance).
Submitted through scripts/blip_review.py — a scripted approval carrying the operator's authority, pinned to head 63c293ed1064125ca252cd224c90c252226bb8bc. This is the mechanism the GitHub App cannot satisfy: an App's approving review does not count toward required_approving_review_count.
Summary
POST /api/governance-library/diffs.Closes #397
AI Coding Governance
@monkey1sai-blipafter required CI is greenrisk_level=CRITICALforcreateCoordinatorApp/GovernanceLibraryWorkflow, but the index was stale and collision-prone. Linked-worktree health reportscurrent_checkout_trust=unknown; compare detect-changes is unavailable and is not claimed as passed. Sol/Terra/Luna sign-off plus raw source, exact diff, and executable tests are the explicit fallback evidence.Frontend Verification
Not applicable: this is a coordinator-internal, behavior-preserving architecture cutover and changes no frontend path.
Deploy Path Verification
Not applicable: no runtime packaging, Docker, Kit, viewer, port, environment, or deploy path changed.
Windows On-Demand Verification
Not applicable: machine-derived changed paths do not match a Windows verification tier.
Self-Referential Bootstrap
Validation
runLibraryDiff/postDiffdid not yet exist; existing route parity tests stayed green.npm run buildinbim-review-coordinator: passed.npx vitest run tests/governance-library-workflow.test.ts tests/governance-library-http-adapter.test.ts tests/governance-library-routes.test.ts: 3 files, 46 tests passed.npm run verifyinbim-review-coordinator: build passed; 69 files, 856 tests passed.npx vitest run src/console/governanceClient.test.tsinweb-viewer-sample: 1 file, 12 tests passed.POST /api/governance-library/diffsExpress owner; zero runtime references to removed inline helpers.git diff --check: passed.Known Risks
502per the accepted ADR; behavior-preserving parity refers to the real HTTP JSON tree path.Summary by CodeRabbit
New Features
Bug Fixes
Tests