[elasticdotventures]

Summary

• Updates status header from "Phase 1 complete" to "Phase 3 complete"
• Adds ✅ status column to Phase 2 and Phase 3 task tables reflecting implemented code
• quarantine_agent, decommission_agent, with_policy_path, with_persist_path, trust_score_for_agent, check_tool_call_with_tx, capability_bridge.rs, McpSecurityScanner baseline are all live in lib.rs and capability_bridge.rs

Test plan

• Verify cargo check -p msft-agent-gov-ledgrrr still passes
• Confirm Phase 2/3 API presence with grep (CI)

Closes #231

🤖 Generated with Claude Code

[elasticdotventures]

Adversarial Review: docs: statechart harmonization plan + PM hooks design

Provider: sm3lly/neo-coder | Generated: 2026-10-13T13:11Z

---

🟢 Advocate

Approach Assessment

This is a well-scoped, documentation-first PR that addresses two distinct but related architectural concerns:

1. Statechart harmonization — A survey and unification plan for three separate statechart implementations across the workspace (SurfaceChart, ProcessChart, HsmChart) plus SCXML export. The approach of picking lazily as the runtime executor and the scxml crate as interchange format is pragmatic — it avoids rewriting three implementations while establishing a common runtime.

2. PM hooks design — A complete event-driven architecture for project management notifications (logging, SSE, task status) with a clear trait-based handler pattern. The design correctly treats PM state as derived from statechart transitions rather than authoritative, which is the right architectural choice.

Both documents are comprehensive, include concrete API specs, and identify open work items for follow-up PRs. This is proper spec-before-implementation practice.

Strong Points

• Clear separation of concerns: The PM hooks design explicitly states that hooks are synchronous, infallible, and never write back to the statechart — this prevents a common class of bugs where notification side-effects corrupt the primary state machine.

• Action-string-to-event mapping table: The design includes a concrete mapping from statechart action strings to PmHookEvent enum variants, making the implementation unambiguous.

• Three built-in handlers specified: Logging, SSE, and task-status handlers are fully specified with frame shapes and tool specs, giving implementers a complete picture.

