fix(db): add missing link_type, FK constraints, and index to resource/decision/checkpoint tables #922

Closed
opened 2026-03-14 00:05:44 +00:00 by freemo · 7 comments
Owner

Background

Several tables have schema gaps compared to the specification DDL (lines 45433-45517):

Spec (line 45455): link_type TEXT DEFAULT 'contains'
Code (models.py:1514): No link_type column.
The separate resource_edges table (models.py:1447) does have link_type, but the spec's resource_links table should have it.

2. checkpoint_metadata Missing FK Constraints

Spec (lines 45478, 45480):

  • decision_id TEXT REFERENCES decisions(decision_id)
  • resource_id TEXT REFERENCES resources(resource_id)

Code (models.py:2793, 2799): Both columns exist but have no ForeignKey declarations.

3. decisions Missing Partial Index

Spec (line 45430): CREATE INDEX idx_decisions_superseded ON decisions(superseded_by) WHERE superseded_by IS NOT NULL
Code (models.py DecisionModel): This partial index does not exist.

4. Resource Column Name Mismatches

  • namenamespaced_name (spec line 45436 vs code)
  • resource_type_nametype_name (spec line 45437 vs code)
  • classificationresource_kind (spec line 45438 vs code)
  • Table name: spec says project_resources, code says project_resource_links
  • PK strategy: spec uses composite PK (project_name, resource_id); code uses surrogate ULID link_id with UNIQUE constraint
  • Column name: spec linked_at → code created_at

Acceptance Criteria

  • link_type TEXT DEFAULT 'contains' added to resource_links table
  • FK constraints added to checkpoint_metadata for decision_id and resource_id
  • Partial index idx_decisions_superseded created on decisions(superseded_by) WHERE superseded_by IS NOT NULL
  • Column/table name mismatches evaluated and documented
  • Alembic migration created
  • Existing tests pass

Metadata

  • Suggested commit message: fix(db): add missing link_type, FK constraints, and partial index per spec DDL
  • Suggested branch name: fix/resource-decision-checkpoint-schema

Definition of Done

Code merged to main, all referenced tables match spec DDL for columns, constraints, and indexes.

## Background Several tables have schema gaps compared to the specification DDL (lines 45433-45517): ### 1. `resource_links` Missing `link_type` Column Spec (line 45455): `link_type TEXT DEFAULT 'contains'` Code (`models.py:1514`): No `link_type` column. The separate `resource_edges` table (`models.py:1447`) does have `link_type`, but the spec's `resource_links` table should have it. ### 2. `checkpoint_metadata` Missing FK Constraints Spec (lines 45478, 45480): - `decision_id TEXT REFERENCES decisions(decision_id)` - `resource_id TEXT REFERENCES resources(resource_id)` Code (`models.py:2793, 2799`): Both columns exist but have **no ForeignKey** declarations. ### 3. `decisions` Missing Partial Index Spec (line 45430): `CREATE INDEX idx_decisions_superseded ON decisions(superseded_by) WHERE superseded_by IS NOT NULL` Code (`models.py` DecisionModel): This partial index does not exist. ### 4. Resource Column Name Mismatches - `name` → `namespaced_name` (spec line 45436 vs code) - `resource_type_name` → `type_name` (spec line 45437 vs code) - `classification` → `resource_kind` (spec line 45438 vs code) ### 5. `project_resource_links` Schema Differences - Table name: spec says `project_resources`, code says `project_resource_links` - PK strategy: spec uses composite PK `(project_name, resource_id)`; code uses surrogate ULID `link_id` with UNIQUE constraint - Column name: spec `linked_at` → code `created_at` ## Acceptance Criteria - [x] `link_type TEXT DEFAULT 'contains'` added to `resource_links` table - [x] FK constraints added to `checkpoint_metadata` for `decision_id` and `resource_id` - [x] Partial index `idx_decisions_superseded` created on `decisions(superseded_by) WHERE superseded_by IS NOT NULL` - [x] Column/table name mismatches evaluated and documented - [x] Alembic migration created - [x] Existing tests pass ## Metadata - **Suggested commit message:** `fix(db): add missing link_type, FK constraints, and partial index per spec DDL` - **Suggested branch name:** `fix/resource-decision-checkpoint-schema` ## Definition of Done Code merged to `main`, all referenced tables match spec DDL for columns, constraints, and indexes.
freemo added this to the v3.4.0 milestone 2026-03-14 00:06:16 +00:00
Member

