Repository navigation
feat(settings): add readonly target scope snapshots - #6281
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
probepark
left a comment
There was a problem hiding this comment.
Review (head a6ab76a, gajae-reviewer on behalf of probepark)
CI: PR-caused, 1 — Affected path validation / plan failed at step Verify PR head contains exact base: Exact-head CI requires this PR head to contain base 100502f8f96727953856b9f57597a2d6a96f6c6d; rebase onto current dev. (job 111225477839). evidence producer and the aggregate fail because of it, and every affected shard is skipped. So check:@gajae-code/coding-agent (biome + check:types) and the new settings-session-memory tests have not run in exact-head CI at all. The body's Exact-prefix verification section is local-only evidence. gjc-state-gates / native addon is still pending.
Scope: +233 / -12, 9 files — packages/coding-agent/src/{config/settings.ts, capability/{index,types}.ts, discovery/helpers.ts}, test, changelog fragment. The three-dot range still carries the #6277 native layer (crates/pi-natives/src/path_identity.rs, packages/natives/...) because the merge-base is 2a97f5f. #6277 merged at 14:51:34Z, so a rebase onto current dev (8de425e) should leave only this PR's own 6 files (+224/-8 vs b7fafa1).
Conventions: changelog fragment packages/coding-agent/changelog.d/task-stack-readonly-settings.md present (Added); no generated files edited in this layer; no labels.
Notable:
git merge-base --is-ancestor 100502f8 a6ab76a→ exit 1; merge-base2a97f5f. Base100502f8has parents2a97f5f+f549fdb, so the head and the base are siblings off2a97f5f. This is the blocker. It is structural, not a code defect: rebase ontodevand the exact-head plan will actually evaluate this head.config/settings.ts:1612-1617—#loadProjectSettingsnow always passesagentDir: this.#agentDirtoloadCapability, and that applies to the ordinary paths too (#load,cloneForCwd:1167), not just snapshots. When#agentDirdiffers fromgetAgentDir()(loadForScope({agentDir}), tests),capability/index.ts:417-422now derivesprofileAuthority: "custom"and resolves user-scope providers from that dir instead of the process agent dir. That is arguably the correct binding, but it is a behaviour change outside the "readonly snapshot" scope the body describes ("Ordinary non-bypass reads retain existing cache semantics" covers caching, not authority). Please call it out in the description or the changelog fragment. Non-blocking.config/settings.ts:1132—snapshotForCwd(sameCwd)returnssnapshot()without re-reading the project layer, so the "fresh operation-local metadata reads" in the changelog only hold for a different target cwd. Fine if intended; worth a doc-comment line. Non-blocking.
Checked and clean: snapshot() builds an inMemory instance (#persist=false, #configPath=null), so #normalizeAfterLoad(false) → flush() hits the early returns in #queueSave:5659 and #refreshDurableSettings:5880, with no writes or queue entry. publishHooks=false skips only #fireAllHooks (6037), and every other caller keeps the default true. bypassCache threads loadCapability → LoadContext → getReadOptions:212 → fs.ts:245/370 (useCache = !bypassCache && !isolatedHome). The isolated-home arm already forces bypassCache: true (helpers.ts:220), so nothing changes there. Non-ENOENT project read errors still propagate (:1645-1652).
Blocking: 1 (head does not contain base 100502f8; exact-head CI never ran the PR's checks/tests). No code defect found in the snapshot logic.
PR body verdict line count=0, not updated. Suggested verdict line: the one below; the rebased head gets a fresh review.
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:c3a05ff7239ee6038413faa0ecd03d7fb79b3f7f676a100b170166b399069cc4 reviewer:critic reviewer-id:gajae-reviewer evidence:head-not-containing-base-100502f8;plan-short-circuited;snapshot-inmemory-no-persist-checked;bypassCache-thread-checked
a6ab76a to
088f50e
Compare
Resolve normalized target project configuration with an operation-local cache bypass. Snapshot normalization must not publish global hooks or mutate the parent's retained configuration. Lore-id: 39c08d71 Constraint: no task authority or persistent owner activation Confidence: high Scope-risk: narrow Reversibility: easy Tested: joined settings discovery retirement codec union 417 passed 8 existing skips Tested: focused corrected union 196 passed and package vendor Biome types Not-tested: final task move integration in this prerequisite prefix
088f50e to
ce22d61
Compare
probepark
left a comment
There was a problem hiding this comment.
Review (head ce22d61, gajae-reviewer on behalf of probepark)
CI: green — Affected path validation aggregate pass (incl. check:@gajae-code/coding-agent, ts-build coding-agent, settings-session-memory.test.ts, discovery/helpers.test.ts), Virtual integration validation pass, gjc-state-gates pass. Approve gate: ALLOW (0 pending / 0 failed, check:@gajae-code/coding-agent covered).
Scope: +224 / -8, 6 files — packages/coding-agent/src/config/settings.ts, src/capability/{index,types}.ts, src/discovery/helpers.ts, test settings-session-memory.test.ts, changelog.d fragment.
Conventions: changelog.d fragment present (### Added), no generated files, no labels. Head now contains base 8de425ed (the stale-base blocker from my CHANGES_REQUESTED on a6ab76a is resolved).
Notable:
settings.ts:1130-1140snapshotForCwd— clone isinMemory(so#persist=false), and#normalizeAfterLoad(false)→flush()is a no-op (#queueSave/#refreshDurableSettingsboth return early on!#persist), with#fireAllHookssuppressed. Matches the "no persistence / no global hooks" claim; the test asserts unchanged config.yml / settings.json bytes and no tab-width hook. Note the same-cwd branch (L1132) returns a plainsnapshot()without#readonly=true; harmless since it is in-memory, just asymmetric.settings.ts:1612-1617— ordinary#loadProjectSettings()now also passesagentDir: this.#agentDirtoloadCapability, a small behavior change for every non-default-agentDir Settings (previously discovery used the default agent dir). Looks like a correctness fix, but it is not called out in the PR body or changelog.
Blocking: none
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:915008d692a20614fc5f6413ab269b906fcda7289ee7472e1f199232c1987c20 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;base-contained;snapshot-no-persist-no-hooks-checked;bypassCache-plumbing-checked
PR body verdict line count=0, not updated. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:915008d692a20614fc5f6413ab269b906fcda7289ee7472e1f199232c1987c20 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;base-contained;snapshot-no-persist-no-hooks-checked;bypassCache-plumbing-checked
|
Merged to dev: approved at ce22d61, checks green, no open concerns. |
Task admission stack — 02: readonly target settings snapshots
Depends on #6277 in the prepared branch stack. Base remains dev. Own layer: 65 production added+deleted lines; actual cumulative dev diff at observed merged-native parent
8de425ed…: 65 lines. Native #6277 was externally approved/merged at b7f; no native delta remains in this dev diff. No parent-relative-only size claim.Add functional
Settings.snapshot()/snapshotForCwd()helpers with normalized different-target settings, operation-local metadata cache bypass, no parent mutation or global normalize hooks. Same-cwd calls intentionally return retained execution settings without reloading. Ordinary non-bypass reads retain cache semantics, but project configuration discovery now explicitly uses the Settings instance'sagentDirrather than the process default: this also correctly binds ordinary loads/clones to custom profiles. This prefix does not activate new task authority or persistent owner production.Exact-prefix verification
Owned-only conflict-free integration onto actual approved native merge
8de425edbf16d8e0fa3ea82553929d8860168f4d, following separately disclosed unrelated AI salvage. Native duplicate not reapplied. Prior heads/proof archived; every listed prefix test/package gate freshly rerun on this exact current head.Head
ce22d610a0163ed671fd11f0d5e92cb56fc85f20separately checked in clean detached nonnested QA, with unchanged-native-source genuine addon bytes matching committed strict provenance; no trusted JSON rewritten:bun --cwd=packages/coding-agent run check: vendor, Biome and types passed.git diff --checkpassed.Existing warnings remain recorded; Darwin runtime only. No final task-move/owner feature qualification or CI green is presumed from this prerequisite.
Source/reference #6240 and protected44-staged worktree unchanged. Subsequent dependent PRs are prepared/tested separately and published only when actual dev diffs fit800. External merge observed: probepark (write-access) approved exact
ce22d610…and merged it at2026-10-03T16:00:38Z, merge commit345b27820380071509222b734f45b41b739e50d0. Exact-head code-event37132359778completed success. Prior stale-base CHANGES_REQUESTED and metadata-only skipped runs remain history, not credited current code proof. The leader performed no merge; approval is not transferred to another head.