feat(architecture): executable lifecycle contracts for the three coordinator machines (Phase 4) - #475
Conversation
…contracts (Phase 4) Adds architecture/lifecycle-contract.json (+schema) declaring the review-session, endpoint-lease, and stage-binding machines as they run today, a standard-library checker that proves machine well-formedness, TS-union state-set synchronization, and readiness-binding consistency (ARCH-LIFECYCLE-001, active), and 52 model-based tests that enumerate every simple path for forbidden shortcuts and evidence gates. Honest records: review-session.failed is declared-only (zero write paths); created-to-active has no runtime transition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
…mmit The introduce-executable-architecture-contracts row now records subject_commit 7c5dd98 (the commit carrying the Phase 4 checklist and delivery evidence), with task_ledger 23/26 and the current slice moved to Phase 5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
|
Warning Review limit reached
Next review available in: 14 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesLifecycle contract model
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as check_lifecycle_contracts.py
participant Validator as lifecycle_contracts.py
participant Contract as lifecycle-contract.json
participant TS as TypeScript unions
participant Architecture as architecture-contract.json
CLI->>Validator: Run lifecycle validation
Validator->>Contract: Load state machines and rules
Validator->>TS: Compare declared state sets
Validator->>Architecture: Check policy and ownership bindings
Validator-->>CLI: Return validation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
architecture/README.md (1)
112-128: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the required virtual-environment interpreter for pytest.
System
pythoncan load conflicting user-site FastAPI, Starlette, or uvicorn packages. Rerun the suite with the required interpreter. Update the documented command and the delivery evidence after that run.
architecture/README.md#L112-L128: replace each pytest command with./.venv/Scripts/python.exe -m pytest tests -p no:cacheprovider.openspec/changes/introduce-executable-architecture-contracts/tasks.md#L350-L353: rerun the reported full suite with./.venv/Scripts/python.exe -m pytest tests -p no:cacheproviderand update the recorded result.🤖 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 `@architecture/README.md` around lines 112 - 128, Use the required virtual-environment interpreter for the pytest commands instead of system python. In architecture/README.md at 112-128, replace each pytest invocation in the architecture/layer/lifecycle contract examples with the venv-based full-suite command shown in the review, and in openspec/changes/introduce-executable-architecture-contracts/tasks.md at 350-353 rerun the reported suite with the same interpreter and update the recorded delivery evidence/result accordingly.Source: Coding guidelines
🧹 Nitpick comments (5)
scripts/lib/lifecycle_contracts.py (1)
596-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the single-declaration assumption of
_extract_union_literals.
pattern.searchreturns the first match only, and the body is captured as[^;]*up to the first;. Two inputs are therefore silently narrowed rather than reported:
- A union declared without a trailing
;. The body then runs into the next statement, residue is non-empty, and the result isunsupported_union. That outcome is fail-closed, so it is safe, but the reason code does not explain the real cause.- The same type name declared twice in one file. Only the first declaration is compared.
The fail-closed posture holds in both cases. A short note in the docstring records the assumption for the next maintainer.
♻️ Proposed docstring addition
Returns ``(literals, None)`` on success or ``(None, reason)`` when the union cannot be interpreted. Anything but a pure string-literal union is rejected (fail closed) rather than partially parsed: a referenced type alias, a comment, or single-quoted literals all leave residue. + + Two assumptions hold: the declaration ends with ``;`` (the body is read up + to the first semicolon), and the type name is declared once per file (only + the first match is compared). A missing semicolon leaves residue and is + reported as ``unsupported_union``. """🤖 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/lib/lifecycle_contracts.py` around lines 596 - 621, Update the docstring of _extract_union_literals to explicitly document that it assumes exactly one semicolon-terminated declaration of the requested type and only evaluates the first matching declaration; note that missing terminators or duplicate declarations are not separately detected and remain fail-closed.architecture/lifecycle-contract.schema.json (1)
67-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: forbid
..segments inrepoRelativePathat the schema level.The pattern
^[A-Za-z0-9_.-]+(?:/[A-Za-z0-9_.-]+)*$accepts..as a segment, sobim-review-coordinator/../secrets.tsis schema-valid._check_source_syncinscripts/lib/lifecycle_contracts.py(lines 640-650) already rejects that input, so this is defense in depth only. Tightening the pattern moves the rejection into the schema and keeps both layers aligned.♻️ Proposed pattern tightening
"repoRelativePath": { "type": "string", - "pattern": "^[A-Za-z0-9_.-]+(?:/[A-Za-z0-9_.-]+)*$" + "pattern": "^(?!.*(?:^|/)\\.\\.?(?:/|$))[A-Za-z0-9_.-]+(?:/[A-Za-z0-9_.-]+)*$" },Note: the repository validator implements a Draft-07 subset. Confirm it supports the lookahead before adopting this pattern; otherwise keep the current pattern and rely on the checker.
🤖 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 `@architecture/lifecycle-contract.schema.json` around lines 67 - 70, Update the repoRelativePath schema pattern to reject any path segment equal to "..", while preserving valid relative paths and existing checker behavior. Before using a lookahead, verify that the repository’s Draft-07 validator supports it; otherwise leave the pattern unchanged and rely on _check_source_sync for this validation.scripts/dev/check_lifecycle_contracts.py (1)
76-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDerive the failure decision from
result.status.The exit code uses
result.error_countonly.LifecycleCheckResult.statusalso returns"failed"whencomparedisFalse. Today every non-compared path records at least one error, so the two agree. If a future load path returnscompared=Falsewithout an error issue, the JSON payload would report"failed"while the process exits0, and the gate would pass silently. Readingstatuskeeps the CLI and the payload aligned by construction.🛡️ Proposed fix
- failed = result.error_count > 0 or (args.strict and result.warning_count > 0) + failed = result.status != "passed" or (args.strict and result.warning_count > 0) return 1 if failed else 0🤖 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/dev/check_lifecycle_contracts.py` around lines 76 - 77, Update the exit-code decision near the final return to derive failure from LifecycleCheckResult.status, treating a "failed" status as a nonzero exit while preserving the existing strict warning behavior as part of status computation. Keep the CLI exit code aligned with the JSON-reported result for all compared and non-compared outcomes.tests/test_lifecycle_contracts.py (2)
767-775: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnchor the mutation to the
SessionStatusunion.The sibling test at line 759 asserts the exact substring
'"closing" | "closed" | "failed";'before mutating, which pins the target declaration. This test replaces the first'"failed";'occurrence in the whole file. If an unrelated"failed";literal is added earlier intypes.ts, the mutation lands on the wrong declaration and the test fails with a misleading reason. Reuse the anchored substring.♻️ Proposed fix
def test_source_sync_state_missing_in_contract_rejected(tmp_path) -> None: source_file, _ = PINNED_SOURCE_BINDINGS["review-session"] text = (ROOT / source_file).read_text(encoding="utf-8") - mutated = text.replace('"failed";', '"failed" | "paused";', 1) - assert mutated != text + anchor = '"closing" | "closed" | "failed";' + assert anchor in text + mutated = text.replace(anchor, '"closing" | "closed" | "failed" | "paused";', 1) repo = build_tmp_repo(tmp_path, source_overrides={source_file: mutated})🤖 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 `@tests/test_lifecycle_contracts.py` around lines 767 - 775, Update test_source_sync_state_missing_in_contract_rejected to anchor its mutation to the SessionStatus union by first asserting the exact expected union substring, then replacing that anchored declaration with the paused variant. Preserve the existing repository setup and lifecycle contract assertions.
441-625: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeveral fail-closed branches have no counterexample test.
The file states that the suite protects the gate against silent loosening. These error branches of
scripts/lib/lifecycle_contracts.pyare not exercised by any counterexample:
lifecycle.machine.no_initialandlifecycle.machine.no_terminal(lines 334-349).lifecycle.state.kindfor an invalidkindvalue (lines 312-319).lifecycle.forbidden.duplicateandlifecycle.forbidden.duplicate_id(lines 478-504).lifecycle.reentry.unknown_stateandlifecycle.reentry.duplicate(lines 535-551).lifecycle.cross.unlisted_machineandlifecycle.cross.duplicate(lines 738-746, 805-812).lifecycle.evidence.duplicateandlifecycle.readiness.duplicate_evidence(lines 508-515, 837-844).lifecycle.readiness.providerfor a missing provider (lines 848-856).lifecycle.source_sync.binding_missingfor an emptysource_binding(lines 630-639).Each one follows the existing pattern: mutate the loaded contract, call
check_lifecycle_contracts, assert the code. Untested branches can regress without any test turning red.Do you want me to generate the missing counterexample tests?
🤖 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 `@tests/test_lifecycle_contracts.py` around lines 441 - 625, Extend the fail-closed test section with counterexamples for every listed issue code: mutate the contract to remove initial or terminal states, use an invalid state kind, duplicate forbidden shortcuts or IDs, add unknown or duplicate reentry entries, duplicate or unlisted cross-machine declarations, duplicate evidence and readiness evidence, omit a readiness provider, and clear source_binding. For each case, follow the existing load_contract/build_tmp_repo/check_lifecycle_contracts pattern and assert the corresponding issue code in issue_codes(result).
🤖 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 `@architecture/architecture-contract.json`:
- Around line 365-374: Update the ARCH-LIFECYCLE-001 enforcement entry so its
active lifecycle-contract-gate mode is backed by an existing dispatch table and
runner command; if no such gate exists, mark the rule delegated instead of
active. Ensure the configured enforcement mode matches an actually executed
gate.
In `@scripts/lib/lifecycle_contracts.py`:
- Around line 33-36: Declare Python 3.10 or newer as the supported runtime for
lifecycle_contracts.py through the repository’s existing project metadata or
version-guard convention. Keep the current typing.TypeGuard import and
dataclass(frozen=True, slots=True) usage unchanged; do not add Python 3.9
compatibility unless the repository requires retaining that support.
---
Outside diff comments:
In `@architecture/README.md`:
- Around line 112-128: Use the required virtual-environment interpreter for the
pytest commands instead of system python. In architecture/README.md at 112-128,
replace each pytest invocation in the architecture/layer/lifecycle contract
examples with the venv-based full-suite command shown in the review, and in
openspec/changes/introduce-executable-architecture-contracts/tasks.md at 350-353
rerun the reported suite with the same interpreter and update the recorded
delivery evidence/result accordingly.
---
Nitpick comments:
In `@architecture/lifecycle-contract.schema.json`:
- Around line 67-70: Update the repoRelativePath schema pattern to reject any
path segment equal to "..", while preserving valid relative paths and existing
checker behavior. Before using a lookahead, verify that the repository’s
Draft-07 validator supports it; otherwise leave the pattern unchanged and rely
on _check_source_sync for this validation.
In `@scripts/dev/check_lifecycle_contracts.py`:
- Around line 76-77: Update the exit-code decision near the final return to
derive failure from LifecycleCheckResult.status, treating a "failed" status as a
nonzero exit while preserving the existing strict warning behavior as part of
status computation. Keep the CLI exit code aligned with the JSON-reported result
for all compared and non-compared outcomes.
In `@scripts/lib/lifecycle_contracts.py`:
- Around line 596-621: Update the docstring of _extract_union_literals to
explicitly document that it assumes exactly one semicolon-terminated declaration
of the requested type and only evaluates the first matching declaration; note
that missing terminators or duplicate declarations are not separately detected
and remain fail-closed.
In `@tests/test_lifecycle_contracts.py`:
- Around line 767-775: Update
test_source_sync_state_missing_in_contract_rejected to anchor its mutation to
the SessionStatus union by first asserting the exact expected union substring,
then replacing that anchored declaration with the paused variant. Preserve the
existing repository setup and lifecycle contract assertions.
- Around line 441-625: Extend the fail-closed test section with counterexamples
for every listed issue code: mutate the contract to remove initial or terminal
states, use an invalid state kind, duplicate forbidden shortcuts or IDs, add
unknown or duplicate reentry entries, duplicate or unlisted cross-machine
declarations, duplicate evidence and readiness evidence, omit a readiness
provider, and clear source_binding. For each case, follow the existing
load_contract/build_tmp_repo/check_lifecycle_contracts pattern and assert the
corresponding issue code in issue_codes(result).
🪄 Autofix
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: dd09a2d1-32db-487c-a06d-567fd89298cd
📒 Files selected for processing (12)
architecture/README.mdarchitecture/architecture-contract.jsonarchitecture/deltas/introduce-executable-architecture-contracts.jsonarchitecture/lifecycle-contract.jsonarchitecture/lifecycle-contract.schema.jsonopenspec/changes/introduce-executable-architecture-contracts/design.mdopenspec/changes/introduce-executable-architecture-contracts/tasks.mdopenspec/lifecycle-ledger.jsonscripts/dev/check_lifecycle_contracts.pyscripts/lib/lifecycle_contracts.pyscripts/verification-manifest.jsontests/test_lifecycle_contracts.py
There was a problem hiding this comment.
🔵 Human review recommended
Although I found no concrete defects and verified the load-bearing claims, the change modifies governance/CI-gating machinery itself (a new active root-contracts invariant, verification-manifest wiring, and openspec machine-truth ledger edits) and the author explicitly requested CODEOWNERS review.
Pull request overview
This PR delivers Phase 4 of the introduce-executable-architecture-contracts OpenSpec change: it makes the three coordinator-owned runtime lifecycles (review-session, endpoint-lease, stage-binding) machine-checkable. It follows the established Phase 1–3 pattern (a JSON contract + Draft-07 schema + stdlib-only checker + model-based tests, wired into the root-contracts gate), and adds a new active architecture invariant ARCH-LIFECYCLE-001. The gate proves contract well-formedness, bidirectional state-set synchronization with the owning TypeScript unions, and readiness-binding equality with the review-session-ready policy — explicitly not runtime transition behavior.
I verified the load-bearing claims: the three source_binding unions (SessionStatus, ViewerLeaseStatus, StageBindingStatus) exist and match the pinned states exactly; sessionStore.setStatus has only test-file callers (so failed being declared_only is accurate); the readiness policy's 5 required-evidence ids match the binding; the ledger 23/26 matches the tasks.md checkbox count (26 total, 23 checked); the new invariant id is permitted (REQUIRED_INVARIANT_IDS is a must-be-present set, not an allowlist — ARCH-LAYER-001 is likewise absent from it); and the manifest was updated in both path_globs locations.
Changes:
- New executable lifecycle contract (
architecture/lifecycle-contract.json+ Draft-07 schema), stdlib checker (scripts/lib/lifecycle_contracts.py), CLI (scripts/dev/check_lifecycle_contracts.py), and 52 model-based/fail-closed tests. - New active invariant
ARCH-LIFECYCLE-001in the architecture contract + additive delta entries; gate wired into theroot-contractsverification target. - Governance bookkeeping: lifecycle-ledger row → 23/26 (slice → Phase 5), plus tasks/design/README delivery records and honest-limits disclosures.
File summaries
| File | Description |
|---|---|
scripts/lib/lifecycle_contracts.py |
Core checker: machine semantics, source-union sync, cross-machine rules, readiness binding, unused-evidence warnings. |
scripts/dev/check_lifecycle_contracts.py |
Developer/CI CLI entry point with human/JSON output and strict mode. |
tests/test_lifecycle_contracts.py |
Pinned-literal + model-based property tests and ~30 fail-closed counterexamples. |
architecture/lifecycle-contract.json |
Machine definitions (states/transitions/forbidden shortcuts/evidence/reentry/cross-machine/readiness). |
architecture/lifecycle-contract.schema.json |
Draft-07 structural schema for the contract. |
architecture/architecture-contract.json |
Adds active ARCH-LIFECYCLE-001 invariant. |
architecture/deltas/introduce-executable-architecture-contracts.json |
Additive contract + state_machine_changes declarations. |
architecture/README.md |
Documents the new files, rule row, run commands, and Phase 4 known limits. |
scripts/verification-manifest.json |
Adds the two new scripts to the root-contracts path class and target globs. |
openspec/lifecycle-ledger.json |
Advances the change row to 23/26 and updates last_verified/subject_commit. |
openspec/changes/.../tasks.md |
Marks 4.1–4.4 done and records the Phase 4 delivery/validation notes. |
openspec/changes/.../design.md |
Marks Phase 4 complete with the current-runtime-truth framing. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…ation fix Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
![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: 681371f597
ℹ️ 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".
![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 #475
- Repo head:
feat/phase4-lifecycle-contracts@5e8b0fd - Base:
main@afa5c73 - Files changed: 12
- 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.5, test-gap→gpt-5.5
Layer stats
- L1: raw=4 deduped=4 finder_failures=0
- L2: confirmed=3 refuted=1 unverified=0
- L3 final: 3
Findings (final, after apex)
[medium] Strict warning failures are reported as PASSED in CLI output
- id:
L1-COR-001lens:correctnessfile:scripts/dev/check_lifecycle_contracts.py - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence: The CLI renders
result.status, which ignores warnings, before computingfailed = result.error_count > 0 or (args.strict and result.warning_count > 0). Therefore errors=0, warnings>0, and--strictyieldsstatus="passed"/PASSEDbut exit code 1. - why: This contradicts the CLI promise that strict mode treats warnings as a failing result and makes human and JSON output disagree with the process outcome. The exit code still protects compliant CI, limiting severity to medium.
- proposed fix: Compute the effective strict-mode outcome before rendering and use it for human and JSON status, or expose a separate strict-aware status while preserving the library’s error-only status semantics.
[medium] --strict warning failure path is untested
- id:
TG-001lens:test-gapfile:tests/test_lifecycle_contracts.py - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence: The warning-only test calls the library and expects
status == "passed"; the only strict subprocess test runs against the warning-free canonical repository. No test combines warnings with--strict. - why: Removing the strict warning condition from the CLI would not be detected by the shown tests, weakening the documented lifecycle gate. This is a central CI behavior rather than an incidental branch.
- proposed fix: Run the CLI against the dangling-evidence temporary repository with
--strict; assert exit code 1, the warning code, and the intended strict-aware rendered status.
[low] --output path behavior has no failing or success coverage
- id:
TG-002lens:test-gapfile:tests/test_lifecycle_contracts.py - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence: The new
_emitfile branch resolves relative paths underrepo_root, creates parent directories, writes the report, and suppresses stdout, but no test passes--output; the only subprocess assertion covers stdout. - why: The newly exposed file-writing behavior could regress without detection. Severity remains low because the implementation is small and the diff shows no current functional defect.
- proposed fix: Add a subprocess test using a nested relative
--outputpath and assert exit code, parent/file creation, valid report content, and empty stdout.
Killed (did not survive L2/L3)
S1[low] Negative lifecycle tests repeat the same mutate/build/assert pattern — The repeated lines are a minimal, conventional arrange/act/assert sequence, while each test encodes a distinct contract mutation. The file already centralizes repository construction and issue-code extraction inbuild_tmp_repoandissue_codes. A parametrized table of mutator callables would hide
Summary
KEEP all three survivors at their submitted severities. Make rendered status agree with strict-mode failure, then add a warning-only strict subprocess regression. Also cover the new --output file path; that gap remains low severity.
Agent calls
- 10/10 ok, engine wall-clock 374.9s
VERDICT
SHIP
VERDICT: SHIP
…ode failure Adopts the codex ship-gate findings on PR #475: under --strict a warning-only run exits 1, so the rendered human verdict and the JSON payload now carry a strict-aware cli_status (the library's error-only status is preserved), and two subprocess regressions cover the warning+strict failure path and the --output file branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c652169d2
ℹ️ 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".
Adopts the PR #475 review findings (codex connector P2s + gate report): - ARCH-LIFECYCLE-001 and ARCH-LAYER-001 join REQUIRED_INVARIANT_IDS, so deleting either invariant from the desired architecture now fails the semantic validator instead of passing silently. - The source-union scan is line-anchored and rejects duplicate anchored declarations, so a commented-out stale union can no longer mask real state drift. - Non-terminal, non-declared-only states must have an outgoing transition (lifecycle.state.dead_end); demoting a terminal state to intermediate now fails. - Readiness bindings restate the policy's declared source (policy_source) and a service provider must BE that source, so re-binding evidence to another known service fails instead of hiding readiness-source drift; the full (evidence, provider, source) wiring is pinned in tests. - The stage-binding failed-reentry description now narrows replay to records carrying a completion outcome, matching the runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
…eview round Adopts the five codex-connector P2s on the hardening commit: - reentry_rules joins the machine schema's required set and cross_machine_rules gains minItems 1, with the two canonical rules (gate + cascade) pinned literally in tests. - failure_code is retained in the transition model and validated: attribution only on terminal entries, and every entry into the same target must agree on whether it is attributed. - The two authorization-unavailable transitions declare their real attempt-binding-match evidence (failBeforeMutation verifies identity fields before recording the failure). - The nonce-replay reentry rule states the explicit-role condition the runtime enforces (a changed explicit role is not a replay). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de383955b6
ℹ️ 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".
…olicy dedup, unhashable-id guard Fourth connector round: the union scan strips // and /* */ comments string-aware before matching (a block-commented copy or a re-exported declaration now fails closed, unterminated blocks too); cancel-pending declares its real cancellation-intent-match evidence; duplicate readiness policy ids fail closed instead of silently binding the last copy; and non-string evidence ids are guarded before dictionary lookups so a malformed contract yields the structured failed result rather than a TypeError traceback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
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 157bd79ebb7f123fd3e14352bf541ed4b2bace89. This is the mechanism the GitHub App cannot satisfy: an App's approving review does not count toward required_approving_review_count.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 157bd79ebb
ℹ️ 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".
| coded_targets: dict[str, bool] = {} | ||
| for item in transition_models: | ||
| has_code = item.failure_code is not None | ||
| if has_code and kinds.get(item.to_state) != "terminal": |
There was a problem hiding this comment.
Fresh evidence after the earlier failure-attribution fix: changing pending-expire.failure_code from pending_expired to wrong_code, or adding runtime_stage_load_failed to confirm-load-success, still reports passed with zero findings. The validation collapses the value to has_code and checks only whether the target is terminal, so it neither preserves the diagnostic taxonomy implemented by StageBindingState.sweep() nor prevents a success transition from carrying a failure. Validate the expected code for each canonical transition, or pin the complete transition-to-code mapping.
AGENTS.md reference: AGENTS.md:L132-L139
Useful? React with 👍 / 👎.
| policy_sources = { | ||
| rid: item.get("source") | ||
| for item in _list_of_mappings(policy.get("required_evidence")) | ||
| if _non_empty_string(rid := item.get("id")) |
There was a problem hiding this comment.
If review-session-ready.required_evidence contains the same ID twice, this dictionary silently keeps one row instead of rejecting the ambiguous policy. I verified that adding a second kit-process-alive row with the same source but side: "browser" leaves both validate_repository() and check_lifecycle_contracts() passing, and all 67 lifecycle tests remain green; consumers keyed by evidence ID can therefore disagree about which side owns the evidence. Detect duplicate IDs before constructing policy_sources.
Useful? React with 👍 / 👎.
| for requirement in _list_of_mappings(rule.get("required_states")): | ||
| machine_id = requirement.get("machine") | ||
| if machine_id not in machines: |
There was a problem hiding this comment.
When one cross-machine rule contains two required_states entries for the same machine, each entry is validated independently and no ambiguity is reported. I verified inserting a review-session: [closed] requirement before the canonical review-session: [created, active] requirement still passes all 67 lifecycle tests, because the pinned test also reduces the list to a dictionary and keeps the later row; the executable rule can thus simultaneously require mutually exclusive states. Enforce one requirement per machine within each rule.
Useful? React with 👍 / 👎.
| machine_id = requirement.get("machine") | ||
| if machine_id not in machines: |
There was a problem hiding this comment.
If a schema-invalid required_states[].machine is an array or object, this membership test hashes the raw value and raises TypeError instead of returning the schema findings already collected. The same occurs for malformed cascade machine IDs, so editing a contract into an invalid intermediate state terminates the CLI with a traceback rather than its documented structured, fail-closed result. Check for a non-empty string before every machines lookup.
Useful? React with 👍 / 👎.
| stripped, comment_error = _strip_ts_comments(text) | ||
| if stripped is None: | ||
| return None, comment_error | ||
| pattern = re.compile( | ||
| r"(?m)^[ \t]*export\s+type\s+" + re.escape(type_name) + r"\s*=\s*(?P<body>[^;]*);" | ||
| ) | ||
| matches = list(pattern.finditer(stripped)) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier comment-stripping fixes: _strip_ts_comments() deliberately preserves quoted content, so the anchored regex accepts a multiline template-literal example as executable source. I verified that a file containing const example = `\nexport type SessionStatus = ...;\n`; plus a live re-export returns the stale literals successfully; moving the real union while leaving such documentation would therefore let source-state drift pass. Blank template-string content before matching declarations, or use a TypeScript-aware parser.
AGENTS.md reference: AGENTS.md:L132-L139
Useful? React with 👍 / 👎.
| for machine in machines.values(): | ||
| if machine.owner_service not in known_services: | ||
| issues.append( |
There was a problem hiding this comment.
The owner check accepts any service declared by the architecture contract, even when it contradicts the machine's coordinator source binding. I verified changing review-session.owner_service to web-viewer-sample still passes the checker and all 67 lifecycle tests, despite its type remaining in bim-review-coordinator/src/types.ts; this allows lifecycle authority to drift silently across service boundaries. Validate the canonical machine-to-owner mapping, or derive ownership from an authoritative architecture capability mapping.
AGENTS.md reference: AGENTS.md:L88-L95
Useful? React with 👍 / 👎.
| "evidence_required": [ | ||
| "attempt-binding-match" | ||
| ], |
There was a problem hiding this comment.
When a pending transaction survives until its session closes or its primary lease becomes invalid, RuntimeMutationAuthority.authorizeRuntimeCommand() rejects the request before calling StageBindingState.consume(): it requires the session to remain created/active and inspectRuntimeLease() to authorize an active primary lease. This transition declares only attempt-binding-match, while the cross-machine rule covers creation of the preauthorization rather than later Kit consumption, so the executable contract overstates when pending -> executing is allowed. Add the session/lease requirements to this transition or to a consumption-specific cross-machine rule.
AGENTS.md reference: AGENTS.md:L132-L139
Useful? React with 👍 / 👎.
| "id": "release-lease", | ||
| "from": "active", | ||
| "to": "released", | ||
| "trigger": "release-lease", | ||
| "evidence_required": [], | ||
| "effects": [], |
There was a problem hiding this comment.
For the public caller-initiated release flow, app.ts always passes X-Viewer-Lease-Token to ViewerLeaseStore.release(), and the store leaves an active lease unchanged when that token or the session does not match. Declaring an empty evidence set therefore says the active -> released transition has no authority precondition even though missing or incorrect credentials prevent it at runtime. Declare and require lease/session match evidence for this transition so the contract reflects the actual release gate.
AGENTS.md reference: AGENTS.md:L132-L139
Useful? React with 👍 / 👎.
…ommit (#477) Follows the Phase 3 convention (its row binds c5b9089, the #464 squash): after #475 squashed, the pre-merge branch commit 7c5dd98 stopped being reachable from any fresh clone, which fails machine-truth test 25 (subject_unavailable - the no-base call path cannot run the #474 watermark derivation) on every subsequent PR. The row now binds e7bb0b9, the #475 squash that carries the final Phase 4 tasks.md, which is permanently reachable main history. Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
Phase 4 of
introduce-executable-architecture-contracts: the three coordinator-owned runtime lifecycles —review-session,endpoint-lease,stage-binding— become executable contracts (tasks 4.1–4.4).architecture/lifecycle-contract.json(+ Draft-07 schema) declares each machine as it runs today: states, observed transitions (triggers, evidence, failure codes, effects), forbidden shortcuts with their runtime enforcement, reentry rules, two cross-machine rules, and the readiness evidence binding.scripts/lib/lifecycle_contracts.py(stdlib-only; reuses the Phase 1/3 loader and schema validator) proves three things: machine well-formedness (initial reachability, closed terminals, no direct edge on any forbidden pair, deterministic(from, trigger), full reference integrity), TS-union state-set synchronization (SessionStatus/ViewerLeaseStatus/StageBindingStatusliterals must equal the contract exactly — editing either side alone fails; non-literal unions fail closed), and readiness-binding equality with thereview-session-readypolicy. Wired intoroot-contractsdispatch;ARCH-LIFECYCLE-001is markedactivebecause the gate actually runs there.tests/test_lifecycle_contracts.py(52 tests) is model-based: it enumerates every simple path of the canonical model and asserts properties over all of them — forbidden pairs have no single-step edge, every pending→active path crossesexecutingwith evidence union exactly{attempt-binding-match, runtime-load-outcome}, lease terminals cannot reachactive, terminals are closed, every state is reachable or declared-only. Pinned literals (states / forbidden pairs / evidence-gated transitions / source bindings / readiness evidence / schema load-bearing keys) make loosening the contract visible in the same diff. ~30 fail-closed counterexamples cover loading, machine semantics, cross-machine rules, readiness, and source-sync mutations.Honest records, not aspirations:
review-session.failedis declared-only (the union declares it; zero runtime write paths —sessionStore.setStatushas no callers), andcreated→activehas no runtime transition (activation is decided at creation time), so the contract marks bothcreatedandactiveinitial instead of inventing an edge. Wiring either path later is a behavioral state-machine change that must be declared in a delta.The lifecycle-ledger row moves to 23/26 with the current slice at Phase 5, and a follow-up commit binds
subject_committo the tasks.md commit (the #474 watermark mechanism covers post-squash reachability).Behavior contract changed = yesrefers to the governance machine contract surface: the PR addsarchitecture/lifecycle-contract.json(+ schema) and activatesARCH-LIFECYCLE-001(declared additively in the delta'spublic_contract_changesandstate_machine_changes). No product API, event contract, or runtime behavior changes.Validation (Windows governed worktree, Python 3.12.7 / pytest 8.2.2 / jsonschema 4.25.1)
python -m pytest tests -q -p no:cacheprovider— 445 passed, 9 skipped (clean-tree base 393; +52 from this change)python scripts/dev/check_lifecycle_contracts.py --repo-root . --strict— PASSED: 3 machines, 13 states, 15 transitions, 0 errors, 0 warningspython scripts/dev/check_layered_architecture.py --repo-root . --strict— PASSED (Phase 3 ratchet untouched)python scripts/dev/export_observed_architecture.py --repo-root . --strict— PASSED (Phase 2 ratchet untouched)python scripts/dev/validate_architecture_contract.py --repo-root . --strict— PASSED (ARCH-LIFECYCLE-001and the delta'sstate_machine_changesaccepted by the Phase 1 semantic validator)node scripts/tests/test-verification-plan.mjs— 23/23 (the new checker paths dispatchroot-contracts)node scripts/tests/test-openspec-machine-truth.mjs— 24/24 (after the ledger edit and the subject bind)npx openspec validate introduce-executable-architecture-contracts --strict— passednpx openspec validate --all --strict— 71 passed, 0 failedgit diff --check— cleanKnown Risks
architecture/README.md§ "Phase 4 的已知偏離與界線" spells out every limit.🤖 Generated with Claude Code
https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6