Skip to content

fix(gjc): keep persisted goal inspection read-only - #43

Open
snowykr wants to merge 2 commits into
devswha:mainfrom
snowykr:fix/goal-inspection-read-only
Open

fix(gjc): keep persisted goal inspection read-only#43
snowykr wants to merge 2 commits into
devswha:mainfrom
snowykr:fix/goal-inspection-read-only

Conversation

@snowykr

@snowykr snowykr commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

What this changes

Fixes #42.

Keep persisted Goal inspection entirely off the SDK manager's writable lifecycle. The final product change is limited to inspectGjcGoal() and narrowing the existing goal reader's input type to the single getBranch capability it uses. No dependencies, execution/resume behavior, UI, wire schemas, or CLI identity guards change.

  • Use listForResumePickerReadOnly, not list() with orphan-backup recovery.
  • Capture the selected source with captureTranscriptStrict; validate an immutable in-memory copy with the SDK's strict inspector and compare session ID/SHA-256.
  • Apply JSONL/header/entry-patch semantics with the existing SDK parseSessionEntries function. Follow the last entry's parent chain, matching SDK getBranch(), before using the unchanged goal/owner projection.
  • Preserve cwd/ownership checks, revalidate the original source before returning, and close the snapshot.
  • Never construct/hydrate a SessionManager for inspection. No source transcript, sidecar, or shared image-blob write can be triggered by that lifecycle.

Four existing files changed: adapter, a one-line goal-reader type narrowing, regression tests, and one protocol documentation row. Branch is based directly on upstream main (aedb95c) and does not include scrolling PR #41.

Why

The original Goal get called SessionManager.open(path, stringSessionRoot), which persisted assistant replay sanitation and invalidated an externally active managed CLI writer's cached identity.

Review and additional RED/GREEN tests found that merely changing to the pinned SDK's advertised recovery/memory-only hydration is insufficient: it can still clean/rebuild source sidecars, and even with MemorySessionStorage it can rewrite shared image blobs through a separate BlobStore. The final implementation removes hydration entirely rather than hiding errors or weakening integrity checks. SDK parsing/validation stays reused; only the short cycle-bounded parent-chain projection is explicit.

The immutable copy uses the SDK's existing materialization budget. Malformed, missing, ambiguous, identity-changed or digest-mismatched sources fail closed, with no writable fallback.

Regression proof

9 focused cases using temporary real SDK transcripts and the actual adapter, without model transport:

  • no goal on current branch, including an abandoned branch containing another goal/owner;
  • active, paused and complete persisted goals;
  • exact transcript bytes, replay payload/signature, inode, size, mtime and ctime preserved;
  • the original managed writer's next append remains durable;
  • foreign-owner/cwd behavior retained; no provider session constructed;
  • compacted managed and explicit sessions preserve sidecar contents/identity/directory inventory on successful and rejected queries;
  • image-bearing sessions never call the filesystem BlobStore write path;
  • orphan backups and missing targets untouched;
  • malformed and duplicate-ID transcripts rejected without mutation.

RED proof:

  • Original implementation: 5/6 initial cases failed (four replay sanitation writes plus backup recovery).
  • Intermediate recovery-hydration approach: both compacted sidecar tests failed, and the image-write trap failed even after isolating SessionStorage.

Final GREEN: all 9 focused cases pass. Independent read-only review: APPROVE, both hydration-related findings resolved.

Verification

Final code passed:

  • SDK contract + Goal SDK suites: 100 passed, 1 existing skip across two files.
  • npm run typecheck.
  • npm run lint && npm run check:identity && npm run build.
  • npm run test:e2e:gjc: 8 passed (existing driver-level and wire suites).
  • git diff --check; pre-commit lint and commitlint.

Also run earlier in this PR (the relevant unchanged gates):

  • Goal hook/component DOM tests: 17 passed, existing i18next initialization warning.
  • npm run verify: audit, licenses, notices, typecheck, Rust checks, all server Node tests (771 passed, 2 existing skips) and all server Bun files passed, then stopped at the baseline frontend failure below. Final changed SDK/Goal suites and static/build gates were rerun after review corrections as listed above.

Existing full-gate blocker — not suppressed

npm run verify is not fully green locally. Client Node phase: 398 passed / 12 failed, all caused by:

src/shared/view/syntaxHighlighter.ts:69
SyntaxError: The requested module
'react-syntax-highlighter/dist/esm/styles/prism'
does not provide an export named 'oneDark'

The same src/shared/view/syntaxHighlighter.test.tsx failure was reproduced against an archived, unchanged upstream/main aedb95c, using Node 22.22.2 and the same installed dependency tree. No PR source was in that baseline directory. The aggregate runner did not reach its later client-Bun/scripts phases; relevant Goal DOM tests and lint/identity/build gates were run separately.

This unrelated frontend import issue is deliberately excluded from the minimal Goal fix. No checks or warnings were suppressed. No real user transcript was modified or used as a mutable fixture; no manual GUI or desktop-packaging claim is made.

Contribution status

The contributor guide, MIT license, CLA and PR template have now been reviewed.
The PR was opened before that review was complete; this update corrects the
omitted template/checklist rather than claiming it was followed beforehand.

CLA signature pending: no existing signature for @snowykr has been verified.
The contributor must personally sign under the instructions in
CLA.md, which includes
copyright and patent grants and permission for proprietary relicensing. No
signature or legal acceptance has been submitted on the contributor's behalf.
The code is ready for technical review. CLA signature remains a separate
contribution requirement to resolve before merge; the CLA checkbox is intentionally unchecked.


  • I have signed the Contributor License Agreement, or I am the project owner.
  • npm run verify passes, or I have said below which gate fails and why.

The checked verification item uses the disclosure alternative, not a claim that
all tests pass: the client Node phase fails on the baseline oneDark import
error documented in Verification above. The same error was reproduced on
unchanged upstream main; the focused tests and separately completed gates are
listed with their actual results.

Use the pinned SDK's read-only inventory and unpromoted memory hydration
instead of writable resume. Preserve external CLI writer identity and avoid
orphan-backup recovery during Goal queries.

Add real managed-writer regression coverage and document the contract.
Full verify hits an existing frontend oneDark export failure, reproduced
on unchanged upstream/main. Server suites, typecheck, Rust checks, lint,
identity and build pass.

Fixes devswha#42
@snowykr
snowykr marked this pull request as draft September 6, 2026 12:38
The pinned SDK's recovery hydration still cleans sidecars and rewrites
shared image blobs, even with a memory-backed transcript. Avoid manager
construction entirely: capture and validate immutable bytes, apply the
SDK parser, and project only the current leaf's parent chain.

Keep original identity and digest checks, final source revalidation, and
existing goal ownership semantics. Add compacted managed/explicit sidecar
and image-store regressions; all nine focused cases pass. SDK and goal
suites pass 100 tests with one existing skip; typecheck, lint, build and
eight GJC e2e tests also pass.
@snowykr
snowykr marked this pull request as ready for review September 6, 2026 12:50
@snowykr
snowykr marked this pull request as draft September 6, 2026 12:54
@snowykr
snowykr marked this pull request as ready for review September 6, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Goal inspection mutates live CLI transcripts and causes managed_append_identity_mismatch

1 participant