feat(core,storage,cli): evidence pointers and staleness flags for foreign session handoff - #1512
Conversation
|
Reviewed at P1: A normal Claude Edit or Write is reported as The digest uses the assistant P1: An untrusted transcript timestamp can produce Any finite or parseable timestamp is accepted. A timestamp in the year 9999 suppresses every current file mtime warning and renders a clean result. Transcript time cannot be the authority for a Maka verification result without a trusted upper bound. P1: The probe can leave the repository and perform unbounded reads. Absolute paths, P2: Probe failures can still render as verified clean. Permission errors and unreadable Git metadata are folded into missing or unknown values. A result with nothing verifiable can therefore have no stale flags and render as clean. Keep an explicit unknown/error state and never emit a verified clean result when a required probe did not complete. P2: Claude branch comparison uses the first branch in the session, not the branch at the stop point. A session that checks out another branch can be reported as |
…eign session handoff Follow-up to apache#1057 (parts 1+2 merged as apache#1208/apache#1221), implementing the compatible half of ofekron's post-merge design feedback: the handoff should carry claims with evidence pointers and confidence flags, not just a summary. - ForeignSessionDigest gains `transcriptPath` (the evidence pointer for the whole digest) and `filesTouched` entries carry `lastEventAtMs` — the source event's own timestamp. Timestamps were chosen over transcript line offsets deliberately: large transcripts are read as a bounded TAIL window, where line numbers would be window-relative and silently wrong. - New handoff-time staleness check: @maka/storage probes the CURRENT repo state (cwd existence, branch via .git/HEAD — worktree gitdir files followed one level, no git spawn — and per-touched-file mtimes), and pure core assessment turns mismatches into typed flags (cwd_missing / branch_changed / files_changed / files_missing). Per-file mtimes compare against each touch's own event timestamp, falling back to the session clock. - The report renders as a Maka-authored <repo-state-check> block between the instruction and the untrusted envelope. An empty flag list renders an explicit all-clear (a receiving agent must be able to tell "checked and clean" from "never checked"), and stripEnvelopeTags now covers the new tag so a hostile path cannot forge maka-verified content. Probe failure degrades to the unchecked handoff shape. - Deliberately NOT implemented from the feedback: carrying "checks run and observed results" forward — tool outputs stay excluded per the apache#1057 safety contract; the staleness flags provide the confidence signal without trusting the transcript's account of the world. Tests: core 1183/1183 (evidence semantics, every flag kind, all-clear rendering, envelope-forgery defense, handoff composition), storage 530/530 (probe on real dirs: branch, worktree gitdir, detached HEAD, missing cwd), cli 652/652 (handoff carries the flagged report end-to-end through the picker import).
…e transcripts Review follow-up for apache#1512 (3 P1 + 2 P2): - P1 completion boundary: file touches now anchor to the matching tool_result record (paired by tool_use_id, bounded pending map), not the tool_use record — the write lands between the two, so the old anchor misreported every normal Edit/Write as changed-after-session. Interrupted calls keep the conservative session-end fallback. - P1 trusted ceiling: the probe stats the source transcript itself and the assessment clamps every transcript-claimed timestamp to min(transcript mtime, probedAt). A forged year-9999 anchor clamps down and cannot suppress mtime warnings; with no ceiling available, mtime comparisons are declared unverified instead of silently passing. - P1 confinement + bounded reads: touched files are realpath-confined to the realpath of the session cwd (absolute escapes, ../, and symlinks out are reported out_of_scope and never followed) and are only ever stat'd. Git metadata reads (.git pointer, HEAD) are lstat-gated to regular files ≤4KB and re-checked on the held fd before reading — the same TOCTOU discipline as readDigest. - P2 honest failure: every probe observation carries an explicit missing/unreadable/out_of_scope/detached state, the report gains an `unverified` list, and the render is tri-state — a probe failure can never fold into `result=clean`; "nothing verifiable" renders partial. - P2 branch at stop: the digest reports the LAST branch observation in the transcript window (the scanner summary keeps its first-observation semantic for listing). Tests: core 1248/1248 (forged-future-timestamp cannot buy clean, no-ceiling → partial, out-of-scope/unreadable → unverified never missing, detached/unreadable git states, tri-state rendering), storage 616/616 (absolute/dot-dot/symlink escapes all out_of_scope, oversized HEAD unreadable, worktree gitdir still bounded-read, completion-boundary anchoring, branch-at-stop vs scanner first-observation), cli 652/652.
de6054e to
4b9ee1e
Compare
|
@Astro-Han All five findings addressed in 4b9ee1e — each got the structural fix plus a test that fails on the reviewed head: P1 completion boundary — touches now anchor to the matching P1 trusted ceiling — the probe stats the source transcript itself; the assessment clamps every transcript-claimed timestamp (per-touch anchors AND the P1 confinement + bounded reads — touched files are realpath-confined to the realpath of the session cwd; absolute escapes, P2 honest failure — every observation now carries an explicit state ( P2 branch at stop — the digest reports the last branch observation in the transcript window (checkout-mid-session test: summary keeps its first-observation listing semantic, the digest used for comparison reports the stop point). core 1248/1248, storage 616/616, cli 652/652 locally; branch rebased onto current main. |
|
Rechecked
The current failure is in this PR diff, so I am not treating it as stale infrastructure. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at 4b9ee1ee. The earlier P1 findings are fixed. The remaining containment-contract failure is P2, so it does not block code approval; the required test still needs to be green before merge.
The containment-guard contract (apache#1145) retires private `isInside` definitions — the probe's confinement check now uses the same `isInsideOrSamePath` recipe as artifact-store.ts and session-metadata-maintenance.ts, the strict-interior family the contract allows in packages that cannot import @maka/runtime's isPathInside (runtime depends on storage, not the reverse). containment-guard contract 2/2; storage 616/616.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approving current head 83fba1e with non-blocking P2 notes.
- The containment helper can treat a Windows cross-volume result as in-scope and can classify an out-of-scope missing path as trusted
missing. - A follow-up should reject absolute
relative()results, distinguish the real parent segment from names such as..rules, and fail closed before reporting missing paths.
Given the current Windows support scope and metadata-only impact, these do not block this PR.
|
/agentic_review |
Code Review by Qodo
1. Nested repo branch missed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| async function readGitState(realCwd: string): Promise<ForeignSessionRepoProbe['gitState']> { | ||
| const gitPath = join(realCwd, '.git'); | ||
| let gitDir = gitPath; |
There was a problem hiding this comment.
1. Nested repo branch missed 🐞 Bug ≡ Correctness
Disposition: fix-now. readGitState checks only <session cwd>/.git, so sessions run from a repository subdirectory are reported as not_a_repo and a real branch change is omitted from the handoff.
Agent Prompt
## Issue description
Branch probing only checks `.git` in the exact session cwd, so a valid cwd below the repository root skips branch-change detection.
## Issue Context
`cwd` is the session working directory, not guaranteed to be the repository root. Reusing the existing bounded `readGitState` metadata reader is preferable; deletion or consolidation cannot satisfy repository discovery because no repository root is currently available in the digest.
## Fix Focus Areas
- packages/storage/src/foreign-session-store.ts[506-520]
- packages/storage/src/foreign-session-store.ts[606-629]
- packages/storage/src/__tests__/foreign-session-store.test.ts[633-714]
Walk upward from the canonical cwd to the nearest `.git` entry, while retaining bounded regular-file reads and explicit failure states. Add a regression test with cwd in a nested repository directory and a changed HEAD branch; this adds only the required ancestor-search branch and its test burden, without introducing public state or configuration.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (use.toolUseId !== undefined && pendingToolFiles.size < PENDING_TOOL_FILES_MAX) { | ||
| pendingToolFiles.set(use.toolUseId, use.paths); | ||
| } |
There was a problem hiding this comment.
2. Duplicate tool ids misstamp 🐞 Bug ≡ Correctness
Disposition: fix-now. pendingToolFiles stores one path list per Claude tool-use ID, so a duplicate ID overwrites the earlier operation—or retains the old one at the cap—and the next result timestamp is attached to the wrong files, corrupting the evidence pointer and staleness comparison.
Agent Prompt
## Issue description
Duplicate untrusted Claude tool-use IDs make result timestamps attach to the wrong touched-file claim, especially when the pending map is full.
## Issue Context
The closest existing seam is `pendingToolFiles`; preserve one authority rather than introducing another map. Deletion is insufficient because valid unique tool IDs still need completion timestamps.
## Fix Focus Areas
- packages/storage/src/foreign-session-store.ts[400-407]
- packages/storage/src/foreign-session-store.ts[433-452]
- packages/storage/src/__tests__/foreign-session-store.test.ts[489-505]
Represent an ID already seen while pending as ambiguous in the existing map (for example, a sentinel value), and never timestamp either path set from a result for that ID. Ensure duplicate detection occurs before the capacity condition, and add duplicate-ID tests both below and at the cap; the sentinel adds one internal state and corresponding test burden but avoids a second authority.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| function toIsoOrUnknown(ms: number): string { | ||
| return Number.isFinite(ms) ? new Date(ms).toISOString() : 'unknown'; |
There was a problem hiding this comment.
3. Invalid timestamps crash handoff 🐞 Bug ☼ Reliability
[fix-now] A transcript tool_result timestamp such as 1e100 is accepted as finite and stored in lastEventAtMs, but new Date(1e100).toISOString() throws a RangeError while rendering the digest. This makes the foreign-session import fail instead of producing the intended best-effort handoff for a malformed/untrusted timestamp.
Agent Prompt
## Issue description
`toIsoOrUnknown` treats every finite number as ISO-renderable, but JavaScript Dates outside the valid time range throw from `toISOString()`. A numeric timestamp from an untrusted transcript can therefore abort the handoff.
## Issue Context
`foreignRecordTimestampMs` accepts finite numeric `timestamp` values and the new per-file rendering calls `toIsoOrUnknown` for `lastEventAtMs`.
## Fix Focus Areas
- packages/core/src/foreign-session.ts[885-887]
- packages/core/src/foreign-session.ts[915-920]
Make the existing local formatter return `unknown` unless the constructed Date has a finite time value; no new public surface or state is needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| try { | ||
| const realParent = await realpath(dirname(candidate)); | ||
| return isInsideOrSamePath(realCwd, join(realParent, basename(candidate))) | ||
| ? { status: 'missing' } | ||
| : { status: 'out_of_scope' }; | ||
| } catch { | ||
| return { status: 'missing' }; |
There was a problem hiding this comment.
4. Escaped paths flagged missing 🐞 Bug ≡ Correctness
[fix-now] When a transcript-controlled touched path such as ../nonexistent-parent/x escapes the repository and both the candidate and its outside parent are absent, probeTouchedFile unconditionally converts the parent realpath() failure to missing without containment validation. The assessment then emits a Maka-verified files_missing flag instead of the required unverified/out-of-scope result, falsely asserting a repository-state mismatch.
Agent Prompt
## Issue description
A transcript-controlled path that lexically escapes the repository is classified as `missing` when its nonexistent parent cannot be resolved with `realpath()`, bypassing the probe's out-of-scope classification contract. Fix the unresolved-parent fallback so it distinguishes an in-scope missing leaf from an unresolved lexical escape and does not produce a false Maka-verified `files_missing` result.
## Issue Context
For a repository at `/tmp/repo` and a touched path such as `../not-created/file.ts` or `../nonexistent-parent/x`, the normalized candidate lies outside `/tmp/repo`, while both the leaf and its parent fail resolution with `ENOENT`; the current catch unconditionally returns `missing`. Add the smallest local lexical containment check for unresolved paths while keeping realpath-based containment authoritative for existing paths and symlink escapes, and classify non-not-found parent errors as unreadable. No new public state, configuration, or API is needed; deletion or consolidation alone cannot distinguish an in-scope missing leaf from an unresolved escape.
## Fix Focus Areas
- packages/storage/src/foreign-session-store.ts[533-560]
- packages/storage/src/foreign-session-store.ts[568-578]
- packages/storage/src/__tests__/foreign-session-store.test.ts[669-687]
Before returning `missing` from the unresolved-parent fallback, verify that the normalized candidate is lexically inside the canonical cwd and return `out_of_scope` for escapes. Add a regression case for `../nonexistent-parent/x` while retaining existing realpath containment behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2 participants
Follow-up to #1057 (parts 1+2 shipped as #1208/#1221), implementing the compatible half of @ofekron's post-merge design feedback (comment): handoffs should carry claims with evidence pointers and confidence flags, not just a summary. Closes #1057 — Cursor support and desktop integration are non-blocking per the original scope and can get their own issues.
What the handoff gains
Evidence pointers.
ForeignSessionDigestnow names itstranscriptPath(rendered assource_transcript=in the envelope), and everyfilesTouchedentry carrieslastEventAtMs— the source event's own timestamp, rendered as(last touched …). Timestamps over transcript line offsets was a deliberate call: large transcripts are read as a bounded tail window, where line numbers would be window-relative and silently wrong; the event clock survives windowing and is also exactly what the staleness check needs.Maka-verified staleness flags. At import time the CLI probes the CURRENT repository (read-only, never throws):
.git/HEAD— no git spawn; worktree/submodulegitdir:pointer files followed one level;A pure core assessment turns mismatches into typed flags —
cwd_missing/branch_changed/files_changed/files_missing— rendered as a Maka-authored<repo-state-check maka-verified="true">block between the instruction and the untrusted envelope. Two properties worth review attention:result=clean …) — a receiving agent must be able to tell "checked and clean" from "never checked";stripEnvelopeTagsnow covers the new tag — a hostile file path could otherwise close the block and forge maka-verified content (test:cannot be closed early by a hostile path in a flag detail).Probe failure degrades the handoff to the previous unchecked shape rather than blocking the import.
Deliberate divergence from the feedback
"Checks actually run and observed results" is not carried forward: tool outputs stay excluded per the #1057 safety contract (untrusted transcript, stale evidence by definition). The staleness flags give the receiving agent a confidence signal computed by Maka itself, without trusting the transcript's account of the world. Argued on the issue thread; ofekron's other points are all in.
Verification
gitdir:file, detached HEAD, missing cwd; digest carriestranscriptPath+ per-touch timestamps end-to-endfiles_missing)