fix(gjc): keep persisted goal inspection read-only - #43
Open
snowykr wants to merge 2 commits into
Open
Conversation
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
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
marked this pull request as ready for review
September 6, 2026 12:50
snowykr
marked this pull request as draft
September 6, 2026 12:54
snowykr
marked this pull request as ready for review
September 6, 2026 13:07
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
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 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 singlegetBranchcapability it uses. No dependencies, execution/resume behavior, UI, wire schemas, or CLI identity guards change.listForResumePickerReadOnly, notlist()with orphan-backup recovery.captureTranscriptStrict; validate an immutable in-memory copy with the SDK's strict inspector and compare session ID/SHA-256.parseSessionEntriesfunction. Follow the last entry's parent chain, matching SDKgetBranch(), before using the unchanged goal/owner projection.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:
RED proof:
Final GREEN: all 9 focused cases pass. Independent read-only review: APPROVE, both hydration-related findings resolved.
Verification
Final code passed:
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):
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 verifyis not fully green locally. Client Node phase: 398 passed / 12 failed, all caused by:The same
src/shared/view/syntaxHighlighter.test.tsxfailure was reproduced against an archived, unchanged upstream/mainaedb95c, 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
@snowykrhas 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.
npm run verifypasses, 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
oneDarkimporterror documented in Verification above. The same error was reproduced on
unchanged upstream
main; the focused tests and separately completed gates arelisted with their actual results.