Close governance traceability tasks and finish T0-4/T0-5 hardening - #188
Close governance traceability tasks and finish T0-4/T0-5 hardening#188mindfn wants to merge 13 commits into
Conversation
Complete the remaining closeout items for enhance-doc-governance-traceability with fresh verification evidence and TODO truth alignment. Key changes: - extend check_governance_traceability.sh to: - validate frontmatter contracts for active and archived feature aggregation docs - enforce Claim Ledger active/done rows with non-pending change ids map to real OpenSpec tasks.md artifacts - enforce project_overall_todos Detail Claim Ref resolution and non-pending change-id consistency against agentscope_domain_execution_todos - tighten same-record TODO/change matching to Claim Ledger columns and ignore deprecated claims - add regression coverage in tests/unit/test_governance_traceability_gate.py for: - archived feature frontmatter missing required status - missing tasks artifact for active/done claim mappings - missing master->detail claim references - archived-change mapping acceptance path updates - mark OpenSpec change tasks 3.2/3.3/3.4/5.3 done with concrete evidence links - close feature aggregation doc status to done and refresh verification/risk narrative - sync project_overall_todos baseline and T0-6 claim/item status to done Rationale: - remove governance drift between claim ledgers, change artifacts, and feature evidence records - increase CI gate depth to catch stale or inconsistent closeout states before merge - keep docs-first governance records machine-checkable and auditable.
Prepare deterministic closeout guidance for p0-conformance-gate task 3.2, which depends on repo-admin branch protection changes outside the repository. Key changes: - add a dedicated 'P0-Gate Required Check Rollout (Admin Checklist)' section to docs/governance/branch-protection.md - preconditions for promotion readiness - exact execution steps in branch protection/rulesets - acceptance criteria (blocked-on-fail and unblocked-on-pass) - evidence bundle required for governance closeout - update docs/features/p0-conformance-gate.md - refresh frontmatter updated date - record the new rollout-checklist artifact in Results - tighten Next Milestone to execute checklist and backfill evidence links - update openspec/changes/p0-conformance-gate/tasks.md item 3.2 note - link to the rollout checklist - define closeout evidence expectation (settings + blocked/pass run links) Rationale: - task 3.2 cannot be closed by code-only changes; it needs admin execution plus auditable proof - documenting one canonical checklist reduces ambiguity, prevents partial rollout, and keeps docs-first governance evidence machine-reviewable.
…r import rule Advance T0-4 (__init__.py facade compliance) by removing wildcard exports from the public model facade and hardening regression checks. Key changes: - replace wildcard import in dare_framework/model/__init__.py - from: from dare_framework.model.adapters import * - to: explicit imports of OpenAIModelAdapter and OpenRouterModelAdapter - extend tests/unit/test_package_initializers_facade_pattern.py with Rule 5 - package initializers now explicitly prohibit star imports (import *) - preserves existing docstring/definition/assignment/import facade constraints Why: - wildcard exports make public API surface implicit and fragile - facade initializers should remain deterministic, reviewable, and intentionally exported - adding a concrete rule prevents future regressions from reintroducing wildcard initializer exports Verification: - .venv/bin/python -m pytest -q tests/unit/test_package_initializers_facade_pattern.py tests/unit/test_default_model_adapter_manager.py tests/unit/test_prompt_store.py tests/unit/test_context_implementation.py => 30 passed - smoke import check confirms both adapters remain available from dare_framework.model.
Advance T0-5 by making the "failed test -> responsibility module -> owner" mapping explicit, machine-checkable, and continuously enforced in CI. What changed: - scripts/ci/p0_gate.py - extend CategorySpec with owner field - assign owner for each P0 category - extend FAIL summary contract to print owner alongside tests/modules/action - scripts/ci/check_test_failure_ownership.py (new) - add deterministic validator for P0 failure ownership mapping - enforce non-empty tests/modules/owner - enforce owner format as GitHub handle (@...) - detect duplicate test selector mappings across categories - provide CI-friendly pass/fail output - tests/unit/test_p0_gate_ci.py - update summary contract assertion to require owner line - tests/unit/test_check_test_failure_ownership.py (new) - add regression coverage for valid mapping, duplicate selector detection, and missing owner rejection - .github/workflows/ci-gate.yml - add failure-ownership-map job running python scripts/ci/check_test_failure_ownership.py - docs/guides/P0_Gate_Runbook.md - document ownership mapping health-check command and CI job linkage - openspec/changes/p0-conformance-gate/tasks.md - refresh task 3.3 evidence to include owner contract and routine ownership mapping check - docs/features/p0-conformance-gate.md - append result evidence for ownership-map check and CI integration - docs/todos/project_overall_todos.md - mark T0-5 done with evidence and set AG11 claim active (T0-4 ongoing) Why: - before this change, p0-gate triage exposed failing tests and modules but had no explicit owner signal - mapping integrity was implicit in code and not continuously validated - adding a dedicated CI check turns ownership mapping into an auditable, enforceable governance contract Verification: - .venv/bin/python -m pytest -q tests/unit/test_p0_gate_ci.py tests/unit/test_check_test_failure_ownership.py => 7 passed - .venv/bin/python -m pytest -q tests/unit/test_p0_gate_ci.py => 4 passed - python scripts/ci/check_test_failure_ownership.py => [failure-ownership] passed - .venv/bin/python scripts/ci/p0_gate.py => p0-gate: PASS (all categories 0 failures) - ./scripts/ci/check_governance_traceability.sh => passed - ./scripts/ci/check_governance_evidence_truth.sh => passed - openspec validate p0-conformance-gate --type change --strict --json --no-interactive => valid (1/1, 0 issues)
…izers Continue T0-4 (__init__.py facade compliance) by enforcing and implementing a stricter public-facade boundary contract. What changed: - strengthened tests/unit/test_package_initializers_facade_pattern.py - add Rule 6: public package initializers must not directly import from `._internal` - keep existing guardrails (docstring-only facade, no defs, metadata-only assignments, no star imports) - introduced explicit public export layers for supported defaults: - dare_framework/checkpoint/defaults.py - dare_framework/embedding/defaults.py - dare_framework/event/defaults.py - dare_framework/hook/defaults.py - dare_framework/plan/defaults.py - dare_framework/transport/adapters.py - updated public facades to import from those public layers instead of direct `_internal` paths: - dare_framework/checkpoint/__init__.py - dare_framework/embedding/__init__.py - dare_framework/event/__init__.py - dare_framework/hook/__init__.py - dare_framework/plan/__init__.py - dare_framework/security/__init__.py (switch DefaultSecurityBoundary import to security.impl) - dare_framework/transport/__init__.py - synced governance docs: - docs/todos/project_overall_todos.md: mark T0-4 done and close AG11 claim - docs/design/Framework_MinSurface_Review.md: add implementation note for this compliance batch Why: - direct `_internal` imports in public `__init__.py` files blur API boundaries and couple callers to implementation topology - introducing explicit public export layers preserves compatibility while tightening facade discipline - codifying the rule in tests prevents regressions from reintroducing direct internal coupling Verification: - .venv/bin/python -m pytest -q tests/unit/test_package_initializers_facade_pattern.py => 1 passed - .venv/bin/python -m pytest -q tests/unit/test_package_initializers_facade_pattern.py tests/unit/test_event_sqlite_event_log.py tests/unit/test_security_boundary.py tests/unit/test_embedding_openai_adapter.py tests/unit/test_default_planner.py tests/unit/test_default_remediator.py tests/unit/test_hook_extension_point_governance.py tests/unit/test_transport_channel.py tests/unit/test_execution_control.py => 52 passed - import smoke checks for moved default exports (event/embedding/plan/transport/security/hook) => ok - ./scripts/ci/check_governance_traceability.sh => passed - ./scripts/ci/check_governance_evidence_truth.sh => passed
![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: 0095b35d50
ℹ️ 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".
| # Rule 6: Public facades must not directly import internal modules. | ||
| if is_public_facade and isinstance(node, ast.ImportFrom): | ||
| module = node.module or "" | ||
| if "._internal" in module: |
There was a problem hiding this comment.
_internal imports in facade guard
The new facade rule misses relative imports like from ._internal.foo import Bar: in ast.ImportFrom, node.module is "_internal.foo" (without a leading dot), so checking only for "._internal" lets prohibited imports bypass the gate. This weakens the CI contract you just added, because public __init__.py files can still directly depend on internal modules by switching to relative syntax.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a709b7d.
The _internal guard now detects both absolute and relative ImportFrom forms, including from ._internal..., from .._internal..., and from . import _internal, in addition to Import statements. I also added regression cases in tests/unit/test_package_initializers_facade_pattern.py to lock this behavior.
The package initializer facade test only matched "._internal" substrings when checking ImportFrom nodes. Relative syntax such as sets node.module to "_internal.foo", which allowed prohibited imports to bypass the guard.\n\nThis update extracts direct internal import detection into helper functions and applies one consistent check for both ImportFrom and Import nodes. It also handles relative module-less forms like so the public facade contract remains enforceable regardless of import style.\n\nAdded a regression test that covers relative and absolute import forms and a non-internal control case to lock the behavior in CI.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a709b7da61
ℹ️ 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".
| existing = test_to_label.get(normalized) | ||
| if existing and existing != spec.label: | ||
| issues.append( |
There was a problem hiding this comment.
The duplicate check only compares selectors by exact string equality, so overlapping selectors across categories are silently accepted (for example, one category mapping tests/foo.py and another mapping tests/foo.py::test_bar). Because p0_gate.py executes each category separately, that same failing test can appear in multiple categories with different owners, which breaks the intended one-to-one failed test -> owner contract this script is meant to enforce.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2a2475.
validate_category_specs now rejects cross-category selector overlap (not just exact duplicates). It detects prefix overlaps like tests/foo.py vs tests/foo.py::test_bar and also normalizes parametrized nodes (test_x[param]) to prevent ownership ambiguity.
Regression coverage: tests/unit/test_check_test_failure_ownership.py::test_validate_category_specs_rejects_overlapping_test_mapping.
…through Add a new AnthropicModelAdapter backed by the official anthropic SDK and wire it into the default adapter manager and model exports. Key changes: - implement Anthropic Messages API serialization/parsing for system/user/assistant/tool blocks, tool calls, thinking content, and usage normalization - enforce direct model-name pass-through from Config.llm.model or ANTHROPIC_MODEL (no hard-coded alias mapping), so new Anthropic models do not require framework code changes - extend CLI doctor checks to support anthropic adapter API key and dependency diagnostics - add anthropic dependency to pyproject/requirements - add unit coverage for Anthropic adapter behavior and manager/doctor integration - add OpenSpec change artifacts and feature evidence doc for docs-first governance - update model/client design and usage docs, including a new client/examples/config.anthropic.example.json sample Also include existing workspace cleanup in openrouter_adapter.py (unused imports).
mindfn
changed the title
mindfn
changed the title
…el pass-through" This reverts commit ed550e8.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06330ef384
ℹ️ 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".
| from dare_framework.security.impl import ( | ||
| DefaultSecurityBoundary, | ||
| NoOpSecurityBoundary, | ||
| PolicySecurityBoundary, |
There was a problem hiding this comment.
Importing DefaultSecurityBoundary from dare_framework.security.impl changes the behavior exposed by the public facade: impl aliases DefaultSecurityBoundary to PolicySecurityBoundary, which is stricter than the previously exported _internal default and can return APPROVE_REQUIRED for non-idempotent tool calls. That means callers relying on from dare_framework.security import DefaultSecurityBoundary (including default agent wiring) now get different policy outcomes without opting into a new boundary, so this refactor silently changes runtime behavior rather than only import structure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2a2475.
DefaultSecurityBoundary is now routed to the legacy permissive implementation again (via dare_framework.security.impl.default_security_boundary) so from dare_framework.security import DefaultSecurityBoundary keeps historical ALLOW behavior instead of inheriting PolicySecurityBoundary approval gating.
Added regression coverage in tests/unit/test_security_boundary.py::test_default_security_boundary_remains_permissive_for_high_risk.
| existing = test_to_label.get(normalized) | ||
| if existing and existing != spec.label: | ||
| issues.append( |
There was a problem hiding this comment.
The ownership validator only rejects exact duplicate selector strings, so overlapping selectors such as tests/foo.py in one category and tests/foo.py::test_bar in another are accepted. Because p0_gate.py runs each category independently, the same failing test node can be attributed to multiple categories/owners, which breaks the intended one-to-one failure-to-owner mapping this gate is supposed to enforce.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e2a2475 (same root cause as thread #discussion_r2887166866).
Overlapping selectors across categories are now blocked by prefix-aware overlap detection in scripts/ci/check_test_failure_ownership.py, with regression coverage in tests/unit/test_check_test_failure_ownership.py::test_validate_category_specs_rejects_overlapping_test_mapping.
Resolve merge conflicts against origin/main while preserving #188 governance closeout work and newly landed mainline updates. Key changes: - keep both governance jobs in CI (failure-ownership-map and governance-intent-gate) when resolving .github/workflows/ci-gate.yml - align dare_framework/checkpoint/__init__.py and dare_framework/model/__init__.py with current mainline facades to avoid API regression - merge governance feature evidence sections without dropping intent-gate or closeout traceability details - restore DefaultSecurityBoundary compatibility semantics by routing the impl export to the legacy permissive boundary instead of aliasing to PolicySecurityBoundary - harden check_test_failure_ownership.py to reject overlapping pytest selectors across categories (file-level vs node-level overlap), including parametrized node normalization - add regression tests covering both review findings in tests/unit/test_security_boundary.py and tests/unit/test_check_test_failure_ownership.py Verification: - RED: ../../.venv/bin/python -m pytest -q tests/unit/test_security_boundary.py::test_default_security_boundary_remains_permissive_for_high_risk tests/unit/test_check_test_failure_ownership.py::test_validate_category_specs_rejects_overlapping_test_mapping (2 failed before fixes) - GREEN: same command passes after fixes - ../../.venv/bin/python -m pytest -q tests/unit/test_check_test_failure_ownership.py tests/unit/test_security_boundary.py tests/unit/test_package_initializers_facade_pattern.py tests/unit/test_default_model_adapter_manager.py tests/unit/test_prompt_store.py (20 passed) - ../../.venv/bin/python scripts/ci/check_test_failure_ownership.py (passed) - ../../.venv/bin/python scripts/ci/p0_gate.py (PASS) - ./scripts/ci/check_governance_traceability.sh (passed) - ./scripts/ci/check_governance_evidence_truth.sh (passed) - openspec validate p0-conformance-gate --type change --strict --json --no-interactive (valid) - openspec validate enhance-doc-governance-traceability --type change --strict --json --no-interactive (valid)
|
Addressed the newly raised review points in commit What was fixed:
Also resolved merge conflicts with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2a2475307
ℹ️ 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".
| @@ -0,0 +1,21 @@ | |||
| """Supported default checkpoint implementations and contributors.""" | |||
|
|
|||
| from dare_framework.checkpoint._internal.contributors.session_contributor import ( | |||
There was a problem hiding this comment.
dare_framework.checkpoint.defaults now imports dare_framework.checkpoint._internal..., but this package tree does not exist in the repo, so any use of this new public module (for example from dare_framework.checkpoint.defaults import MemoryCheckpointStore) fails immediately with ModuleNotFoundError. This makes the newly added defaults facade unusable at runtime rather than just enforcing a style refactor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in df55503.
dare_framework.checkpoint.defaults no longer imports deleted dare_framework.checkpoint._internal.* modules. I replaced it with in-module compatibility implementations for the exported defaults symbols (MemoryCheckpointStore, DefaultCheckpointSaveRestore, and contributors) so facade imports are valid again.
Regression coverage added in tests/unit/test_checkpoint_defaults.py::test_checkpoint_defaults_module_exports_are_importable to prevent reintroducing broken imports.
Fix the newly failing governance intent gate and the latest review comment on checkpoint defaults. Changes: - rewrite dare_framework/checkpoint/defaults.py to remove imports from deleted checkpoint._internal modules - provide in-module legacy-compatible checkpoint symbols so the defaults facade remains importable - add regression test tests/unit/test_checkpoint_defaults.py to lock importable default exports - add missing Intent PR link to docs/features/p0-conformance-gate.md so governance-intent-gate can validate merged intent metadata for active governed docs Verification: - ../../.venv/bin/python -m pytest -q tests/unit/test_checkpoint_defaults.py (passed) - GOVERNANCE_INTENT_GATE_DIFF_RANGE='origin/main...HEAD' GOVERNANCE_INTENT_GATE_PR_STATE_FIXTURE='#126=merged,#172=merged' ./scripts/ci/check_governance_intent_gate.sh (passed) - ./scripts/ci/check_governance_traceability.sh (passed) - ./scripts/ci/check_governance_evidence_truth.sh (passed) - openspec validate p0-conformance-gate --type change --strict --json --no-interactive (valid)
|
Follow-up fixes pushed in
Local verification passed:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df55503898
ℹ️ 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".
| serialized["config"] = asdict(serialized["config"]) | ||
| except Exception: | ||
| serialized["config"] = None |
There was a problem hiding this comment.
After serialized = asdict(session_context), the config field is already a plain dict for dataclass-backed contexts, so calling asdict(serialized["config"]) raises and the exception path overwrites it with None. This means any checkpoint using a non-null session_context.config silently loses that configuration data, which breaks the stated compatibility goal of this defaults facade and degrades audit/trace payload fidelity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2f7a8d.
SessionContextContributor.serialize() now preserves an already-serialized dict config. We only call asdict() for config when it is non-null and not already a dict, so dataclass-backed session contexts no longer lose config to None.
Regression coverage added in tests/unit/test_checkpoint_defaults.py::test_session_context_contributor_preserves_serialized_config_dict.
…ation Address the latest PR #188 review comment on SessionContextContributor config loss. Root cause: - SessionContextContributor.serialize() called asdict(session_context) and then attempted asdict(serialized['config']) again. - For dataclass-backed contexts, the first asdict() already converted config to dict, so the second call raised and forced config to None. Changes: - only run asdict() for config when config is non-null and not already a dict - keep existing dict config payload untouched - extend tests/unit/test_checkpoint_defaults.py with a regression test that verifies config survives serialization Verification: - ../../.venv/bin/python -m pytest -q tests/unit/test_checkpoint_defaults.py (2 passed) - GOVERNANCE_INTENT_GATE_DIFF_RANGE='origin/main...HEAD' GOVERNANCE_INTENT_GATE_PR_STATE_FIXTURE='#126=merged,#172=merged' ./scripts/ci/check_governance_intent_gate.sh (passed) - ./scripts/ci/check_governance_traceability.sh (passed) - ./scripts/ci/check_governance_evidence_truth.sh (passed)
|
Addressed the newest review comment in
Local checks passed:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f7a8d85e
ℹ️ 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".
| segments = [segment.strip() for segment in selector.split("::")] | ||
| normalized: list[str] = [] | ||
| for segment in segments: | ||
| if not segment: |
There was a problem hiding this comment.
pytest --help defines positional selectors as file_or_dir, so category specs can validly use directory paths (for example tests/unit) as well as node IDs. _normalize_selector_segments only tokenizes on ::, so _selectors_overlap treats tests/unit and tests/unit/test_a.py::test_x as different roots and misses that they select overlapping tests. In that case the validator allows two categories to own the same failing test, defeating the one-to-one failure→owner contract this gate is supposed to guarantee.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ff1dc7.
I updated selector overlap detection to normalize selectors as (path parts, node parts) instead of only splitting on ::. This now treats file_or_dir prefixes as overlap (for example tests/unit vs tests/unit/test_a.py::test_x) when the prefix selector has no node suffix.
Regression coverage added in tests/unit/test_check_test_failure_ownership.py::test_validate_category_specs_rejects_overlapping_directory_selector_mapping.
Address the latest review comment on selector-overlap coverage in scripts/ci/check_test_failure_ownership.py. Root cause: - overlap detection only tokenized selectors by - directory selectors like were treated as unrelated to descendant node selectors such as - this allowed cross-category ownership overlap to bypass validation Changes: - normalize selectors into path parts + pytest node parts - treat file_or_dir path prefix matches as overlapping when the prefix side has no node suffix - keep existing node-level overlap and parametrized node normalization behavior - add regression test for directory selector overlap in tests/unit/test_check_test_failure_ownership.py Verification: - ../../.venv/bin/python -m pytest -q tests/unit/test_check_test_failure_ownership.py (5 passed) - ../../.venv/bin/python scripts/ci/check_test_failure_ownership.py (passed) - ../../.venv/bin/python scripts/ci/p0_gate.py (PASS) - GOVERNANCE_INTENT_GATE_DIFF_RANGE='origin/main...HEAD' GOVERNANCE_INTENT_GATE_PR_STATE_FIXTURE='#126=merged,#172=merged' ./scripts/ci/check_governance_intent_gate.sh (passed) - ./scripts/ci/check_governance_traceability.sh (passed) - ./scripts/ci/check_governance_evidence_truth.sh (passed)
|
Addressed the latest selector-overlap review item in
Local verification passed:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ff1dc715c
ℹ️ 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".
| detail_change="$(awk -F'|' -v target="$ref" '$1 == target {print $2; exit}' "$detail_claim_map")" | ||
| if [[ -z "$detail_change" ]]; then |
There was a problem hiding this comment.
detail_change is looked up by printing only column 2 from detail_claim_map, so an existing referenced claim with an empty OpenSpec Change cell yields an empty string and is treated as “missing Detail Claim Ref mapping”. This creates a false-negative gate failure for repos that allow blank change values (the same script already treats empty/pending changes as optional in check_todo_claim_to_openspec_task_mapping), so valid Detail Claim Ref links can be rejected even when the claim exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 17d0869.
I split Detail Claim Ref existence detection from the referenced OpenSpec Change value in check_master_todo_slice_mapping_consistency:
- first confirm the detail claim row exists by claim id
- then read
OpenSpec Changeindependently
This prevents false negatives when a referenced detail claim exists but its change cell is intentionally empty.
Regression coverage added:
tests/unit/test_governance_traceability_gate.py::test_gate_accepts_detail_claim_ref_with_empty_change_cell
Address two governance gate regressions surfaced on PR #188. Traceability gate: - treat Detail Claim Ref existence separately from the referenced OpenSpec Change value - avoid false negatives when the detail claim exists but its change cell is intentionally empty - add regression coverage in test_governance_traceability_gate.py Intent gate: - remove empty-array expansions that can trip set -u as unbound in some shell behaviors - switch changed/governed feature doc collections to newline-delimited strings for portable iteration - add regression assertion that the gate no longer emits unbound-variable errors Validation: - pytest -q tests/unit/test_governance_intent_gate.py - pytest -q tests/unit/test_governance_traceability_gate.py - scripts/ci/check_governance_intent_gate.sh (now deterministic failure without shell crash in no-doc scenario) - scripts/ci/check_governance_traceability.sh
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17d086998c
ℹ️ 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".
| if [[ -n "$status" && ! "$status" =~ ^(draft|active|done|archived)$ ]]; then | ||
| log "invalid frontmatter status '$status' in $file" |
There was a problem hiding this comment.
The new status whitelist rejects in_review, which now makes check_governance_traceability.sh fail for a status that other governance gates explicitly support as a governed state (see check_governance_evidence_truth.sh lines 430-434 and check_governance_intent_gate.sh lines 264-274). This creates contradictory CI behavior: moving a feature doc into the documented review phase (status: in_review) will be blocked by traceability even though the intent/evidence workflows are designed around that phase.
Useful? React with 👍 / 👎.
| log "inconsistent OpenSpec change between $project_file and $detail_file for ref $ref" | ||
| failures=$((failures + 1)) | ||
| fi | ||
| done < <(grep -oE 'CLM-[A-Za-z0-9-]+' <<<"$detail_refs") |
There was a problem hiding this comment.
This loop only validates refs that match CLM-...; when Detail Claim Ref is non-empty but malformed (for example `BADREF`), grep -oE yields no tokens and the check silently skips the row, so traceability passes with an invalid master/detail mapping. That weakens the new consistency gate because bad references are not surfaced unless they happen to match the expected prefix.
Useful? React with 👍 / 👎.
|
Superseded by a split follow-up:
The governance / failure-ownership / CI gate work from this PR is being deferred for now, so closing this mixed-scope PR in favor of the narrower replacements. |
1 participant
Summary
enhance-doc-governance-traceabilityremaining depth checks and evidence sync (3.2/3.3/3.4/5.3)p0-gaterequired-check rollout checklist and align OpenSpec/doc milestones for admin follow-up (3.2pending external permission)T0-5with machine-checkablefailed test -> module -> ownermapping (p0_gateowner field +check_test_failure_ownership.py+ CI job)T0-4facade compliance by forbidding direct._internalimports in public__init__.pyand moving exports to publicdefaults/adapters/impllayersTest Plan
.venv/bin/pytest -q.venv/bin/python scripts/ci/p0_gate.pypython scripts/ci/check_test_failure_ownership.py./scripts/ci/check_governance_traceability.sh./scripts/ci/check_governance_evidence_truth.shopenspec validate p0-conformance-gate --type change --strict --json --no-interactiveNotes
p0-conformance-gatetask3.2(setp0-gateas required check) still requires repository admin branch-protection/ruleset permissions and is documented with a concrete rollout checklist.valid.