chore: purge identified terraform modules via dbpurge - #28802
Conversation
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 6 findings (1 P0, 1 P2, 1 P3, 1 Nit, 2 Note), COMMENT. Review Finding inventoryFinding inventory - PR #28802Findings
Round logRound 1Netero-only first-pass gate. P0 (test window mismatch, matches the CI failure), 1 P2, 1 P3, 1 Nit, 2 Notes. Panel not yet run; deferred until mechanical floor is clean. Reviewed against a93b225..99c52af. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
…t window The test copied the ingest window from the production constants. Widening those constants to 08:00-22:00 left the copy at 08:35:52-09:09, so the fixture placed at the exclusive upper bound fell inside the real window and was deleted, failing the boundary assertion. Pass the window through a WithIdentifiedModuleCacheWindow option instead, matching WithChatSearchBackfillLimits. The test then asserts the boundary behavior it is actually about, and revising the incident timestamps cannot invalidate it. An internal test guards the constants themselves.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review only. These are mechanical findings from Netero; the full review panel has not yet reviewed this PR. The panel will review once the first-pass findings, particularly the P0, are addressed.
The change is well-scoped: a one-off dbpurge cleanup guarded by a per-process latch, with a data-modifying CTE that clears the FK reference before deleting the file, and a thorough integration test covering both bounds, the negative cases, and the latch. The rationale comments on why this lives in dbpurge rather than a migration are genuinely useful.
One blocker: the integration test's window constants do not match the production constants, so the test deletes fixtures it asserts should survive and fails deterministically. This is the red CI (test-go-pg, test-go-pg-17). As Netero put it, the boundary assertions are "worthless as written: neither windowStart nor windowEnd is a boundary of the code under test." Deriving the fixtures from the production constants (export them, or make the window an instance field set by an option) fixes the failure and stops the test from drifting from the code again.
Severity count: 1 P0, 1 P2, 1 P3, 1 Nit, 2 Notes.
🤖 This review was automatically generated with Coder Agents.
2 participants
No description provided.