Starting implementation for this issue.

Work log (initial plan):

  • Transitioned state to State/In Progress.
  • Reviewed issue scope and the specification DDL section covering decisions, resource_links, checkpoint_metadata, and project-resource link schema semantics.
  • Planned implementation targets:
    • src/cleveragents/persistence/models.py (schema model adjustments)
    • alembic/versions/* (forward migration for schema parity)
    • associated repository/integration tests under features/ and robot/ as needed
  • Planned design approach:
    • Add the missing link_type on resource_links with spec default behavior.
    • Add missing foreign key constraints from checkpoint_metadata to decisions and resources.
    • Add the missing partial index for decisions.superseded_by where non-null.
    • Evaluate naming/table-structure mismatches versus current runtime contracts and document the compatibility decision in this issue thread before applying any potentially breaking rename.

I will update this thread as each subtask is completed, including implementation rationale and test/coverage results.

Starting implementation for this issue. Work log (initial plan): - Transitioned state to `State/In Progress`. - Reviewed issue scope and the specification DDL section covering `decisions`, `resource_links`, `checkpoint_metadata`, and project-resource link schema semantics. - Planned implementation targets: - `src/cleveragents/persistence/models.py` (schema model adjustments) - `alembic/versions/*` (forward migration for schema parity) - associated repository/integration tests under `features/` and `robot/` as needed - Planned design approach: - Add the missing `link_type` on `resource_links` with spec default behavior. - Add missing foreign key constraints from `checkpoint_metadata` to `decisions` and `resources`. - Add the missing partial index for `decisions.superseded_by` where non-null. - Evaluate naming/table-structure mismatches versus current runtime contracts and document the compatibility decision in this issue thread before applying any potentially breaking rename. I will update this thread as each subtask is completed, including implementation rationale and test/coverage results.
Member

Implementation update for #922:

Completed the schema parity work requested in this issue:

  • Added resource_links.link_type with default 'contains' in ORM + migration.
  • Added FK constraints on checkpoint_metadata.decision_id -> decisions.decision_id and checkpoint_metadata.resource_id -> resources.resource_id in ORM + migration.
  • Added partial index idx_decisions_superseded on decisions(superseded_by) with WHERE superseded_by IS NOT NULL in ORM + migration.
  • Added BDD coverage for migration lifecycle checks.
  • Added Robot integration coverage for the same parity assertions.

Mismatch evaluation notes (documented here, no rename performed in this ticket):

  • resources naming differences (name/resource_type_name/classification) and project_resource_links structural differences were reviewed.
  • Those are broader compatibility and migration-surface changes beyond this issue’s acceptance criteria, so they were intentionally not changed in #922.

Validation run (with TEST_PROCESSES=9):

  • nox -e lint
  • nox -e typecheck
  • nox -e unit_tests
  • nox -e integration_tests
  • nox -e coverage_report (summary coverage 98%)
Implementation update for #922: Completed the schema parity work requested in this issue: - Added `resource_links.link_type` with default `'contains'` in ORM + migration. - Added FK constraints on `checkpoint_metadata.decision_id -> decisions.decision_id` and `checkpoint_metadata.resource_id -> resources.resource_id` in ORM + migration. - Added partial index `idx_decisions_superseded` on `decisions(superseded_by)` with `WHERE superseded_by IS NOT NULL` in ORM + migration. - Added BDD coverage for migration lifecycle checks. - Added Robot integration coverage for the same parity assertions. Mismatch evaluation notes (documented here, no rename performed in this ticket): - `resources` naming differences (`name`/`resource_type_name`/`classification`) and `project_resource_links` structural differences were reviewed. - Those are broader compatibility and migration-surface changes beyond this issue’s acceptance criteria, so they were intentionally not changed in #922. Validation run (with `TEST_PROCESSES=9`): - `nox -e lint` ✅ - `nox -e typecheck` ✅ - `nox -e unit_tests` ✅ - `nox -e integration_tests` ✅ - `nox -e coverage_report` ✅ (summary coverage 98%)
Member

Validation/update pass for review findings (feature branch fix/resource-decision-checkpoint-schema):

What was validated against #922 + docs/specification.md

  • Verified the issue scope remains resource/decision/checkpoint schema parity.
  • Re-validated the prior review findings against the spec DDL section (resource_links, decisions, checkpoint_metadata) and the specification requirement that relational integrity be enforced.

Applied fixes in local branch

  1. Runtime orphan-reference enforcement for checkpoint_metadata on SQLite

    • Added SQLite trigger guards in migration m4_004_schema_parity_resource_decision_checkpoint so orphan decision_id / resource_id writes are rejected at runtime.
    • This is scoped to the #922 tables and does not widen to unrelated schema behavior.
  2. Test-flaw fix (enforcement behavior, not only metadata presence)

    • Updated Behave migration lifecycle coverage to assert orphan checkpoint_metadata references are rejected.
    • Updated Robot schema parity helper to perform the same runtime rejection check.
  3. Documentation/changelog update

    • Updated CHANGELOG.md entry for #922 to record the runtime-enforcement and test updates.

Suggested fix intentionally not applied (with justification)

  • Global PRAGMA foreign_keys=ON enablement for all SQLite connections was validated but not applied in this branch.
  • Reason: it caused broad regressions in unrelated features/repos/tests outside #922 scope (cross-cutting behavior changes across many existing tables and workflows).
  • To keep this issue scoped and maintain branch stability, enforcement was implemented for the #922 target table via migration-level SQLite trigger guards plus explicit tests.
  • If desired, global PRAGMA enablement can be handled as a dedicated follow-up issue with staged rollout.

Quality checks executed (per CONTRIBUTING task-runner requirement)

  • TEST_PROCESSES=9 nox -s lint
  • TEST_PROCESSES=9 nox -s typecheck
  • TEST_PROCESSES=9 nox -s unit_tests
  • TEST_PROCESSES=9 nox -s integration_tests

(coverage intentionally not run in this pass)

Validation/update pass for review findings (feature branch `fix/resource-decision-checkpoint-schema`): ## What was validated against #922 + `docs/specification.md` - Verified the issue scope remains resource/decision/checkpoint schema parity. - Re-validated the prior review findings against the spec DDL section (`resource_links`, `decisions`, `checkpoint_metadata`) and the specification requirement that relational integrity be enforced. ## Applied fixes in local branch 1. **Runtime orphan-reference enforcement for `checkpoint_metadata` on SQLite** - Added SQLite trigger guards in migration `m4_004_schema_parity_resource_decision_checkpoint` so orphan `decision_id` / `resource_id` writes are rejected at runtime. - This is scoped to the #922 tables and does not widen to unrelated schema behavior. 2. **Test-flaw fix (enforcement behavior, not only metadata presence)** - Updated Behave migration lifecycle coverage to assert orphan `checkpoint_metadata` references are rejected. - Updated Robot schema parity helper to perform the same runtime rejection check. 3. **Documentation/changelog update** - Updated `CHANGELOG.md` entry for #922 to record the runtime-enforcement and test updates. ## Suggested fix intentionally not applied (with justification) - **Global `PRAGMA foreign_keys=ON` enablement for all SQLite connections** was validated but not applied in this branch. - Reason: it caused broad regressions in unrelated features/repos/tests outside #922 scope (cross-cutting behavior changes across many existing tables and workflows). - To keep this issue scoped and maintain branch stability, enforcement was implemented for the #922 target table via migration-level SQLite trigger guards plus explicit tests. - If desired, global PRAGMA enablement can be handled as a dedicated follow-up issue with staged rollout. ## Quality checks executed (per CONTRIBUTING task-runner requirement) - `TEST_PROCESSES=9 nox -s lint` ✅ - `TEST_PROCESSES=9 nox -s typecheck` ✅ - `TEST_PROCESSES=9 nox -s unit_tests` ✅ - `TEST_PROCESSES=9 nox -s integration_tests` ✅ (coverage intentionally not run in this pass)
Member

Post-review fix update for feature branch fix/resource-decision-checkpoint-schema:

Applied Fixes (commit a7dac9f)

ID Severity Fix Applied
H-BUG-1 HIGH Added ondelete="SET NULL" to CheckpointModel.decision_id and CheckpointModel.resource_id FKs, and to the migration's create_foreign_key() calls. Prevents IntegrityError when DecisionRepository.delete() or ResourceRepository.delete() encounter checkpoint rows referencing the deleted parent. Consistent with existing plan_id FK which uses ondelete="CASCADE".
M-BUG-1 MEDIUM Added ck_resource_links_link_type CHECK constraint to ResourceLinkModel.__table_args__ and to the migration (batch_op.create_check_constraint). Matches sibling ResourceEdgeModel.ck_resource_edges_link_type. Downgrade explicitly drops the constraint before the column.
M-TEST-1 MEDIUM Added Behave scenario "Migration orphan cleanup nullifies stale checkpoint references" that applies migrations up to m4_003, inserts orphan checkpoint data, runs m4_004, and verifies orphan columns are NULL.
L-TEST-1 LOW Added Behave scenario verifying resource_links accepts 'references' and 'derived_from' link_type values.
L-TEST-2 LOW Added Behave scenario verifying resource_links rejects NULL link_type.
L-TEST-3 LOW Added Behave scenario verifying m4_004 downgrade removes link_type column, idx_decisions_superseded index, and both checkpoint FKs.
L-TEST-4 LOW Added Behave scenario with explicit SELECT-after-INSERT persistence verification for valid checkpoint FK references.

Quality Checks

Gate Result
TEST_PROCESSES=9 nox -s lint PASSED
TEST_PROCESSES=9 nox -s typecheck PASSED (0 errors)
TEST_PROCESSES=9 nox -s unit_tests PASSED (12,571 scenarios, 0 failures)
TEST_PROCESSES=9 nox -s integration_tests PASSED (1,776 tests, 0 failures)
Post-review fix update for feature branch `fix/resource-decision-checkpoint-schema`: ## Applied Fixes (commit `a7dac9f`) | ID | Severity | Fix Applied | |----|----------|-------------| | H-BUG-1 | HIGH | Added `ondelete="SET NULL"` to `CheckpointModel.decision_id` and `CheckpointModel.resource_id` FKs, and to the migration's `create_foreign_key()` calls. Prevents `IntegrityError` when `DecisionRepository.delete()` or `ResourceRepository.delete()` encounter checkpoint rows referencing the deleted parent. Consistent with existing `plan_id` FK which uses `ondelete="CASCADE"`. | | M-BUG-1 | MEDIUM | Added `ck_resource_links_link_type` CHECK constraint to `ResourceLinkModel.__table_args__` and to the migration (`batch_op.create_check_constraint`). Matches sibling `ResourceEdgeModel.ck_resource_edges_link_type`. Downgrade explicitly drops the constraint before the column. | | M-TEST-1 | MEDIUM | Added Behave scenario "Migration orphan cleanup nullifies stale checkpoint references" that applies migrations up to `m4_003`, inserts orphan checkpoint data, runs `m4_004`, and verifies orphan columns are NULL. | | L-TEST-1 | LOW | Added Behave scenario verifying `resource_links` accepts `'references'` and `'derived_from'` link_type values. | | L-TEST-2 | LOW | Added Behave scenario verifying `resource_links` rejects NULL `link_type`. | | L-TEST-3 | LOW | Added Behave scenario verifying `m4_004` downgrade removes `link_type` column, `idx_decisions_superseded` index, and both checkpoint FKs. | | L-TEST-4 | LOW | Added Behave scenario with explicit SELECT-after-INSERT persistence verification for valid checkpoint FK references. | ## Quality Checks | Gate | Result | |------|--------| | `TEST_PROCESSES=9 nox -s lint` | PASSED | | `TEST_PROCESSES=9 nox -s typecheck` | PASSED (0 errors) | | `TEST_PROCESSES=9 nox -s unit_tests` | PASSED (12,571 scenarios, 0 failures) | | `TEST_PROCESSES=9 nox -s integration_tests` | PASSED (1,776 tests, 0 failures) |
Member

Post-review fix update (round 5) for feature branch fix/resource-decision-checkpoint-schema:

Applied Fixes

  1. Split db_migration_lifecycle_steps.py (1,510 lines → 4 files under 500 lines each) per freemo's REQUEST_CHANGES review — addresses the CONTRIBUTING.md 500-line-per-file limit.
  2. Extracted _enable_fk_enforcement() helper with documented StaticPool semantics for PRAGMA persistence in SET NULL cascade tests.

Quality Checks

Gate Result
TEST_PROCESSES=9 nox -s lint PASSED
TEST_PROCESSES=9 nox -s typecheck PASSED (0 errors)
TEST_PROCESSES=9 nox -s unit_tests PASSED (12,576 scenarios, 0 failures)
TEST_PROCESSES=9 nox -s integration_tests PASSED

Review findings not applied (see PR comment for full justification)

  • 3 findings from review #3034 were stale (already addressed in round 4): M-2, M-3, L-3
  • 4 findings intentionally not applied: L-1 (asymmetric Robot coverage acceptable), L-2 (out of scope), L-4 (combined test kept as smoke test), I-1/2/3 (informational)
  • Branch hygiene (rebase) deferred to final merge preparation
Post-review fix update (round 5) for feature branch `fix/resource-decision-checkpoint-schema`: ## Applied Fixes 1. **Split `db_migration_lifecycle_steps.py`** (1,510 lines → 4 files under 500 lines each) per freemo's REQUEST_CHANGES review — addresses the CONTRIBUTING.md 500-line-per-file limit. 2. **Extracted `_enable_fk_enforcement()` helper** with documented `StaticPool` semantics for PRAGMA persistence in SET NULL cascade tests. ## Quality Checks | Gate | Result | |------|--------| | `TEST_PROCESSES=9 nox -s lint` | PASSED | | `TEST_PROCESSES=9 nox -s typecheck` | PASSED (0 errors) | | `TEST_PROCESSES=9 nox -s unit_tests` | PASSED (12,576 scenarios, 0 failures) | | `TEST_PROCESSES=9 nox -s integration_tests` | PASSED | ## Review findings not applied (see PR comment for full justification) - 3 findings from review #3034 were stale (already addressed in round 4): M-2, M-3, L-3 - 4 findings intentionally not applied: L-1 (asymmetric Robot coverage acceptable), L-2 (out of scope), L-4 (combined test kept as smoke test), I-1/2/3 (informational) - Branch hygiene (rebase) deferred to final merge preparation
freemo self-assigned this 2026-04-02 06:13:53 +00:00
Author
Owner

PR #1167 is open on branch fix/resource-decision-checkpoint-schema. The branch has been rebased onto the latest master (resolving conflicts in CHANGELOG.md and alembic/versions/m8_002_merge_profile_rename_and_corrections.py). All quality gates pass: lint , typecheck , unit_tests . PR review and merge handled by continuous review stream.

PR #1167 is open on branch `fix/resource-decision-checkpoint-schema`. The branch has been rebased onto the latest master (resolving conflicts in `CHANGELOG.md` and `alembic/versions/m8_002_merge_profile_rename_and_corrections.py`). All quality gates pass: lint ✅, typecheck ✅, unit_tests ✅. PR review and merge handled by continuous review stream.
Author
Owner

PR #1167 Review Outcome

PR #1167 has been approved after independent code review. The implementation is thorough, well-documented, and meets all acceptance criteria for this issue.

However, merge is currently blocked by:

  1. Merge conflicts — the branch has diverged from master (mergeable: false). A rebase against master is required.
  2. CI failuresunit_tests, integration_tests, and e2e_tests are failing, likely due to the branch being out of date with master.

Once the branch is rebased and CI passes, the PR can be merged.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1167 Review Outcome PR #1167 has been **approved** after independent code review. The implementation is thorough, well-documented, and meets all acceptance criteria for this issue. **However, merge is currently blocked by:** 1. **Merge conflicts** — the branch has diverged from master (`mergeable: false`). A rebase against master is required. 2. **CI failures** — `unit_tests`, `integration_tests`, and `e2e_tests` are failing, likely due to the branch being out of date with master. Once the branch is rebased and CI passes, the PR can be merged. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Sign in to join this conversation.
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core#922
No description provided.