FE-674: Inline source diff + edit-target on pending review rows#120
Open
kostandinang wants to merge 7 commits into
Open
FE-674: Inline source diff + edit-target on pending review rows#120kostandinang wants to merge 7 commits into
kostandinang wants to merge 7 commits into
Conversation
This was referenced May 8, 2026
Contributor
Author
1 task
kostandinang
changed the title
kostandinang
force-pushed
the
ka/fe-674-plan-v3-1-verification
branch
from
51310d0 to
a492dcf
Compare
kostandinang
force-pushed
the
ka/fe-674-cascade-edit-and-agent
branch
from
b5f8b9b to
5c3a70c
Compare
3 tasks
PR SummaryMedium Risk Overview Upgrades the Pending review UI to (a) render an inline Introduces a new Reviewed by Cursor Bugbot for commit c5bb7ef. Bugbot is set up for automated code reviews on this repo. Configure here. |
🤖 Augment PR SummarySummary: Extends the reconciliation-need (Pending review) workflow for side-chat V3.1 and completes node-edit UX affordances, plus adds a first-cut reconciliation classifier “run-agent” backend. Changes:
Technical Notes: The agent route is intentionally in-process and per-row updates are durable via incremental lifecycle transitions; 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
kostandinang
force-pushed
the
ka/fe-674-plan-v3-1-verification
branch
from
a492dcf to
7fc63be
Compare
kostandinang
force-pushed
the
ka/fe-674-cascade-edit-and-agent
branch
2 times, most recently
from
7e4d0d8 to
98c902c
Compare
kostandinang
force-pushed
the
ka/fe-674-plan-v3-1-verification
branch
from
9ed2e7b to
9e27baa
Compare
kostandinang
force-pushed
the
ka/fe-674-cascade-edit-and-agent
branch
from
98c902c to
1caa9f8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1caa9f8. Configure here.
kostandinang
force-pushed
the
ka/fe-674-cascade-edit-and-agent
branch
from
1caa9f8 to
81a4822
Compare
kostandinang
force-pushed
the
ka/fe-674-plan-v3-1-verification
branch
from
9e27baa to
642f4f7
Compare
kostandinang
force-pushed
the
ka/fe-674-cascade-edit-and-agent
branch
from
81a4822 to
d36f61a
Compare
kostandinang
force-pushed
the
ka/fe-674-plan-v3-1-verification
branch
from
642f4f7 to
387cbb5
Compare
kostandinang
force-pushed
the
ka/fe-674-plan-v3-1-verification
branch
from
387cbb5 to
95f6976
Compare
kostandinang
force-pushed
the
ka/fe-674-cascade-edit-and-agent
branch
from
d36f61a to
1f26ae7
Compare
memory/CARDS.md is the per-branch execution queue for the next three
buildable scope cards on this frontier (ka/fe-674-cascade-edit-and-agent):
1. Source-content snapshots on reconciliation_need (server)
- Add source_previous_content + source_current_content TEXT NULL
- Thread through OpenReconciliationNeedInput + edit-route hard path
- Expose on ReconciliationNeedRecord shared type
2. Source diff rendered inline on each Pending review row (client)
- Reuse <ContentDiff> from FE-665
- Render only when both snapshots are present and non-equal
- Legacy rows (snapshots null) keep today's bare layout
3. Edit-target affordance per Pending review row (client + reuse)
- Inline textarea pre-filled with target's current content
- Saves through existing PATCH /knowledge-items/:id then resolve endpoint
- Re-entrant cascade works without reload
All three are light scope cards; promotion checklist run on each, all
stay light (no new requirements, assumptions, decisions, invariants;
all stay inside the V3.0 cascade seam).
The V3.1 agent slices (4-6) are deliberately NOT pre-queued -- they
depend on Card 3's inline-edit shape and on the LLM oracle strategy
landing in PR #119 (ka/fe-674-plan-v3-1-verification). Re-run ln-scope
for slice 4 after Card 3 ships.
CARDS.md follows AGENTS.md's sanctioned exception: it is a derivative
queue scoped to one branch and one frontier item, not canonical
narrative -- so it lives on the feature branch, not the planning PR
(per /planning-pr decision rule).
Adds two nullable TEXT columns to reconciliation_need so the cascade
producer can freeze the source item's before/after content on the need
row at open time. Frozen snapshots flow through the GET listing
endpoint and are exposed on the shared ReconciliationNeedRecord type
for the client (cards 2 + 3) and the V3.1 classifier pre-image.
Why frozen-on-row instead of derived: the source knowledge_item is
mutable. By the time the user reviews a Pending review row, the source
content may have changed again. A snapshot captured at need-open time
gives the user the diff that triggered THIS need, not the latest delta.
Why nullable: legacy rows (created before this change) and direct test
seeds that bypass the cascade producer should round-trip cleanly. Card
2 (the inline source diff in <PendingReviewSection>) only renders the
diff when both fields are present and non-equal.
Why advisory, not load-bearing: the snapshots are render data and
agent pre-image, not a foundation for any invariant. caused_by_turn_id
remains the durable provenance link. No I### added; no D### / A##
changed.
Schema + migration:
- drizzle/0018_reconciliation_need_source_snapshots.sql adds two
ALTER TABLE statements; meta/_journal.json gets idx 18 entry
(timestamp 1776350000000, +10s after 0017 to preserve ordering).
- schema.ts: source_previous_content + source_current_content as
nullable text() columns; doc-comment explains the advisory stance.
Server seams:
- OpenReconciliationNeedInput: optional sourcePreviousContent +
sourceCurrentContent; openReconciliationNeed writes them through
(defaulting to null). openReconciliationNeedIfAbsent passes them
transparently since it forwards the whole input.
- edit-route.ts hard path: passes the existing previousContent
variable plus parsed.data.content (the just-applied content) into
every openReconciliationNeedIfAbsent call. No new variables; no
extra reads from the DB.
Shared:
- ReconciliationNeedRecord gains the two fields as string | null;
the GET reconciliation-needs response inherits the shape via
drizzle's InferSelectModel so no route handler change is needed.
Tests (red → green, all on the inner loop per the V3.1 oracle
strategy in SPEC.md §Verification Design):
- reconciliation-need.test.ts: snapshot-columns existence test;
new lifecycle test for the snapshot write-through path; legacy
test now also asserts both fields default to null.
- edit-route.test.ts: the existing hard-impact cascade test now
asserts every opened need carries 'Central goal' as previous and
'Updated goal' as current.
- reconciliation-needs-route.test.ts: the response-shape test seeds
one need with snapshots and one without, asserts both round-trip
correctly through the wire.
- reconciliation-need-fixtures.ts (client): builder gains the two
nullable defaults so existing patch-list-overlay /
pending-review-section tests stay typesafe.
CARDS.md: card 1 marked done; cards 2 + 3 remain next.
Verification: - Inner: npm run verify — all reconciliation-touching tests green
(47 of 47 in the three updated files); full suite at 1098 passed
(+1 net new test); 2 inherited failures in
structured-list-view.test.tsx remain unchanged from cascade-polish
baseline.
- oxlint type-aware + type-check + oxfmt: clean.
- vite build (client + server-runtime): clean.
Each row in <PendingReviewSection> now renders a <ContentDiff> of the
source item's before/after content directly under the source/target
reference line, so the user can read what changed without leaving the
cascade surface or hovering anything.
Implementation is pure composition over Card 1's snapshots and FE-665's
existing <ContentDiff> component. No new diff library, no new
tokenization, no new styling — same colors, same word-level split, same
accessibility surface.
Conditional rendering (the only product logic):
- Both snapshots present + non-equal → diff renders, label
'Source change' shown
- Either snapshot is null (legacy rows) → no diff, no label
- Both present but before === after → ContentDiff returns null
by design; the gate also
omits the label
Card 1's contract makes the snapshot fields nullable on purpose so
any row produced before this change (or by a test seed that bypasses
the cascade producer) renders cleanly with the bare layout.
Layout change in <PendingReviewSection>:
- Each <li> goes from a horizontal flex (label/refs + Resolve) to a
vertical flex column. The header row preserves its
items-center/justify-between layout inside an inner <div>; the
diff hangs below at gap-1.
- List gap goes from gap-0.5 to gap-1.5 to give multi-row content
breathing room.
- 'Source change' label is supplied via ContentDiff's existing label
prop (already styled as the 10px tracking-wide hint label
elsewhere in the app).
Tests (4 new under 'source diff inline (card 2)'):
- renders the source diff when both snapshots present and differ
(asserts data-content-diff plus removed + added word segments)
- labels the source diff with 'Source change'
- renders no diff when either snapshot is null (legacy rows;
Resolve buttons still work)
- renders no diff when before === after (ContentDiff returns null;
label is also absent)
Existing 7 tests in the file (region count, kind chips, source/target
refs, Resolve interaction, double-click guard, mutation pending state,
empty-state collapse) all stay green untouched.
CARDS.md: card 2 marked done; card 3 remains next.
Verification: - Inner: pending-review-section.test.tsx — 11 of 11 green;
full suite at 1102 passed (+4 net new tests, all the card 2
cases); 2 inherited failures in structured-list-view.test.tsx
remain unchanged from cascade-polish baseline.
- oxlint type-aware + type-check + oxfmt: clean.
- vite build (client + server-runtime): clean.
Each row in <PendingReviewSection> gains an Edit target button that
expands an inline textarea pre-filled with the target item's current
content. Save sequences edit → resolve → invalidate so the row leaves
the section atomically; Cancel collapses without a request. The whole
cascade-resolution surface now offers all three node-edit affordances:
view source diff (card 2), edit target inline (this card), or Resolve
as-is (V3.0 baseline).
Architectural choice — target content source of truth:
The card explicitly asked for one of two paths: (a) read target
content from a separately-mounted knowledge-items query, or (b)
thread it as a new field on ReconciliationNeedRecord populated by the
listing endpoint. This commit chooses (b).
Why (b): the existing component test scaffolding mocks the open-needs
hook directly without any QueryClientProvider, so adopting (a) would
have required threading a second query through the whole render-time
contract. Threading the field via the listing endpoint reuses the
one query the section already depends on, keeps the type-shape one
place (ReconciliationNeedRecord), and inherits the existing refetch
cadence so target content is fresh-as-of-last-listing.
Why this is read-time enrichment, NOT a table column: the field
reflects mutable knowledge_item.content at query time and would be
meaningless if persisted (it would be wrong the moment the target
edits via any other surface). Card 1's source_previous_content and
source_current_content remain on the table because they are frozen
by design at need-open time.
Reversibility: switching to (a) later is mechanical — drop the field
from the response, mount the items query, swap the prop. The
ReconciliationNeedView wire type is the only seam that would change.
Server (src/server/reconciliation-needs-route.ts):
- New ReconciliationNeedView type extends the persistent row with
target_current_content: string | null.
- handleListOpenReconciliationNeeds enriches each row by calling
getKnowledgeItem on the target_item_id. Per-row N+1 lookups are
deliberate; open-need counts are single-digit per spec in practice
so a drizzle leftJoin layer is premature. Comment flags the
promotion trigger (latency under manual walkthrough).
- target_current_content is null when the target item is missing
(race / partial state); FK cascade normally takes care of this.
Shared (src/shared/reconciliation-need.ts):
- ReconciliationNeedRecord gains target_current_content: string | null
with a doc-comment explaining the read-time-only stance.
Client (src/client/components/pending-review-section.tsx):
- Two new useState slots: editDrafts (Map<needId, draft>) tracks
which rows are in edit mode + their textarea text; savingNeedIds
tracks in-flight saves.
- startEditing / cancelEditing / updateDraft / handleSave manage the
per-row lifecycle; handleSave sequences editKnowledgeItemRequest
→ resolveReconciliationNeedRequest → invalidate, then collapses.
- Per-row JSX: Edit target button hidden when target_current_content
is null (user falls back to Resolve). When editing, the row expands
a textarea (aria-label per row) plus Cancel + Save buttons; both
Resolve and Edit target disable while a save is in flight.
- Header refs/buttons regrouped into right-side flex span so Edit
target sits immediately left of Resolve at the same vertical line.
Re-entrant cascade behavior:
Save calls editKnowledgeItemRequest which itself can return
impact: 'hard' and open new reconciliation_need rows downstream of
the target. After invalidate, those new needs surface in the same
Pending review section on the next render. The current resolve still
closes THIS need atomically; new needs are independent rows that
the user reviews in the same surface — no recursion, no special UI.
This is on the watch list per SPEC.md §Acknowledged Blind Spots
("Re-entrant cascade behavior under inline target edit"); the
current oracle is the inner-loop integration test in this commit
plus the future outer-loop walkthrough.
Tests (6 new in pending-review-section.test.tsx 'Edit-target inline
form (card 3)' block, 1 new in reconciliation-needs-route.test.ts):
Server:
- exposes the target item current content on each open need (live join)
Client:
- renders an Edit target button per row when target_current_content
is present
- expands an inline textarea pre-filled with target_current_content
- Cancel collapses the form without calling any request
- Save calls editKnowledgeItemRequest then
resolveReconciliationNeedRequest then invalidates (with the
correct (specId, targetItemId, { content }) and (specId, needId)
argument shapes)
- disables Save and Resolve while the edit is in flight
- Edit target button does not appear when target_current_content
is null
Existing 11 tests in pending-review-section.test.tsx (region count,
kind chips, source/target refs, Resolve interaction, double-click
guard, mutation pending, empty-state collapse, plus the four card 2
source-diff tests) all stay green.
reconciliation-need-fixtures.ts: makeNeed gains target_current_content
default of null; existing fixtures stay typesafe.
CARDS.md: card 3 marked done; the 'Not yet queued' section (slices 4-6
for V3.1 agent + bulk actions) is preserved as the next-scoping guide.
The next /ln-scope run re-writes the queue with the agent backend
slice using the oracle strategy already in SPEC.md §Verification Design.
Verification: - Inner: pending-review-section.test.tsx — 17 of 17 green;
reconciliation-needs-route.test.ts — 7 of 7 green;
full suite at 1109 passed (+7 net new tests across the three
cards: 1 new in card 1, 4 in card 2, 7 in card 3 — matching the
+1 / +4 / +7 progression in commit 1, 2, 3); 2 inherited
failures in structured-list-view.test.tsx remain unchanged from
cascade-polish baseline.
- oxlint type-aware + type-check + oxfmt: clean.
- vite build (client + server-runtime): clean.
…route)
Backend for the V3.1 reconciliation-classifier agent. Walks every open
`reconciliation_need` row in a specification through one in-process LLM
classification per call, persisting one of `{auto-confirm, auto-edit,
substantive}` and an optional text proposal. Lifecycle is the
observable contract per I114:
null → queued → classifying → classified | failed
Recoverability is structural: invalid labels and thrown LLM errors
both transition to `failed` with the diagnostic in `agent_proposal`;
`agent_proposal` is text-only and is never auto-applied (slice 6 owns
the user-initiated Apply / Skip actions). That recoverability is what
lets the inner-loop tests stay shallow (state machine + parse +
purity) per SPEC.md §Verification Design row 553. Middle-loop seed
corpus is shipped as a JSON resource for the runner that arrives in a
later slice; outer-loop A88 walkthrough waits for slice 6.
Schema + migration:
- drizzle/0019_reconciliation_need_agent_columns.sql adds three
nullable TEXT columns (agent_status, agent_classification,
agent_proposal). meta/_journal.json idx 19 entry uses the same
+10s offset as 0018 (1776360000000) per HANDOFF.md non-TTY
caveat.
- schema.ts: enum-bounded text() columns matching the I114 label
vocabulary, with doc-comment naming the lifecycle and the
text-only-never-auto-applied stance.
Pure function (src/server/reconciliation-agent.ts):
- classifyNeed(input, runModel) → { status, classification, proposal }
is pure: same input + same stub-returned string yields the same
output. The runModel callback is the single LLM swap point so
tests can stub it without touching any provider.
- reconciliationClassificationSchema (Zod) validates the model
response inline; failures collapse into a 'Parse error: ...'
proposal on the row instead of throwing into the route.
- Snapshot fallbacks: null source snapshots collapse to '(no
recorded snapshot)' in the prompt; missing relation collapses to
'(unknown)'. Either fallback still classifies; user sees the
label and can re-run with a stronger pre-image if needed.
- defaultRunModel routes through generateText + Output.object on
the AI SDK adapter already used by side-chat-route + observer
(RECONCILIATION_CLASSIFIER_MODEL env var; defaults to claude-haiku-4-5).
Prompt asset (src/server/prompts/reconciliation-classifier.md):
- Registered via the existing prompt-loader pipeline. Variables:
{{source_previous}}, {{source_current}}, {{target_current}},
{{relation_kind}}, {{need_kind}}.
- Classifier is conservative by design: when in doubt, choose
substantive and let the user decide; never propose a mechanical
rewrite when the change is judgment-laden.
Route (src/server/reconciliation-agent-route.ts):
- POST /api/specifications/:id/reconciliation-needs/run-agent
- listOpenReconciliationNeedsAwaitingClassification filters to
status=open AND agent_status IS NULL — already-classified rows
stay untouched, so re-invoking the endpoint while rows are in
flight is safe (it skips them; slice 5's per-row Re-run resets
agent_status to null to re-pick).
- In-process serial loop with one UPDATE per transition so partial
progress is durable. Per HANDOFF.md the per-spec open-need count
stays single-digit in practice; promote to a queue substrate
only if outer-loop walkthroughs surface user-visible blocking.
- Orphan needs (source/target item missing at classification
time) transition to `failed` with a structured note rather than
throwing. 404 / 400 parity with the existing reconciliation-needs
route.
- Returns { specId, ranAt, classifiedCount, failedCount }.
Db helpers (src/server/db.ts):
- listOpenReconciliationNeedsAwaitingClassification: open + agent_status IS NULL.
- updateReconciliationNeedAgentFields: partial update over the
three agent_* columns; one transition per call, callers own the
order.
- getCascadeRelationBetween: looks up the typed dependency edge
that caused a (source, target) pair, returning undefined for
orphan needs (graceful fallback into the prompt).
Wire shape (src/server/reconciliation-needs-route.ts inheritance):
- GET /api/specifications/:id/reconciliation-needs response
automatically exposes the three new fields via InferSelectModel.
No handler change needed; the new wire-shape test asserts the
fields appear with null defaults so slice 5's status chips and
slice 6's action buttons can render without a separate query.
- shared/reconciliation-need.ts: ReconciliationNeedRecord gains
the three fields plus exported ReconciliationNeedAgentStatus and
ReconciliationNeedAgentClassification union types so the client
code stays typesafe.
Tests (red → green, all on the inner loop):
- reconciliation-need.test.ts: schema columns exist; lifecycle
test now also asserts agent_* fields default to null on new rows.
- reconciliation-agent.test.ts (new, 9 tests): happy paths for
each of the three labels; failed path on stub throw; failed
path on invalid label; failed path on non-object return; purity;
fallback rendering when source snapshots are null; fallback when
relationKind is undefined.
- reconciliation-agent-route.test.ts (new, 7 tests): empty case
(no awaiting needs, generateText not called); happy path with
two needs walks through classified + persists labels; failed
path persists 'LLM unavailable' to agent_proposal; invalid
label persists 'Parse error: ...'; already-classified rows are
skipped (generateText called once for the only fresh row); 404
on missing spec; 400 on non-numeric spec.
- reconciliation-needs-route.test.ts: extends with one wire-shape
test asserting the listing endpoint exposes agent_* fields with
null defaults.
- reconciliation-need-fixtures.ts (client): makeNeed gains null
defaults for the three new fields; existing patch-list-overlay /
pending-review-section tests stay typesafe.
Middle-loop seed corpus:
- src/server/__corpus__/reconciliation-classifier-seeds.json
seeds five (source change, target content, relation kind) →
expected classification tuples per the scope card §Verification
Approach. The runner that exercises these against the live AI
SDK adapter is its own slice; this slice ships only the seed
+ the prompt the runner will exercise.
SPEC.md:
- I114 added with the four named test files as oracles. Existing
'planned I114' references in §Verification Design rows 553-554
swap to live I114; row 554 now also names the seed corpus
location so the future runner slice has a concrete starting
point.
- No new D### — single-shot LLM call + in-process loop stay
deliberate MVP choices with documented promotion triggers in
the scope card; will promote only if outer-loop walkthroughs
surface user-visible blocking or label instability.
- A88 stays open — slice 4 does not validate it; slice 6 outer-
loop walkthrough does.
CARDS.md:
- Card 5 (the V3.1 agent backend in the post-rewrite numbering)
marked done. Cards 6-7 (V3.1 client + bulk actions) remain in
the 'Not yet queued' section per the scope card's deferred-
until-after-this-slice rule.
Verification: - Inner: npm run verify — 1126 passed (+17 net new tests across
schema/agent/route/wire-shape suites); 2 inherited failures in
structured-list-view.test.tsx remain unchanged from cascade-
polish baseline (per HANDOFF.md).
- oxlint type-aware + type-check + oxfmt: clean.
- vite build (client + server-runtime): clean.
Textarea, Cancel, and Save now also check isResolving, so a click can't fire Save mid-resolve and cause overlapping mutations on the same need.
Replace the unconditional 'queued' UPDATE with a conditional one that requires agent_status IS NULL, and skip the row when the UPDATE matches zero rows. Two concurrent run-agent requests can no longer both pick up the same row, double-call the model, or race on later transitions.
kostandinang
force-pushed
the
ka/fe-674-cascade-edit-and-agent
branch
from
1f26ae7 to
c5bb7ef
Compare
kostandinang
force-pushed
the
ka/fe-674-plan-v3-1-verification
branch
from
95f6976 to
466e4cb
Compare
6 tasks
kostandinang
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Lets the user fix downstream items inline on each Pending review row. Three cards.
Stacked on #119.
How
reconciliation_neednow stores the source's previous/current content (migration0018), frozen when the need opens.editKnowledgeItem → resolveReconciliationNeed → invalidateso the row leaves atomically. Cancel collapses without a request.target_current_contentis enriched at read-time on the listing endpoint, not stored as a column (target content is mutable; freezing it would diverge from reality).Bug fix folded in
Atomic agent claim: the per-row claim uses a conditional UPDATE so concurrent calls can't double-dispatch.
Test plan