[gurnben]

What

The remediation scheduling contracts change, reworked per the review to the minimal shape: no new tables — remediation.schema.json is the system of record for the out-of-band world (fork url, branch, checks, status), and the candidate decisions on the work item already link findings to each effort. What the model genuinely lacked, in one commit:

• run_order.work_item_id (Gurney's work, kept in substance): the remediation assessment an order runs, with its partial index, the repository-pairing guards (an order runs a work item of its own repository; a ref holding an ordered work item keeps its repository; FOR SHARE on Postgres serializes concurrent moves).
• run_order.fork_url / .fork_branch: the mirror an effort runs on and its fix branch — operational facts, write-once at scheduling, read-once at claim. Deliberately not a repository FK: mirrors are out of band and never inventory, and the durable record lives in the run's remediation artifact.
• idx_decision_assessment on decision (assessment_id) WHERE assessment_id IS NOT NULL (both dialects): a work item's decisions had no index, so the remediation fold and the in-flight queries scanned the fastest-growing table.

Status tracking stays where G11 put it: judgment checkpoints are decision rows (candidate → fixed → landed), run outcome is assessment.status, in-flight is run_order.state. The remediation/remediation_finding tables from the original branch are dropped, as is the upgrade file — existing databases alter by hand (below) until there is an official setup for tracking upgrades.

Measured

100k decisions, 5k work items, 20k findings on Postgres (make db-up):

Query	With idx_decision_assessment	Without
one work item's 20 decisions	0.018 ms	3.858 ms (seq scan)
in-flight dashboard (500 open orders → findings through work_item_id, 10k rows)	5.3 ms, decision leg an index scan	—

Upgrading an existing database (by hand)

Databases are recreated, not migrated; an operator whose run_order predates these columns applies the additive statements themselves — existing orders name no work item and carry no fork; none is inferred:

-- Postgres
ALTER TABLE traust_ops.run_order
    ADD COLUMN work_item_id UUID REFERENCES traust.assessment(id) ON DELETE RESTRICT;
ALTER TABLE traust_ops.run_order ADD COLUMN fork_url TEXT;
ALTER TABLE traust_ops.run_order ADD COLUMN fork_branch TEXT;
CREATE INDEX IF NOT EXISTS idx_ops_run_order_work_item
    ON traust_ops.run_order (work_item_id) WHERE work_item_id IS NOT NULL;

-- SQLite
ALTER TABLE ops_run_order
    ADD COLUMN work_item_id TEXT REFERENCES assessment(id) ON DELETE RESTRICT;
ALTER TABLE ops_run_order ADD COLUMN fork_url TEXT;
ALTER TABLE ops_run_order ADD COLUMN fork_branch TEXT;
CREATE INDEX IF NOT EXISTS idx_ops_run_order_work_item
    ON ops_run_order (work_item_id) WHERE work_item_id IS NOT NULL;

The pairing-guard triggers come with a fresh bootstrap; a hand-upgraded database gets them on its next recreate, or the operator creates them from run_order.sql.

Down / rollback

No down-migration machinery; for an operator who must roll back by hand — test_the_documented_rollback_leaves_findings_refs_and_orders_working runs exactly these statements:

-- Postgres
DROP TRIGGER IF EXISTS ref_of_ordered_work_item_stays ON traust.repository_ref;
DROP FUNCTION IF EXISTS traust_ops.ref_of_ordered_work_item_stays();
DROP TRIGGER IF EXISTS run_order_work_item_of_repository ON traust_ops.run_order;
DROP FUNCTION IF EXISTS traust_ops.run_order_work_item_of_repository();
DROP INDEX IF EXISTS traust_ops.idx_ops_run_order_work_item;
ALTER TABLE traust_ops.run_order DROP COLUMN fork_branch;
ALTER TABLE traust_ops.run_order DROP COLUMN fork_url;
ALTER TABLE traust_ops.run_order DROP COLUMN work_item_id;
DROP INDEX IF EXISTS traust.idx_decision_assessment;

-- SQLite
DROP TRIGGER IF EXISTS ops_ref_of_ordered_work_item_stays;
DROP TRIGGER IF EXISTS ops_run_order_work_item_of_repository;
DROP TRIGGER IF EXISTS ops_run_order_work_item_of_repository_update;
DROP INDEX IF EXISTS idx_ops_run_order_work_item;
ALTER TABLE ops_run_order DROP COLUMN fork_branch;
ALTER TABLE ops_run_order DROP COLUMN fork_url;
ALTER TABLE ops_run_order DROP COLUMN work_item_id;
DROP INDEX IF EXISTS idx_decision_assessment;

Tests

test_db_behavior.py: an order names a real work item of its own repository or none (and carries its fork facts); concurrent ref moves and orders never both commit (Postgres); the documented rollback leaves findings, refs and orders working. Both dialects — Postgres runs in CI (make db-up).

After merge

traust-core re-pins to the merge commit and reworks the merged scheduling service (RemediationSchedulingService) onto this shape: the order carries the fork facts, reads fold from decisions.

💘 Generated with Crush

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Concurrent ref moves misroute orders ✓ Resolved

Resolving commit: 40a9e66

Resolution reason

The order-side validation trigger run_order_work_item_of_repository now takes a FOR SHARE lock on
the repository_ref row (SELECT ... FOR SHARE OF r) before comparing repository ids, so a concurrent
ref move waits for the order and vice versa. Both bootstrap (run_order.sql) and upgrade
(run_order_work_item.sql) definitions are aligned, and a two-connection PostgreSQL regression test
(test_a_concurrent_ref_move_and_order_never_both_commit) interleaves the operations to verify that
they cannot both commit. The fix uses FOR SHARE on the ref rather than a lock on the ref's trigger
path, which is a different but valid serialization than the suggested fix prompt wording, though it
follows its recommended approach closely in spirit.

Description

ref_of_ordered_work_item_stays queries committed orders without synchronizing with an order being
assigned a work item. If that assignment validates the ref's old repository while another
transaction moves the ref before the order commits, both transactions can commit an order whose
repository differs from its work item's.

---

2. Concurrent moves misattribute fixes ✓ Resolved

Resolving commit: 40a9e66

Resolution reason

The insert trigger now takes FOR SHARE on the traust.finding row and reads its repository_id on a
locked row, so a concurrent move waits for the link commit (and vice versa). The
finding_under_remediation_stays trigger runs after the lock on a fresh snapshot, so the two
orderings cannot both commit a mismatch. The commit also adds a two-connection PostgreSQL regression
(test_a_concurrent_move_and_link_never_both_commit) that interleaves the two statements and checks
for zero mismatched rows. It uses row-level locking rather than the suggested composite FK / unique
parent key, so it is a different implementation.

Description

finding_under_remediation_stays checks visible links without synchronizing with a concurrent
insertion of remediation_finding. If an insertion checks the finding's old repository while
another transaction moves that finding before the link commits, both checks can pass and the
committed link names the wrong repository.

---

3. Repository views can misattribute fixes ✓ Resolved

Resolving commit: 1145c0c

Resolution reason

Both dialects enforce that a remediation_finding row's repository_id matches its finding's
repository_id, via BEFORE INSERT/UPDATE triggers (Postgres function plus trigger; SQLite insert and
update triggers). A regression test checks that a mismatched repository is rejected and that
updating repository_id is rejected.

Description

remediation_finding independently references a finding and a repository without requiring that the
repository own the finding. When both referenced rows exist but belong to different repositories, an
insert succeeds and the per-repository remediation view can attribute that finding to the wrong
repository.

---

4. Upgraded databases scan decisions 🐞 Bug ➹ Performance

Description

run_order_work_item_upgrade loads only the run-order upgrade, while idx_decision_assessment is
defined only in the storage bootstrap and neither dialect's upgrade creates it. When an existing
database applies the exported additive upgrade without replaying the storage bootstrap, work-item
decision lookups remain without the index intended for the remediation fold and in-flight queries.

Code

db/v1/traust/postgres/schema/decision.sql[R62-64]

+CREATE INDEX IF NOT EXISTS idx_decision_assessment
+    ON traust.decision (assessment_id)
+    WHERE assessment_id IS NOT NULL;

Evidence

The index is created by the two decision schema files, but the exported upgrade loader reads only
the run-order upgrade files. The upgrade test applies that loader and replays only the ops run-order
bootstrap, so it does not establish that an existing database receives the decision index.

db/v1/traust/postgres/schema/decision.sql[61-64]
db/v1/traust/sqlite/schema/decision.sql[61-64]
src/traust_contracts/v1/ops/sql.py[57-64]
db/v1/traust_ops/postgres/upgrades/run_order_work_item.sql[1-14]
db/v1/traust_ops/sqlite/upgrades/run_order_work_item.sql[1-14]
tests/test_db_behavior.py[538-553]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Existing databases that apply the exported run-order upgrade without replaying the storage bootstrap do not receive the new decision assessment index.
## Fix Focus Areas
- db/v1/traust/postgres/schema/decision.sql[61-64]
- db/v1/traust/sqlite/schema/decision.sql[61-64]
- db/v1/traust_ops/postgres/upgrades/run_order_work_item.sql[1-14]
- db/v1/traust_ops/sqlite/upgrades/run_order_work_item.sql[1-14]
- tests/test_db_behavior.py[519-553]
## Recommended Fix
Provide an explicit, idempotent existing-database path that creates the decision index in both dialects, and test that an upgraded database receives it without replaying the storage bootstrap.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗

---

5. Manual rollback breaks finding updates ✓ Resolved

Resolving commit: 40a9e66

Resolution reason

Added test_the_documented_rollback_leaves_findings_refs_and_orders_working, which runs the PR's
documented rollback statements verbatim (including DROP TRIGGER IF EXISTS
finding_under_remediation_stays ON traust.finding for postgres and DROP TRIGGER IF EXISTS
finding_under_remediation_stays for sqlite, both before DROP TABLE remediation_finding), then
updates an unlinked finding's repository_id and asserts it works. This is the rollback coverage the
suggested fix asked for, and it verifies the trigger is dropped in each dialect. The diff does not
modify the sqlite schema or rollback docs themselves, but the rollback statements already drop the
trigger and the new test exercises that path.

Description

The new finding_under_remediation_stays trigger remains attached to finding when
remediation_finding is dropped. An operator following the PR's documented manual rollback leaves
that trigger in place, so a later finding repository update queries a missing table and fails in
either dialect.

---

6. Orders can name another repository's work ✓ Resolved

Resolving commit: 1145c0c

Resolution reason

Added repository-ownership guard triggers on run_order for insert and update of
work_item_id/repository_id in Postgres (fresh and upgrade) and SQLite (fresh and upgrade), rejecting
work items whose assessment's repository_ref belongs to a different repository, plus regression
tests for rejection in both dialects.

Description

run_order.work_item_id validates that an assessment exists but does not require its repository to
match the order’s repository_id. An order for one repository can therefore name an assessment
belonging to another; the same mismatch is accepted by the additive upgrade in both dialects.

---

View medium (1)

7. Empty-effort deletion goes untested ✓ Resolved

Resolving commit: 1145c0c

Resolution reason

At PR head, test_scheduled_findings_cannot_be_deleted_out_from_under_the_effort no longer uses a
finding ID for the empty-effort deletion. It creates empty = remediation(db) (line 489), deletes
it by its own ID (empty,) (line 490), and asserts it is absent (line 491). The no-op problem is
gone. The fix differs from the suggested prompt only in that the linked efforts are kept and the
unlinked one is created in-line, which is valid.

Description

test_scheduled_findings_cannot_be_deleted_out_from_under_the_effort passes other, a finding ID,
to a deletion from remediation. With no remediation matching that ID, the statement succeeds
without deleting a row, so the claimed empty-effort deletion rule is not exercised in either
dialect.

---

Context sources

✅ Web pages:

  🌐 PostgreSQL: Documentation: 18: 13.2. Transaction Isolation

  🌐 PostgreSQL: Re: Not understanding this behavior of a subselect + volatile function

  🌐 PostgreSQL: Documentation: 18: 37.2. Visibility of Data Changes

  +16 more

✅ Context preparation

  Commit messages: summarized; full text retained for this review

Review mode: Auto: ⚖️ Balanced: Downgraded extended -> standard: change is below the extended eligibility bar (hunks 12/18, lines 254/200; both must reach the floor). Router rationale: Dense DDL, trigger and concurrency logic across both dialects, with prior defects.

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 7c9554c

Results up to commit 33d903d ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Repository views can misattribute fixes ✓ Resolved

Resolving commit: 1145c0c

Resolution reason

Both dialects enforce that a remediation_finding row's repository_id matches its finding's
repository_id, via BEFORE INSERT/UPDATE triggers (Postgres function plus trigger; SQLite insert and
update triggers). A regression test checks that a mismatched repository is rejected and that
updating repository_id is rejected.

Description

remediation_finding independently references a finding and a repository without requiring that the
repository own the finding. When both referenced rows exist but belong to different repositories, an
insert succeeds and the per-repository remediation view can attribute that finding to the wrong
repository.

---

2. Empty-effort deletion goes untested ✓ Resolved

Resolving commit: 1145c0c

Resolution reason

At PR head, test_scheduled_findings_cannot_be_deleted_out_from_under_the_effort no longer uses a
finding ID for the empty-effort deletion. It creates empty = remediation(db) (line 489), deletes
it by its own ID (empty,) (line 490), and asserts it is absent (line 491). The no-op problem is
gone. The fix differs from the suggested prompt only in that the linked efforts are kept and the
unlinked one is created in-line, which is valid.

Description

test_scheduled_findings_cannot_be_deleted_out_from_under_the_effort passes other, a finding ID,
to a deletion from remediation. With no remediation matching that ID, the statement succeeds
without deleting a row, so the claimed empty-effort deletion rule is not exercised in either
dialect.

---

Results up to commit aef70bb ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Orders can name another repository's work ✓ Resolved

Resolving commit: 1145c0c

Resolution reason

Added repository-ownership guard triggers on run_order for insert and update of
work_item_id/repository_id in Postgres (fresh and upgrade) and SQLite (fresh and upgrade), rejecting
work items whose assessment's repository_ref belongs to a different repository, plus regression
tests for rejection in both dialects.

Description

run_order.work_item_id validates that an assessment exists but does not require its repository to
match the order’s repository_id. An order for one repository can therefore name an assessment
belonging to another; the same mismatch is accepted by the additive upgrade in both dialects.

---

Results up to commit 15fd2a5 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Concurrent moves misattribute fixes ✓ Resolved

Resolving commit: 40a9e66

Resolution reason

The insert trigger now takes FOR SHARE on the traust.finding row and reads its repository_id on a
locked row, so a concurrent move waits for the link commit (and vice versa). The
finding_under_remediation_stays trigger runs after the lock on a fresh snapshot, so the two
orderings cannot both commit a mismatch. The commit also adds a two-connection PostgreSQL regression
(test_a_concurrent_move_and_link_never_both_commit) that interleaves the two statements and checks
for zero mismatched rows. It uses row-level locking rather than the suggested composite FK / unique
parent key, so it is a different implementation.

Description

finding_under_remediation_stays checks visible links without synchronizing with a concurrent
insertion of remediation_finding. If an insertion checks the finding's old repository while
another transaction moves that finding before the link commits, both checks can pass and the
committed link names the wrong repository.

---

2. Concurrent ref moves misroute orders ✓ Resolved

Resolving commit: 40a9e66

Resolution reason

The order-side validation trigger run_order_work_item_of_repository now takes a FOR SHARE lock on
the repository_ref row (SELECT ... FOR SHARE OF r) before comparing repository ids, so a concurrent
ref move waits for the order and vice versa. Both bootstrap (run_order.sql) and upgrade
(run_order_work_item.sql) definitions are aligned, and a two-connection PostgreSQL regression test
(test_a_concurrent_ref_move_and_order_never_both_commit) interleaves the operations to verify that
they cannot both commit. The fix uses FOR SHARE on the ref rather than a lock on the ref's trigger
path, which is a different but valid serialization than the suggested fix prompt wording, though it
follows its recommended approach closely in spirit.

Description

ref_of_ordered_work_item_stays queries committed orders without synchronizing with an order being
assigned a work item. If that assignment validates the ref's old repository while another
transaction moves the ref before the order commits, both transactions can commit an order whose
repository differs from its work item's.

---

3. Manual rollback breaks finding updates ✓ Resolved

Resolving commit: 40a9e66

Resolution reason

Added test_the_documented_rollback_leaves_findings_refs_and_orders_working, which runs the PR's
documented rollback statements verbatim (including DROP TRIGGER IF EXISTS
finding_under_remediation_stays ON traust.finding for postgres and DROP TRIGGER IF EXISTS
finding_under_remediation_stays for sqlite, both before DROP TABLE remediation_finding), then
updates an unlinked finding's repository_id and asserts it works. This is the rollback coverage the
suggested fix asked for, and it verifies the trigger is dropped in each dialect. The diff does not
modify the sqlite schema or rollback docs themselves, but the rollback statements already drop the
trigger and the new test exercises that path.

Description

The new finding_under_remediation_stays trigger remains attached to finding when
remediation_finding is dropped. An operator following the PR's documented manual rollback leaves
that trigger in place, so a later finding repository update queries a missing table and fails in
either dialect.

---

Results up to commit b176d64 ⚖️ Balanced

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Upgraded databases scan decisions 🐞 Bug ➹ Performance

Description

run_order_work_item_upgrade loads only the run-order upgrade, while idx_decision_assessment is
defined only in the storage bootstrap and neither dialect's upgrade creates it. When an existing
database applies the exported additive upgrade without replaying the storage bootstrap, work-item
decision lookups remain without the index intended for the remediation fold and in-flight queries.

Code

db/v1/traust/postgres/schema/decision.sql[R62-64]

+CREATE INDEX IF NOT EXISTS idx_decision_assessment
+    ON traust.decision (assessment_id)
+    WHERE assessment_id IS NOT NULL;

Evidence

The index is created by the two decision schema files, but the exported upgrade loader reads only
the run-order upgrade files. The upgrade test applies that loader and replays only the ops run-order
bootstrap, so it does not establish that an existing database receives the decision index.

db/v1/traust/postgres/schema/decision.sql[61-64]
db/v1/traust/sqlite/schema/decision.sql[61-64]
src/traust_contracts/v1/ops/sql.py[57-64]
db/v1/traust_ops/postgres/upgrades/run_order_work_item.sql[1-14]
db/v1/traust_ops/sqlite/upgrades/run_order_work_item.sql[1-14]
tests/test_db_behavior.py[538-553]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Existing databases that apply the exported run-order upgrade without replaying the storage bootstrap do not receive the new decision assessment index.
## Fix Focus Areas
- db/v1/traust/postgres/schema/decision.sql[61-64]
- db/v1/traust/sqlite/schema/decision.sql[61-64]
- db/v1/traust_ops/postgres/upgrades/run_order_work_item.sql[1-14]
- db/v1/traust_ops/sqlite/upgrades/run_order_work_item.sql[1-14]
- tests/test_db_behavior.py[519-553]
## Recommended Fix
Provide an explicit, idempotent existing-database path that creates the decision index in both dialects, and test that an upgraded database receives it without replaying the storage bootstrap.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 75825fc

[smith-xyz]

@qodo relative to your first finding, the problem with that model is that it doesn't' give consumers the option to use a mirror that simply isn't tied to the repository. E.g. they're handling the fix in total isolation of the repo so the commit ref and the repository ref aren't in alignment.

I think this model gives more flexibility, that the the fk constraint here is that it has match back to a finding, then it gives room for more unique workflows.

[qodo-for-traust-sci]

relative to your first finding, the problem with that model is that it doesn't' give consumers the option to use a mirror that simply isn't tied to the repository. E.g. they're handling the fix in total isolation of the repo so the commit ref and the repository ref aren't in alignment.

I think this model gives more flexibility, that the the fk constraint here is that it has match back to a finding, then it gives room for more unique workflows.

Your mirror point is a fair one, but I don't think it applies to remediation_finding.repository_id. I checked the schema in this PR:

• traust.finding.repository_id is NOT NULL (db/v1/traust/postgres/schema/finding.sql, line 9), so every finding already has an owning repository.
• remediation_finding stores finding_id and repository_id as two independent FKs (remediation_finding.sql, lines 7-9). Nothing ties the repository to the finding's owner, so a row can pair a finding with a different repository.

The fork/mirror isolation you describe already lives in remediation.fork_url and remediation.fork_branch. The PR's own comment says fork_url is deliberately not a FK to repository. So a consumer can work in a mirror with an unaligned commit ref without the junction needing a second repository. Keeping repository_id on the junction only lets the pairing drift, and it doesn't buy any extra mirror flexibility.

Your "match back to a finding" idea is the right constraint. Two ways to get it without giving up the mirror model:

• Drop repository_id from the junction and derive the repository through finding. This is the simplest option, and the per-repo index can join through finding.
• Keep it, but make it a composite FK to finding(id, repository_id), backed by a UNIQUE (id, repository_id) on finding. This keeps the per-repo index usable and makes a mismatched pair impossible. Qodo's suggested fix for finding 1 points this way.

Either way, the test in tests/test_db_behavior.py should add a case that schedules a finding against a different existing repository and expects rejection. If you go with the first option, that test would instead assert the column is gone.

[smith-xyz]

@qodo ahh I misunderstood - AI is smarter than me.

but yes @gurnben I think we can still leverage repository_id as a fk? Even though this table can track out of band of that?

[qodo-code-review]

ahh I misunderstood - AI is smarter than me.

but yes @gurnben I think we can still leverage repository_id as a fk? Even though this table can track out of band of that?

Yes, you can keep repository_id as a foreign key to repository. It just needs a second constraint that ties it to the finding's owning repository.

The current schema checks that finding_id and repository_id each exist, but not that they belong together. A composite key fixes that:

• Add UNIQUE (id, repository_id) on finding (both dialects). id is already the primary key, so this is only there to give the composite key a unique target.
• In remediation_finding, replace the two independent FKs with FOREIGN KEY (finding_id, repository_id) REFERENCES finding (id, repository_id) ON DELETE RESTRICT.
• Keep REFERENCES repository(id) on repository_id as well, so the column still records the repository relationship on its own.

The UNIQUE (remediation_id, finding_id, repository_id) key and the repository_id index can stay as they are.

The one design question is whether you ever need to track a finding under a repository other than its owner. If so, the composite FK would block that, and the alternative is to drop repository_id and derive the repository through finding. If the table is only meant to record the owner, the composite FK is the simplest fix. This is the first item in the current review, finding 1 (Repository views can misattribute fixes).

[qodo-for-traust-sci]

ahh I misunderstood - AI is smarter than me.

but yes @gurnben I think we can still leverage repository_id as a fk? Even though this table can track out of band of that?

Yes, repository_id can stay a foreign key. It already is one in both dialects (REFERENCES traust.repository(id) ON DELETE RESTRICT), so the FK isn't the problem. finding 1 is about something narrower: the FK only proves the repository row exists, not that it's the finding's owning repository.

Those two goals conflict only if the table really should record repositories other than the finding's owner:

• If out-of-band tracking is intended (a finding in one repo is scheduled under another repo's remediation view), keep the current FK and make the behavior explicit. Document repository_id as the scope of the effort rather than the owner, and add a test that accepts a cross-repo pairing so it's a deliberate choice. In that case, finding 1 would be a design decision to reject, not a bug to fix.
• If the owner should always match, you can still keep repository_id as an FK and enforce the pairing too. Add UNIQUE (id, repository_id) on finding, then change the junction to a composite FK (finding_id, repository_id) REFERENCES finding(id, repository_id). This is the middle path finding 1 suggests, and it keeps the repository FK.

Either way, finding 2 is separate: its test deletes a finding ID from remediation, so it passes without deleting anything. It should use an unlinked remediation's own ID.

Which model do you want: repository as effort scope, or repository as the finding's owner?

[qodo-code-review]

relative to your first finding, the problem with that model is that it doesn't' give consumers the option to use a mirror that simply isn't tied to the repository. E.g. they're handling the fix in total isolation of the repo so the commit ref and the repository ref aren't in alignment.

I think this model gives more flexibility, that the the fk constraint here is that it has match back to a finding, then it gives room for more unique workflows.

Your point holds for finding 1 (repository views can misattribute fixes). The suggested fix, a composite FK that ties remediation_finding.repository_id to the finding's owning repository, would block the workflow you describe. A fix developed in an isolated mirror would have its commit and repository refs out of step, and the constraint would reject it.

The finding is right that repository_id is an independent fact the schema doesn't check against the finding. Your model treats it as "the repository this fix is attributed to," not "the finding's owner," and the only enforced link is back to the finding. That's a defensible design. The gap is that it's implicit:

• Document the intent. A comment in both remediation_finding.sql files saying repository_id is the attribution target and may differ from the finding's owner would stop future reviewers from raising this again.
• Make the per-repository view explicit. If the console's per-repo lane is meant to show fixes attributed to a repo, consumers should query on repository_id directly, not infer ownership from finding.
• Pin the mismatch case in a test. The current tests only use matching repositories. A test that schedules a finding against a different existing repository and asserts it's accepted would make the flexibility a deliberate contract.

If you agree, finding 1 can be dismissed as intentional. I can do that when you ask, but I haven't changed anything. Separately, finding 2 (the empty-effort deletion test passes a finding ID) is unaffected by this design question and still looks like a real bug.

[smith-xyz]

@qodo counter point - let's imagine if you will (or can?), someone wanted to run it twice to check on various implementations, yet maybe each on their own accord are able to successfully remediate, but they want to look at both and choose one. Maybe they do this n times, it doesn't matter.

... in that case what then?

[qodo-for-traust-sci]

counter point - let's imagine if you will (or can?), someone wanted to run it twice to check on various implementations, yet maybe each on their own accord are able to successfully remediate, but they want to look at both and choose one. Maybe they do this n times, it doesn't matter.

... in that case what then?

Under the current schema, nothing stops this. Running the remediation n times is allowed, and the schema has no notion of a chosen attempt.

• Multiple efforts per finding are allowed. Each run gets its own remediation row. remediation_finding only enforces UNIQUE (remediation_id, finding_id, repository_id) (db/v1/traust/postgres/schema/remediation_finding.sql:13), so the same finding can be linked to any number of efforts. The test test_scheduled_findings_cannot_be_deleted_out_from_under_the_effort asserts this: "a finding under one effort can be scheduled by another."
• Nothing records the winner. There is no "selected" flag, no pointer from finding to a remediation, and no uniqueness on an in-flight or merged effort per finding. remediation.fork_branch is only <> ''.
• Status is not enforced by the database. status is CHECK (status <> ''). The real values (candidate through merged/abandoned, from enums/v1/remediation-status.json) are checked at intake. Two efforts for the same finding could both reach pr_opened or merged with nothing in the DB to flag it.
• Losing attempts stay linked. Junction FKs are ON DELETE RESTRICT, so an abandoned loser keeps its remediation_finding rows and can't be deleted until those rows go. The test only covers an effort with no findings left. Check this against GC's sweep logic, which isn't in this PR.
• Repository attribution compounds it. This is the open issue in finding 1: remediation_finding doesn't tie repository_id to the finding's owning repository. With several attempts, a mismatched row would be harder to spot.

If you want "pick one of n" enforced rather than left to services, options include:

1. A partial unique index so only one effort per finding can be in a non-terminal or merged state.
2. A selected or chosen marker on remediation_finding or remediation.
3. Keeping it as a process rule and documenting that status is the source of truth.

I can draft any of these for the SQL and tests, or look at how the status-sync code is expected to handle losing attempts.

[qodo-code-review]

counter point - let's imagine if you will (or can?), someone wanted to run it twice to check on various implementations, yet maybe each on their own accord are able to successfully remediate, but they want to look at both and choose one. Maybe they do this n times, it doesn't matter.

... in that case what then?

The schema already supports running remediation several times against the same finding. Choosing a winner is not modelled yet.

• Each run is a separate row. Every attempt gets its own remediation row with its own fork_url, fork_branch, and status, so attempts don't overwrite each other.
• One finding can serve many efforts. remediation_finding is unique on (remediation_id, finding_id, repository_id), so the same finding can be scheduled under n remediations. The test test_scheduled_findings_cannot_be_deleted_out_from_under_the_effort checks this: "a finding under one effort can be scheduled by another."
• No "selected" flag exists. The only place to record the outcome is status. The schema doesn't enforce a state machine. Status values come from contracts and are checked at intake, so the likely pattern is one attempt moving to merged or forked and the others to abandoned. That logic would live in the services that write status.
• Discarding losing attempts takes care. The FKs are RESTRICT, so an effort can't be deleted while it still has remediation_finding rows. You have to unlink its findings first, then delete it. GC's terminal sweep relies on that ordering.

finding 2 (empty-effort deletion goes untested) matters here. Its final DELETE passes a finding ID instead of an unlinked remediation's ID. The statement succeeds without deleting anything, so the cleanup path for discarded attempts isn't actually tested. I'd fix that before relying on the cleanup flow for n candidates.

[qodo-for-traust-sci]

Code Review by Qodo

1. Upgraded orders can cross repositories 🐞 Bug ⭐ New

Description

The documented manual upgrade adds work_item_id but does not install
run_order_work_item_of_repository or the companion repository-ref trigger. Until an operator
separately recreates the database or installs those triggers, an order can name another repository’s
work item, and a ref can move away from the repository of an existing order.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The documented hand-upgrade adds the order columns but omits the pairing triggers, leaving upgraded databases able to store cross-repository work-item links.
## Fix Focus Areas
- db/v1/traust_ops/postgres/schema/run_order.sql[105-150]
- db/v1/traust_ops/sqlite/schema/run_order.sql[94-123]
- tests/test_db_behavior.py[485-518]
## Recommended Fix
Provide executable upgrade instructions or a migration that installs both dialects’ order and ref-move guards along with the columns, and add regression coverage that starts with the previous schema, performs the upgrade, and rejects a mismatched order and ref move.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗

2. Orders still run the wrong work item 🔗 Cross-repo conflict

Description

The new run_order.work_item_id is nullable, but traust-core’s scheduler submits remediation orders
without the assessment ID it just created, and its order model has no field to carry it. When a
repository has an older pending work item, the runner’s next_pending path selects that item
instead of the one for the newly scheduled order.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new order column cannot identify scheduled work because the linked traust-core scheduler does not populate or consume it.
## Fix Focus Areas
- db/v1/traust_ops/postgres/schema/run_order.sql[39-39]
- db/v1/traust_ops/sqlite/schema/run_order.sql[39-39]
- /cross_repos/traust-core/src/traust_core/v1/services/remediation_scheduling.py[102-123]
- /cross_repos/traust-core/src/traust_core/v1/models/operations.py[473-492]
## Recommended Fix
In traust-core, add the field to order requests, models, and repository mappings; submit the newly created assessment ID and have the runner retrieve that named item. Cover two pending work items for one repository, and apply the supplied column upgrade before deploying the new reader or writer to existing databases.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗

[qodo-code-review]

Code review by qodo was updated up to the latest commit aef70bb

[chubirka]

traust-core#377 (b8038c45) now pins this branch at aef70bb and uses run_order.work_item_id: scheduled orders name their work item, and run_order_work_item_upgrade is applied on open for local stores. Once this merges, #377 gets re-pinned to the merge commit.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 1145c0c

[chubirka]

1145c0c addresses the aef70bb review:

• Repository views can misattribute fixes / Repositories show the wrong remediations: a guard trigger on remediation_finding (insert, and update of finding_id/repository_id, in both dialects) requires repository_id to be the finding's owner. It uses a trigger rather than a composite FK, so finding needs no new unique key. This follows the spend_event lane-of-order guard. A missing finding is still left to its FK.
• Orders can name another repository's work: a guard trigger on run_order requires work_item_id to be a work item whose ref (assessment → repository_ref) is on the order's repository. run_order_work_item_upgrade installs the same guard.
• Empty-effort deletion goes untested: the test now deletes an empty effort by its own id and asserts it is gone.
• Core cannot load new remediation tables / Orders still run the wrong work item: traust-core#377 is pinned at 1145c0c (b3ae2f89). Its create submits the order with the work item it opened, and its local stores are upgraded on open.

Changed tests (new on this branch): test_scheduled_findings_cannot_be_deleted_out_from_under_the_effort (the last delete is now a real empty effort), test_a_run_order_names_a_real_work_item_or_none (orders name their repository; a mismatch and a missing repository are rejected), and test_run_order_work_item_upgrade_… (drops the guard before the column; asserts the upgrade brings the guard back).
New test: test_a_scheduled_finding_is_on_the_repository_that_owns_it.

Suite: 1,342 passed on SQLite and Postgres.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 15fd2a5

[chubirka]

15fd2a5 addresses Moved findings appear in the wrong repo and Orders retain a moved work item. A finding under remediation can no longer change repository. A repository_ref that holds a work item named by an order can no longer move to another repository; assessments are immutable, so a moving ref was the only remaining way to break that pairing. run_order_work_item_upgrade installs the second guard too. Tests extend test_a_scheduled_finding_is_on_the_repository_that_owns_it and test_a_run_order_names_a_real_work_item_or_none (the move is refused, and an unlinked finding or unordered ref still moves). The upgrade test drops the new guard along with the column. Suite: 1,342 passed on SQLite and Postgres. traust-core#377 is pinned here (f50d88ab).

[chubirka]

40a9e66 addresses the 15fd2a5 findings:

• Concurrent moves misattribute fixes / Concurrent scheduling mislabels findings / Concurrent ref moves misroute orders / Concurrent orders retain moved work items (Postgres only; SQLite has one writer): both forward guards now take FOR SHARE on the row they compare against (the finding, or the work item's repository_ref). If the move comes second, it waits for the link or order, and its reverse guard then runs on a fresh snapshot and sees it. If the link or order comes second, it waits for the move in flight and then reads where the row went. run_order_work_item_upgrade carries the same change. New two-connection tests, test_a_concurrent_move_and_link_never_both_commit and test_a_concurrent_ref_move_and_order_never_both_commit, run both interleavings for each pairing. Both tests failed on 15fd2a5 before the fix.
• Manual rollback breaks finding updates: the PR description's rollback now drops the guards on finding and repository_ref, and the run_order column as well. test_the_documented_rollback_leaves_findings_refs_and_orders_working runs those statements and then moves an unlinked finding and ref.
• Orders still run the wrong work item / Core cannot load new remediation tables: these are about the core pin. traust-core#377 is re-pinned to 40a9e66.

Suite: 1,346 passed (SQLite and Postgres).

[qodo-code-review]

Code review by qodo was updated up to the latest commit 40a9e66

[gurnben]

@smith-xyz — catching up on your design questions from last night; here's where they landed:

Can we still leverage repository_id as an FK? Yes, and it's in: repository_id keeps its plain FK to repository, and the pairing is enforced by guard triggers instead of a composite FK — remediation_finding_repository_of_finding (junction insert/update) plus, since 15fd2a5, the reverse side on the finding itself, so a scheduled finding can't move repositories and strand its links. That follows the spend_event lane-of-order guard pattern, and it avoids adding a UNIQUE (id, repository_id) to finding just to give a composite key a target. On Postgres the guards take FOR SHARE on the row they compare against (40a9e66), so a concurrent move and a concurrent link can't both commit.

The mirror workflow you described (fix handled in total isolation, commit ref and repository ref out of alignment) lives in remediation.fork_url / fork_branch, which are deliberately not FKs — the mirror can be anywhere. The junction's repository_id is the finding's owner, not the effort's scope, so per-repository views can't misattribute a fix, and the isolated-mirror workflow keeps working: nothing requires the fork to be tied to the repository.

Running it n times and choosing one: the schema allows exactly that — UNIQUE (remediation_id, finding_id, repository_id) permits any number of efforts per finding, each with its own fork, branch and status. There's no "selected" marker by design: the winner is a process rule (one effort moves to merged, the rest to abandoned; status values are checked at intake, and core's record door keeps a replayed verdict from moving a merged/abandoned effort back). If you want the DB to enforce "one chosen effort per finding" — a partial unique index or a selected flag — that's a contracts change worth its own discussion; say the word and I'll open the follow-up.

Everything Qodo raised on this branch is resolved as of 40a9e66 (its review now reads 0 bugs), and traust-core#377 is re-pinned to it.

[smith-xyz]

triple checking the assessment table, we might already have a way to accomplish this - verifying against our use cases here

[gurnben]

@smith-xyz a few schema facts for that verification, since I just walked them:

• traust.assessment is append-only — assessment_reject_mutation rejects UPDATE and DELETE outright (db/v1/traust/postgres/schema/assessment.sql:48-53, same in SQLite). So the assessment table can't carry a mutable "selected" marker; whatever the accomplishes-this shape is, it isn't a column flipped on the assessment.
• What the assessment table already gives the n-efforts flow: each effort's assessment is its own row, remediation.assessment_id names it, and run_order_id records the order that produced each run — so the history of n attempts is fully reconstructable today. baseline_assessment_id is the precedent for an assessment pointing at another assessment, but it's write-once at intake, not a later choice.
• The mutable surfaces that could hold a choice today: remediation.status (the process rule — one effort merged, the rest abandoned, moved by the record/verification doors) and the decision/decision_approver tables (where human judgement already lives, with auto_accept_tier as the precedent for a flag on an assessor's record). If the use cases want the DB to enforce "one chosen effort per finding" rather than trust the status flow, the candidates are a partial unique index over the chosen status on remediation/remediation_finding, or recording the choice as a decision — both are contracts changes, not assessment changes.

Happy to prototype whichever shape the use cases point at.

[smith-xyz]

Reviewing this against the settled data model (attempt = assessment, status = decision, fork/patch/checks = artifact) — keeping the run_order work, dropping the two tables, shrinking the contracts change to work_item_id + the fork columns on run_order. Details inline. Happy to pair on the #377 reshape.

[smith-xyz]

I'd like to drop these two tables and keep the contracts change minimal — run_order.work_item_id is exactly the right shape, but the rest duplicates what already shipped:

• remediation.schema.json is already the system of record for the out-of-band world — fork url, fix_branch, checks, revalidation, PR, and summary.status are all in it.
• The candidate decisions #377 writes on the work item already link findings to each effort.

So we already have the pathway for tracking fixes that live outside the repo, and the artifact gives the flexibility for free — a document can describe any mirror or workflow without the DB's invariants needing to know it exists. The judgment trail stays on the real repo's rails (decision.fixed_repository_ref_id for landing), which covers the three cases we talked about: out-of-band mirror verification (comparison assessment, resolution closes the audited line), track-until-it-lands (candidate → fixed → landed decisions), and n-attempts-choose-one (one work item per attempt, the choosing decision names the winner).

What the runner still needs at claim is the fork facts — nothing else carries them between scheduling and the run's own artifact. I'd put them on run_order beside work_item_id:

    work_item_id UUID REFERENCES traust.assessment(id) ON DELETE RESTRICT,
    fork_url TEXT,       -- the mirror this remediation runs on (remediate_finding only)
    fork_branch TEXT,    -- its fix branch

Write-once at scheduling, read-once at claim, no cross-row query — column-shaped, and it keeps the whole contracts change to one table: three columns, one index, the pairing guards you already built.

[smith-xyz]

Beyond the duplication with the candidate decisions, repository_id here can't carry information: remediation_finding_repository_of_finding enforces it always equals finding.repository_id, so every query on it is a join on finding in disguise.

It's also what buys the reverse guard on finding (a finding under remediation can never move repositories) and the FOR SHARE machinery serializing the two — most of the diff's complexity exists to maintain a column that is by construction always equal to another table's column. Dropping the column (or the table) deletes that machinery with it.

[smith-xyz]

This part I'd keep in full — the column, the partial index, the pairing guards, and the additive upgrade with its never-invent-a-work-item test. It's the one thing the model genuinely lacked: the runner's link from an order to the work item it runs.

Only addition: since the remediation table goes away (see the comment there), add fork_url / fork_branch here as nullable columns so the order carries the mirror facts to the runner at claim.

[smith-xyz]

One thing while you're in here: the dashboard's in-flight query (orders → work item → decisions) has every leg indexed except decision(assessment_id) — decisions are only indexed by finding_id and occurred_at, so fetching a work item's decisions scans. Worth adding idx_decision_assessment ON traust.decision (assessment_id) WHERE assessment_id IS NOT NULL (both dialects) to this PR, with an EXPLAIN (ANALYZE) at seeded volume in perf/queries/ to show it earns its keep — the house rule is measure first, and it'd serve the status fold too, not just dashboards.

[smith-xyz]

Taking this one off your plate @gurnben — reworking the branch per the review: dropping the two remediation tables, keeping your work_item_id + pairing guards + upgrade in full, and adding fork_url/fork_branch on run_order plus the decision(assessment_id) index. Force-push incoming, so the review's line anchors will go stale — the discussion threads stay. Tests pass on SQLite + Postgres; measurement numbers in the commit message. Michele: this still needs your review as DDL.

[qodo-code-review]

Code review by qodo was updated up to the latest commit b176d64

[smith-xyz]

bnote the upgrade script , we need a more formal upgrade path so that needs removed for now and handled manually. once stable we can add that feature

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7c9554c

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gurnben, smith-xyz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [gurnben,smith-xyz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment