Skip to content

fix(parser): use transcript identity for rooted uploads - #1435

Open
rodboev wants to merge 1 commit into
kenn-io:mainfrom
rodboev:pr/1333-upload-identity
Open

fix(parser): use transcript identity for rooted uploads#1435
rodboev wants to merge 1 commit into
kenn-io:mainfrom
rodboev:pr/1333-upload-identity

Conversation

@rodboev

@rodboev rodboev commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Renamed Claude root uploads can collide with an existing archive because the multipart filename supplies session identity. Uploads whose transcript carries one stable archive-safe ID with explicit root markers now select that ID during the existing Claude parse, and the staged file follows it. Sidechain, markerless, mixed-ID, and local discovery parses keep their current identity rules.

Replacement semantics, existing filename-keyed rows, case-equivalent names, and concurrent-writer handling are unchanged.

Refs #1333

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (3aca787)

High-severity identity-selection bugs can overwrite parent sessions; markerless root reuploads are also non-idempotent.

High

  • internal/parser/claude.go:108 — Any compact boundary overrides ordinary sidechain evidence. A compacted child transcript containing normal isSidechain:true messages is treated as a root, allowing its parent ID to replace the parent session’s database row and transcript file.

    • Fix: Ignore the compact boundary’s own sidechain flag, but let any non-boundary sidechain message classify the result as a child.
  • internal/parser/claude.go:81 — Root identity selection scans every parse result. When the main result is correctly recognized as a legacy child but the transcript contains a DAG fork, the RelFork result is not considered a child and supplies the shared parent ID as the upload identity, overwriting the parent session.

    • Fix: Derive upload identity from the main result only, or propagate its child classification to fork results before identity selection.

Medium

  • internal/server/huma_routes_sessions.go:1103 — Archive presence alone reclassifies markerless renamed roots as children. The first upload stores the embedded ID, while a repeat upload creates a basename-keyed child, making uploads non-idempotent.
    • Fix: Do not let archive existence alone override root identity; use stronger transcript or upload-provenance evidence for root reuploads.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 5m4s

@rodboev
rodboev force-pushed the pr/1333-upload-identity branch from 3aca787 to 6112f91 Compare August 16, 2026 13:40
@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (6112f91)

Code review found three medium-severity issues involving fork semantics, ID collisions, and cross-project parent detection.

Medium

  • internal/parser/claude.go:270 — Sidechain fork results are overwritten from RelFork to RelSubagent, losing fork semantics. Preserve explicit fork relationships or restrict child normalization to the main result.

  • internal/parser/claude.go:319 — When the embedded ID equals a generated fork ID, remapping assigns the root and fork the same ID, allowing one branch to overwrite the other. Apply old-ID prefix remapping before the id == newID shortcut and validate result-ID uniqueness.

  • internal/server/huma_routes_sessions.go:1131 — Canonical-parent detection checks only the current project directory, so re-uploading a markerless root under another project incorrectly creates a child session. Recognize canonical upload paths across projects using the stored parent project and path.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 5m14s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (46fcd95)

Verdict: One medium-severity issue found in Claude upload lineage detection.

Medium

  • internal/parser/claude.go:219 — Child detection examines the first retained ParsedMessage, but parsing may discard the actual DAG root (such as an isMeta or empty root entry). The next retained message can have a SourceParentUUID, causing a renamed root upload to be incorrectly stored as a subagent under its transport filename.
    • Fix: Derive head-parent evidence from the first raw user/assistant transcript entry before filtering, and test a renamed root whose root DAG entry is filtered out.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 2m48s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (9a3909d)

Code review found two medium-severity identity collision risks in Claude session uploads.

Medium

  • internal/parser/claude.go:268 — A markerless child whose parent ID starts with agent- is classified as a root based only on that prefix, potentially overwriting the parent session. Do not treat the source-ID prefix as root evidence; use the existing parent-provenance check.

  • internal/server/huma_routes_sessions.go:1094 — An explicit sidechain upload whose embedded parent ID matches its filename can produce an empty parsed parent ID, bypass collision checks, and overwrite an existing root with that ID. When child evidence exists but ParentSessionID is empty, reject an existing session-ID collision or require a distinct child filename.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m0s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (1c5193e)

High-risk identity and collision issues remain in Claude session uploads.

High

  • internal/server/huma_routes_sessions.go:1167 — Confirmed markerless children can bypass collision checks. After mainChildConfirmed sets RelSubagent, ClaudeUploadResultIsChild returns false for the legacy-parent shape, allowing an unrelated session with the upload’s basename ID to be silently overwritten. Preserve the child decision before mutating lineage and include mainChildConfirmed in the collision condition.

Medium

  • internal/parser/claude.go:127 — A markerless DAG or compact root uploaded with an agent-* filename is misclassified as a child because InferRelationshipTypes uses the transport filename. The embedded root identity is ignored and incorrect lineage is persisted. Avoid filename-inferred relationship types for upload classification, or prioritize compact/raw-head root evidence.

  • internal/server/huma_routes_sessions.go:1166 — Re-uploading an unchanged root transcript containing forks conflicts after the first upload because every non-main result with an existing ID is rejected. Allow replacement when the existing result has the same canonical upload-file and project provenance.


Reviewers: 2 done | Synthesis: codex, 12s | Total: 5m41s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (92f57a6)

Code review found three medium-severity issues involving Claude session classification and reuploads.

Medium

  • internal/parser/claude.go:118 — A renamed root whose compact boundary contains parentUuid is misclassified as a child even with isSidechain:false, because the raw parent-edge check precedes root evidence. Prioritize explicit/compact root evidence after ordinary sidechain checks, or exclude compact boundaries from relationship evidence.

  • internal/server/huma_routes_sessions.go:1188 — Reuploading an existing child session always returns 409; the same-project/same-file replacement exception only applies to roots. Allow exact project-and-path matches for child sessions while preserving ownership collision checks.

  • internal/server/huma_routes_sessions.go:1160 — If a reupload gains a usable embedded identity, remapping its basename ID and destination creates a duplicate session while leaving the old row and file intact. Preserve the established identity or atomically migrate/remove the previous row and file.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 5m52s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (5925976)

Medium

  • internal/server/huma_routes_sessions.go:1173: Preserving an established basename ID skips identity remapping, leaving the parsed root’s synthetic ParentSessionID == sourceID. Normalization then treats the root as a continuation. Explicitly clear the synthetic parent and relationship when preserving the established ID, and add a re-upload test confirming root lineage remains unchanged.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 6m58s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (1b9416d)

Potentially unsafe session-ID remapping introduces two medium-severity upload issues.

Medium

  • internal/server/huma_routes_sessions.go:1197 — Crafted JSONL metadata can set sessionId to an existing archive ID with isSidechain:false. Because collision checks are skipped for remapped roots, an innocuously named upload can overwrite an unrelated session, including messages, project association, and transcript. Require collision checks for remapped roots, allowing replacement only when canonical ID and ownership match or overwrite is explicitly confirmed.

  • internal/server/huma_routes_sessions.go:1095 — Parent exclusion checks occur before embedded IDs are remapped. A fork initially referencing the transport-derived root ID can be rejected due to an unrelated excluded or trashed session matching the upload filename, even though its final parent differs. Remap identities before validating parent state, or defer validation of provisional in-batch parent edges until final IDs are known.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 6m19s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (14f3686)

The change needs fixes for two medium-severity session identity and concurrency issues.

Medium

  • internal/server/huma_routes_sessions.go:1154 — Legacy-child disambiguation ignores an existing exact-path session. Reuploading an established markerless child can conflict when its parent is canonical or be reclassified as a root when the parent is absent. Consult the existing pathID row first and preserve its stored lineage when project and file path match.

  • internal/server/huma_routes_sessions.go:1244 — Collision checks occur outside the atomic write. Concurrent cross-project uploads with the same embedded ID can both pass; the second upsert can overwrite the first row and leave an orphaned file. Enforce collision and parent-state preconditions inside the database transaction.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 7m52s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (4c33469)

The upload safeguards have three medium-severity gaps that could permit incorrect transcript replacement.

Medium

  • internal/server/huma_routes_sessions.go:1256internalError returns nil for context.Canceled, causing validateUploadState to report success. The upload may commit without collision validation and overwrite an unrelated active session. Propagate cancellation from the validator and suppress it only after the transaction aborts.

  • internal/server/huma_routes_sessions.go:1328 — Locked validation does not repeat the legacy-parent provenance decisions from lines 1131–1206. If a canonical parent appears between those checks and the callback, sameFileReplacement may let an ambiguous markerless upload overwrite it. Revalidate parent/path provenance under the write lock or perform identity classification there.

  • internal/server/huma_routes_sessions.go:1241 — Destination ownership compares case-sensitive session IDs and cleaned paths. On case-insensitive filesystems, IDs such as Foo and foo resolve to the same file, allowing one upload to replace another’s transcript while both database rows remain. Detect platform-equivalent paths or existing destination files and reject replacements not owned by the authorized session.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 7m3s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (20f2d23)

Medium-severity correctness and concurrency issues remain despite no identified security regression.