• Statechart harmonization is phased: The document outlines a clear migration path (Phase 5 unblock for issue feat(statechart): Phase 5 — ufo-types DecisionNode/ForkNode/JoinNode extension for process chart completion #268) rather than a big-bang rewrite.

• No code changes: The PR correctly limits itself to documentation, avoiding the risk of introducing regressions while establishing the design baseline.

• Dependencies updated: The ufo-types dependency is updated to a newer revision, which is a legitimate maintenance change that supports the revision-io work.

• Revision module inlined: The revision module is moved from ufo-types into ledgrrr-revision-io with clear justification (it was the last revision of ufo-types that included this module). This reduces coupling and gives the project direct control over the revision data model.

Evidence

• The PR is documentation-only, so traditional test evidence doesn't apply.
• The design documents reference existing PRs (feat(http-transport): replace tiny_http with axum/SSE Streamable HTTP (#227) #264, feat(kr0ki-core): SysML-v2 ActionUsage/Succession → lazily StateChart (issue #259 phase 4) #265, feat(b00t-iface): SurfaceChart — lazily ThreadSafeStateChart live execution layer #267) and issues (feat(statechart): Phase 5 — ufo-types DecisionNode/ForkNode/JoinNode extension for process chart completion #268), showing integration with the broader project context.
• The ufo-types dependency update is a verifiable, low-risk change.
• The revision module migration includes proper re-exports and maintains backward compatibility through the public API.

Recommended to Merge If...

• The design documents are reviewed for completeness and correctness by the team.
• The ufo-types dependency update is verified to not break other workspace crates.
• The revision module migration is confirmed to maintain API compatibility.
• Open work items identified in the design documents are tracked as follow-up issues/PRs.

This is a solid, low-risk PR that establishes important architectural direction. The documentation is thorough and the design decisions are well-justified.

---

🔴 Challenger

Concerns

PR title/content mismatch (major). The PR is titled "docs: statechart harmonization plan + PM hooks design" and the summary describes only documentation work. However, the actual diff changes Cargo.toml, Cargo.lock, and adds a substantial new Rust module (crates/ledgrrr-revision-io/src/revision.rs — 905 lines) that inlines code from ufo-types. This is not a docs-only PR. Either the title/summary is wrong or the code changes are out of scope.

Dependency downgrade with feature bloat. ufo-types is downgraded from 0.16.0 to 0.15.0, and ledgrrr-revision-io gains new direct dependencies (bigdecimal, chrono, oxilangtag, oxiri, schemars, sha2) that were previously transitively provided by ufo-types. This increases the crate's dependency surface and maintenance burden. Why not stay on 0.16.0?

Code duplication via inlining. The revision.rs module is explicitly "inlined from ufo-types @ rev 5f8d018." This creates two copies of the same logic that must be kept in sync. Any bug fix or feature in the original will need manual porting. This is a maintenance liability.

No tests for new code. The PR adds ~905 lines of Rust code including validation logic, identity parsing, and model hydration/dehydration — yet claims "no test suite needed." The validate() method alone has complex multiplicity, ownership, and endpoint checks that deserve unit tests.

Breaking API change. The public API changes from ufo_types::revision::* to ledgrrr_revision_io::revision::* (visible in the book example). Any downstream consumers of the old import path will break. Is this intentional and documented?

Missing Coverage

• Unit tests for PortableModel::validate() edge cases: multiplicity violations, duplicate identities, missing endpoints, inferred facts without rules, compiler-proven facts without anchors, connection with <2 ends.
• Round-trip tests for serialization/deserialization of TypedValue, TypeRef, and the full PortableModel structure.
• Migration notes in the book or CHANGELOG explaining the ufo-types → ledgrrr_revision_io import path change.
• Rollback plan — if the inlined code proves problematic, reverting means removing the new module and bumping ufo-types back up. This should be documented.
• Dependency justification — why each new direct dependency is needed instead of being pulled transitively.

Alternative Approaches

1. Keep ufo-types at 0.16.0 and use its revision feature instead of inlining. If 0.16.0 broke something specific, document what and why inlining is preferable to pinning at 0.15.0.

2. If inlining is necessary, add a script or CI job that diffs the inlined code against the upstream source to detect drift.

3. Split the PR — docs in one PR, code changes in another. This makes review focused and reduces blast radius.

4. Use a local path dependency on a forked ufo-types instead of inlining, enabling normal git-based updates.

Blocking Issues

• Title/summary vs. actual changes mismatch must be resolved before merge — reviewers are being misled about scope.
• No tests for 905 lines of new validation logic — at minimum, tests for validate() should be added before this is considered complete.

[elasticdotventures]

Adversarial Review: docs(msft-agent-gov-ledgrrr): update SPEC.md status to Phase 3 complete

Provider: sm3lly/neo-coder | Generated: 2026-10-15T18:03Z

---

🟢 Advocate

Approach Assessment

This PR correctly updates the SPEC.md to reflect the actual implementation state. The approach is sound: rather than leaving Phase 2 and Phase 3 tasks marked as pending, it adds a Status column with ✅ done markers and updates the phase headers to "(done)". This maintains the spec as a living document that accurately represents what has been built.

The decision to mark fork_sub_agent as "✅ done (local workaround)" is honest and precise — it acknowledges that while the gap is addressed, it's not via the upstream PR path originally envisioned. This is good documentation practice.

Strong Points

• Comprehensive status tracking: Every Phase 2 and Phase 3 task now has explicit completion status, making it immediately clear what work remains vs. what's done.
• API documentation preserved: The "New public API after Phase 2" code block is retained, so readers still understand the interface contract.
• Capability bridge documented: The PR notes that CapabilityBridge is a new module with its own file, which helps maintainers understand the codebase structure.
• Non-breaking change: This is pure documentation update — no code changes, no risk to existing functionality.
• Closes issue msft-agent-gov-ledgrrr SPEC.md status header is stale (says Phase 1, code is Phase 2/3) #231: Proper issue linkage for traceability.

Evidence

The PR summary states that all listed implementations are "live in lib.rs and capability_bridge.rs". The test plan suggests verifying with cargo check and grep for API presence, which are reasonable validation steps for a documentation PR that claims implementation completeness.

Recommended to Merge If...

• The test plan items pass (cargo check -p msft-agent-gov-ledgrrr and API grep verification)
• The reviewer confirms that the claimed implementations actually exist in the codebase (spot-check a few key APIs like quarantine_agent, with_policy_path, McpSecurityScanner)
• No other pending documentation updates are needed for this spec

---

🔴 Challenger

Concerns

Status update without implementation evidence. The PR claims "Phase 3 complete" and marks tasks ✅ done, but the diff only touches SPEC.md — no code changes are shown. This is either:

• A documentation-only PR that assumes code landed elsewhere (fragile coordination)
• Premature status claiming (the code isn't actually there yet)

Either way, the PR title says "update SPEC.md status" but the summary says features are "live in lib.rs" — these are contradictory claims in the same PR.

No verification of the actual implementation. The test plan is:

• cargo check (compile only, not runtime)
• grep for API presence

This doesn't verify that quarantine_agent, decommission_agent, McpSecurityScanner, or CapabilityBridge actually work — only that symbols exist. A grep test passes for stub functions.

Phase 2/3 boundary is fuzzy. The PR lumps Phase 2 and Phase 3 into a single status update. If Phase 2 has issues, Phase 3 is built on top of them. They should be verified and merged separately.

Missing Coverage

No integration tests. The PR mentions 7 unit tests for Phase 1 but says nothing about tests for the Phase 2/3 features:

• How does quarantine_agent interact with in-flight tool calls?
• Does McpSecurityScanner correctly identify unknown tools?
• What happens when CapabilityBridge is called with a malformed capability definition?

No rollback plan. If Phase 2/3 introduces regressions, how do you revert to Phase 1 status? The SPEC.md change is trivial, but the code changes (wherever they landed) need a rollback story.

No migration docs. Phase 2 introduces API renames (trust_score(did) → trust_score_for_agent(id)). Who calls the old API? Is there a deprecation warning? Breaking changes need migration notes.

No monitoring/alerting. Phase 3 adds security scanning and capability bridging — these are failure-prone paths. What metrics are exposed? What logs are emitted when a tool call is denied or an agent is quarantined?

Alternative Approaches

Separate PRs per phase. Phase 2 and Phase 3 should be separate PRs with their own code, tests, and review. Bundling them makes it harder to identify which phase introduced a problem.

Code-first, docs-second. The implementation PRs should reference the SPEC.md status update, not the other way around. The status update should be an automatic consequence of code landing, not a manual claim.

Automated status tracking. Instead of manually updating SPEC.md, use a CI job that checks which features are implemented and updates the status automatically. This eliminates the "claim vs. reality" gap.

Blocking Issues

None. This is a documentation update, but it's premature and unverified. It should be merged after the implementation PRs are verified, not before or in parallel.

Recommendation: Hold this PR until the implementation code lands and passes integration tests. Then merge the SPEC.md update as a follow-up.