Medium

  • internal/server/huma_routes_sessions.go:1409 — Validation reruns ClaudeUploadResultIsChild after a confirmed legacy child is changed to RelSubagent. Because session-ID-transition evidence is only considered for RelContinuation, a valid child of a canonically uploaded parent becomes ambiguous and is rejected. Preserve mainChildConfirmed or cached child evidence instead of rerunning the mutable classifier.

  • internal/db/sessions.go:1152GetSessionFullCaseInsensitive retains a reader connection while acquiring another. Four concurrent Windows lookups can exhaust the four-connection reader pool and deadlock. Save the matching ID, close the rows, and then perform the full-session query.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 6m58s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (8ade195)

Medium-severity lineage bug found; no critical or high-severity issues identified.

Medium

  • internal/parser/claude.go:325 — Normalization clears lineage for archive-confirmed children whose parent ID is not Windows-filename-safe, such as an existing Unix session named CON. The upload route confirms the child, but normalization reclassifies it as a root. Pass confirmed-child evidence into normalization and preserve archive-confirmed parent relationships.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 6m43s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (f401a4c)

The PR has two medium-severity correctness issues; no critical, high, or security findings were identified.

Medium

  • internal/parser/claude.go:118 — Legacy child heuristics run before explicit isSidechain:false root evidence, causing resumed roots with a head parentUuid or later session-ID transition to be misclassified as subagents. Check claudeUploadHasRootEvidence first and add coverage for these cases.

  • internal/db/sessions.go:1153 — Case-insensitive lookup returns the first unordered match. Since IDs may differ only by case, uploads can nondeterministically resolve or overwrite the wrong session. Detect and reject ambiguous case-equivalent matches; on Windows, ensure the exact-ID fast path does not bypass this check.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 10m51s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (e1b9e5d)

Medium-severity issue found in case-insensitive filesystem handling; no security vulnerabilities identified.

Medium

  • internal/server/huma_routes_sessions.go:1114 — Case-equivalent project and session lookup is Windows-only. On case-insensitive macOS volumes, reuploading Case.jsonl as case.jsonl finds the existing file via os.SameFile but not its database row, causing a false ownership conflict. Resolve ownership using actual filesystem path equivalence rather than runtime.GOOS, while preserving distinct IDs on case-sensitive volumes.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 7m41s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (380b971)

The PR has one medium-severity correctness issue; no security regressions were identified.

Medium

  • internal/server/huma_routes_sessions.go:1177 — Filesystem-equivalent lookup cannot identify a child transcript’s main session when that file also owns fork rows. Because every row has a parent, a valid upload is rejected as ambiguous.
    • Fix: Select the unique top-level row whose parent is outside the matched set, and add an alias-reupload test covering a child session with forks.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 8m13s

@roborev-ci

roborev-ci Bot commented Aug 16, 2026

Copy link
Copy Markdown

roborev: Combined Review (53945eb)

Changes requested: two medium-severity reliability and filesystem-detection issues remain.

Medium

  • internal/db/session_batch.go:151 — The upload file is committed before database rows are written. A crash during batch writes can replace the file while SQLite rolls back, permanently desynchronizing the archive. Separate pre-write validation from file commit, keeping commitUpload immediately before the SQL commit.

  • internal/server/huma_routes_sessions.go:1114 — Case-insensitivity detection can produce false positives on case-sensitive filesystems when unrelated case-variant entries are symlinks or hardlinks to the same inode. This may conflate session IDs or assign an incorrect parent. Probe with a unique sentinel in the upload directory and account for case-sensitive Windows directories.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 9m59s

@roborev-ci

roborev-ci Bot commented Aug 17, 2026

Copy link
Copy Markdown

roborev: Combined Review (d73cc4e)

High-severity crash-recovery flaw could leave uploaded files inconsistent with database state.

High

  • internal/server/upload.go:425 — Upload journals usually omit FileHash, so recovery may mistake pre-existing rows with the same ID/path for a committed transaction. If a crash occurs after file replacement but before SQL commit, recovery can delete the backup, leaving new file contents paired with old database messages.
    • Fix: Persist and verify a transaction-unique commit marker or complete write fingerprint. Add a replacement-upload crash test covering rollback before SQL commit.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 7m2s

@roborev-ci

roborev-ci Bot commented Aug 17, 2026

Copy link
Copy Markdown

roborev: Combined Review (265a33b)

Medium-severity issues remain in the upload commit and session batch flows.

Medium

  • internal/server/huma_routes_sessions.go:1808commitUpload can fail after moving the old or new file without rolling back, leaving the database and upload path inconsistent until restart. Roll back every mutated commit state, including the backup-only state after the first rename.

  • internal/db/session_batch.go:136WriteSessionBatchAtomic now invokes beforeCommit before any SQL writes, so failed batches can trigger side effects that previously occurred only after successful row writes. Preserve the existing callback timing and use WriteSessionBatchAtomicWithCommit for separate pre-write validation.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 7m31s

@roborev-ci

roborev-ci Bot commented Aug 17, 2026

Copy link
Copy Markdown

roborev: Combined Review (4a79738)

Changes need fixes for two medium-severity recovery and lineage issues.

Medium

  • internal/server/upload.go:380 — Recovery rejects the old final file if a crash occurs after journal creation but before the backup rename, leaving the journal permanently unrecoverable. Record journal phases or the previous-file hash, recover each rename state explicitly, and add crash-point tests.

  • internal/parser/claude.go:337 — Archive-confirmed continuation relationships are always rewritten as subagent, altering lineage and sidebar visibility after re-upload. Preserve established relationship types and synthesize subagent only for ambiguous or untyped children.


Reviewers: 2 done | Synthesis: codex, 5s | Total: 7m33s

@roborev-ci

roborev-ci Bot commented Aug 17, 2026

Copy link
Copy Markdown

roborev: Combined Review (61aff97)

Changes need revision: crash recovery can lose the canonical file during unchanged reuploads.

Medium

  • internal/server/upload.go:421 — If a process crashes after moving the original file to backup but before installing the staged file, recovery may mistake the transaction for committed because the existing database row already matches the journal fingerprint. This leaves the canonical file missing. Detect the backup-only phase from backup/staged-file state or a durable phase marker, restore the backup before checking commit fingerprints, and add coverage for a preexisting matching database row.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 7m19s

@roborev-ci

roborev-ci Bot commented Aug 17, 2026

Copy link
Copy Markdown

roborev: Combined Review (27d66bd)

Medium — Recovery can misclassify a pre-rename upload as committed.

  • internal/server/upload.go:427: Preexisting matching database rows may be mistaken for the committed upload when no prior destination existed and a crash happened before the temporary-file rename. Recovery then reports a missing final file and leaves the journal and temporary file indefinitely.
  • Fix: Before checking database fingerprints, detect a still-present TempPath matching ContentHash with no final file, and recover it as a pre-rename transaction.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m41s

@rodboev
rodboev marked this pull request as draft August 17, 2026 04:11
@rodboev
rodboev force-pushed the pr/1333-upload-identity branch from 27d66bd to 3ccb7d5 Compare August 17, 2026 06:53
@rodboev rodboev changed the title fix(server): derive uploaded session identity from transcript fix(parser): use transcript identity for rooted uploads Aug 17, 2026
@roborev-ci

roborev-ci Bot commented Aug 17, 2026

Copy link
Copy Markdown

roborev: Combined Review (3ccb7d5)

Code review passed with no findings at Medium severity or above.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 5m3s

@rodboev

rodboev commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

The underlying fix for #1333 genuinely crosses three authorities:

  • internal/parser owns transcript identity and lineage.
  • Upload handling owns the destination path, conflict response, and replacement policy.
  • internal/db owns race-free persistence.

After the repeated bounces trying to work all this into one PR I reworked it to just the identity-collision authority. This establishes the canonical identity input:

  • Parser selects a trustworthy explicit-root ID.
  • Upload handling uses that ID as the destination.
  • Existing DB persistence is reused.

This is a better design and turned out smaller. Once this foundation merges, the next PR would cover the destructive behavior described in the original issue without reopening identity or filesystem design. Once both merge, the reported data-loss bug should be fixed such that we can close out the original issue. There are other possible follow-ups in:

  • existing archive audit/repair (mentioned in the original issue)
  • ambiguous identity/namespace policy (surfaced during review of the larger fix I tried at first)

But their value needs to be evaluated once the two-part fix here is in, since there may not be a need for them.

@rodboev
rodboev marked this pull request as ready for review August 17, 2026 07:52
@rodboev

rodboev commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

The second of this two-part fix is tracked as PR #1443. If that merges, this can be closed; if fixes are needed here, that one gets rebased. The pair closes #1333.

wesm pushed a commit that referenced this pull request Aug 19, 2026
Session uploads currently replace an existing transcript wholesale, so a shorter re-upload can reduce a stored session from 96 messages to 24 with a successful response and no caller consent.

This makes shorter replacements return `409 Conflict` by default and requires the caller to pass `allow_shorter=true` for an intentional rewrite. If any member of a multi-session upload would shrink, the whole batch rolls back and the destination file does not move.

The guard compares message counts inside the existing atomic batch transaction, keeping the decision and replacement under the same lock.

This is the destructive-replacement slice of #1333. It stacks on #1435, which establishes transcript identity and lineage.

Closes #1333

Co-authored-by: Rod Boev <rodboev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant