feat(settings): unify workflow settings behind one resolver + migrate config-root legacy - #20
feat(settings): unify workflow settings behind one resolver + migrate config-root legacy#20snowykr wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b86864b8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4fc95dca1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The config-root workflow-settings migration must read the target config.yml, decide absent-only patches, apply them, and finalize a marker/source transition inside one critical section. withAtomicYamlConfigTransaction re-exposes the existing per-file queue + cross-process lock so a caller can inspect root/current and applyPatches without re-acquiring the lock; parse failures surface before the callback so no migration action can run against a malformed target. Lore-id: settings-unify-20260805 Constraint: preserve validateRoot/onRestored CAS-restore semantics for existing callers Tested: atomic-yaml-patch suite (12 pass incl. root/current exposure, parse-before-callback, scalar-root no-write)
f4fc95d to
4310923
Compare
…eserve valid legacy values Addresses PR #20 review P2 findings (chatgpt-codex-connector): 1. Don't abort settings load on malformed config.yml. The migration entered the atomic YAML transaction unconditionally, parsing the target before the callback could discover there was no config-root settings.json to migrate; a malformed ~/.gjc/agent/config.yml then made Settings.load() throw, bypassing #loadYaml's recoverable malformed-config path. Now the migration short-circuits when there is no source/backup/marker (never touches the target), and the transaction is wrapped so a parse failure warns and leaves source/backup/marker untouched instead of aborting the load. 2. Preserve valid legacy values when the target value is invalid. The absent-only patch used presence, so an invalid target value (e.g. gjc.ultragoal.nudgeBudget: bad) blocked the patch while the valid legacy fallback was still moved to .bak and marked complete - losing the user's effective value (the resolver would have continued past the invalid layer to the legacy fallback). Only a *valid* present target value now blocks the patch, using the SETTINGS_SCHEMA validators (ranges/enums) so an invalid modern value is repaired with the user's valid legacy setting. Lore-id: settings-unify-20260805 Constraint: migration must never break settings load (best-effort, bounded warn) Tested: 285 focused tests 0 fail (migration 17 incl. 3 new, atomic 12); check:types clean; biome clean
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e70712c052
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e70712c052
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses PR #20 review P2 (chatgpt-codex-connector): when the agent config.yml stores an invalid value in the accepted FLAT YAML form (e.g. "gjc.ralplan.maxIterations": bad), the migration's nested patch was masked after the legacy source moved to .bak because extractWorkflowSetting checks flat keys before nested ones - so strict ralplan would still exit 2 on the old invalid value. The atomic transaction seam gains removeTopLevelKeys, which deletes dotted top-level key names verbatim (the patch grammar always splits on '.' and cannot address such keys). The migration now removes the flat form of every key it patches, so the valid migrated nested value is what resolution sees. Lore-id: settings-unify-20260805 Constraint: transaction writes stay atomic under the config lock; no patch-grammar change Tested: 287 focused tests 0 fail (migration 18 incl. flat-invalid replacement, atomic 13 incl. removeTopLevelKeys); check:types clean; biome clean
Addresses PR #20 review P2 (chatgpt-codex-connector): a legacy config-root settings.json value that is invalid for a tolerant workflow key (e.g. gjc.ultragoal.nudgeBudget: "bad") was copied verbatim into the durable config.yml before the source moved to .bak - seeding a public setting that Settings.load()/config doctor would report on every startup (previously the tolerant runtime simply ignored it and fell back to the default). The migration now validates each legacy value with the same SETTINGS_SCHEMA validators used for the target before adding the patch; invalid legacy values are not migrated, so config.yml never receives them. Lore-id: settings-unify-20260805 Tested: 288 focused tests 0 fail (migration 19 incl. invalid-source-value not-copied); check:types clean; biome clean
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0db0d54647
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…alues Addresses PR #20 review P2 (chatgpt-codex-connector): when the legacy config-root settings.json holds an invalid STRICT ralplan key (e.g. gjc.ralplan.maxIterations: "bad"), skipping it and moving the source to .bak consumed the only invalid layer, so the strict resolver (invalidPolicy "throw") no longer exits 2 and silently fell back to defaults - hiding the user's misconfiguration that previously failed loudly. The migration now aborts (keeps the source active, no marker, no .bak) when any gjc.ralplan.* key has an invalid legacy value, preserving the loud exit-2 contract; tolerant keys keep the skip behavior from 0db0d54. Lore-id: settings-unify-20260805 Constraint: strict ralplan failures must stay loud (AGENTS.md fail-closed contract) Tested: 289 focused tests 0 fail (migration 20 incl. strict-invalid keeps source active); check:types clean; biome clean
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de7c5aa5e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ig.yml Addresses PR #20 review P2 (chatgpt-codex-connector): flat dotted keys in config.yml won extraction, but Settings.get/set/unset and gjc config get/reset address nested paths only - so a flat override was invisible to and unmanageable via the public config CLI. extractWorkflowSetting now gates flat-key support to JSON documents: config.yml uses the nested (schema) form (consistent with schemas/config.schema.json), while legacy settings.json keeps honoring flat dotted keys for backward compatibility. The migration applies the same rule to its YAML target checks; the flat-key cleanup (removeTopLevelKeys) stays for inert leftovers. SKILL docs updated to match. Lore-id: settings-unify-20260805 Constraint: config.yml settings must be manageable via Settings/config CLI Tested: 290 focused tests 0 fail (resolver 12 incl. flat-ignored-in-YAML, migration 20); check:types clean; biome clean; skill-docs + default-definitions gates pass
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c98a452229
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses PR #20 review P2 (chatgpt-codex-connector): when the default global agent config.yml is written by a newer schema version, the migration ran before #loadYaml sets #futureSchemaVersion and could patch the target and consume the legacy source - contradicting Settings' intentional read-only treatment of future-schema configs. The migration now checks configSchemaVersion in the transaction root first and returns without writing a marker, patching, or moving the source. Lore-id: settings-unify-20260805 Constraint: future-schema config.yml stays read-only across all Settings paths Tested: 291 focused tests 0 fail (migration 21 incl. future-schema skip); check:types clean; biome clean
319c2c4 to
ce6611c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce6611c3c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…gs schema Addresses fork PR #20 review P2 (chatgpt-codex-connector): a quoted numeric workflow value in config.yml (e.g. gjc.ralplan.maxIterations: "7") was passed raw to the workflow parser, so strict ralplan exited 2 and tolerant deep-interview/ultragoal fell through to lower layers/defaults - while reconcileSettingsSchema already coerces numeric strings for number settings and Settings.get treats the value as usable. The resolver now applies the same scalar coercion (numeric string -> number) before the workflow parser for every layer, matching Settings. Enum workflow keys never carry numeric strings, so the coercion is a no-op there. Lore-id: settings-unify-20260805 Tested: 248 focused tests 0 fail (resolver 13 incl. quoted-numeric coercion, settings.json non-numeric string preserved); check:types clean; biome clean
3c292ec to
522ab32
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 522ab32fb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
352f3ab to
2e9d49e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e9d49ee7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2f16ff6 to
0a055fd
Compare
The ultragoal nudge budget is a workflow setting read by the runtime but was missing from SETTINGS_SCHEMA, so gjc config set rejected it as an unknown path and the generated config.schema.json did not document it. Registered with the existing default (10) and integer >= 0 validation, schema regenerated. Lore-id: settings-unify-20260805 Tested: check:schemas clean; generated config.schema.json contains ultragoal.nudgeBudget default 10
d0a62bd to
17b1565
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17b1565ae2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
19fa5f8 to
1408842
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14088421bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f589c3a to
8e79772
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e79772c9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8e79772 to
99999c2
Compare
Introduce workflow-settings.ts as the single source of precedence for the four workflow setting surfaces (project config.yml > project settings.json > agent config.yml > legacy config-root settings.json > default). No workflow runtime hand-rolls file discovery, YAML/JSON parsing, or key extraction anymore; the resolver exposes typed diagnostics, an explicit invalid policy (continue by default, throw for ralplan), flat/nested extraction (flat wins), and canonical realpath sources using the exported standardizeMacOSPath precedent. Lore-id: settings-unify-20260805 Constraint: pure module - no Settings/discovery imports, no module-scope path hoisting Tested: workflow-settings suite (11 pass) + atomic-yaml suite (12 pass); package + utils check:types clean
…g.yml One-time migration of the orphan config-root settings.json workflow keys into the default global agent config.yml, hardened by codex review: malformed config.yml never aborts settings load (pre-transaction short-circuit + resilience catch); only valid target values block the patch; invalid flat keys are removed verbatim (removeTopLevelKeys, non-restorable receipt); legacy values are validated before migrating; invalid strict ralplan values keep the source active so gjc ralplan still fails loudly; flat keys are honored only in legacy JSON; future-schema config.yml targets are skipped; quoted numeric values are coerced like the Settings schema; the no-clobber move fallback uses COPYFILE_EXCL. Lore-id: settings-unify-20260805 Constraint: custom/temporary agentDir (SDK loadForScope, tests) must never consume the global source Constraint: future-schema config.yml stays read-only across all Settings paths Tested: 291 focused tests 0 fail; check:types clean; biome clean
ralplan, ultragoal, and deep-interview no longer hand-roll settings.json reads. All five workflow keys resolve through workflow-settings.ts with the fixed five-layer precedence; project configuration now beats user configuration (deep-interview's former user-YAML-first inversion is corrected), and ralplan is strict for all three keys (malformed/invalid explicit sources exit 2; the former silent maxIterations fallback is removed, user-confirmed option B). ultragoal/deep-interview stay tolerant (invalid optional files continue). Lore-id: settings-unify-20260805 Constraint: keep resolve* signatures, JSON source fields, and ralplan exit-2 error contract (malformed JSON/YAML reasons stable) Tested: 188 focused tests (ralplan 110, deep-interview 25, ultragoal-nudge 17, config-root 3, resolver 11, migration 10, atomic-yaml 12) + settings-manager/state-runtime/ultragoal-runtime 258 pass; check:types clean
All three workflow SKILL files now describe the identical precedence (project .gjc/config.yml > project .gjc/settings.json > user agentDir/config.yml > legacy config-root settings.json > default), flat/nested key shapes (flat wins), project-beats-user, canonical source, and the one-time paired root migration. ralplan states the strict exit-2 contract including the removed maxIterations silent fallback; ultragoal/deep-interview state the tolerant continue-to-default behavior. Stale deep-interview user-YAML-first and ralplan project-only paragraphs removed. Lore-id: settings-unify-20260805 Tested: default-gjc-definitions (28 pass), check-visible-definitions OK, verify-g002-gates pass, verify-gjc-skill-docs --fail 0 drift, rebrand-inventory --strict clean
User-facing changes per CONTRIBUTING.md: five-layer settings precedence, nudgeBudget schema registration, config-root one-time migration, strict ralplan exit-2, and nested-only config.yml key form. Lore-id: settings-unify-20260805
99999c2 to
a641218
Compare
1 participant
What
Unify the three workflow runtimes' (ralplan / ultragoal / deep-interview) settings reads behind one shared resolver with a fixed five-layer precedence, and migrate the orphan legacy config-root
settings.jsoninto the modernconfig.ymlexactly once.src/gjc-runtime/workflow-settings.ts— one resolver, five layers: project.gjc/config.yml→ project.gjc/settings.json→ user<agentDir>/config.yml(trustedGJC_CODING_AGENT_DIR/GJC_CONFIG_DIR+ XDG) → legacy<configRoot>/settings.json→ built-in default.config.ymluses the nested schema form (flat dotted keys honored only in legacysettings.json), so every effective override stays manageable viaSettings/gjc config. Canonical realpathsource, typed diagnostics, explicit invalid policy (continue by default, throw for ralplan).src/config/atomic-yaml-patch.ts—withAtomicYamlConfigTransaction(under-lock transaction seam withremoveTopLevelKeysfor dotted top-level keys).src/config/settings.ts— one-time config-root migration into the default global agentconfig.yml(absent-only, durable pending/complete marker, no-clobber.bak, crash-recovery state machine, strengthened default-global pairing gate; invalid strict ralplan legacy values keep the source active; future-schema targets never touched).src/config/settings-schema.ts— registergjc.ultragoal.nudgeBudget(default 10, integer ≥ 0);schemas/config.schema.jsonregenerated.maxIterationsfallback removed — user-confirmed); ultragoal/deep-interview tolerant; deep-interview's inverted precedence corrected to project-beats-user.Why
The workflow runtimes hand-rolled
settings.json-only reads and never consulted the modernconfig.yml, sogjc config set gjc.ralplan.maxIterations 7(config.yml) was silently ignored by ralplan. The legacy config-root~/.gjc/settings.jsonwas an orphan path no migration covered, thegjc.ultragoal.nudgeBudgetkey was missing from the public schema, and deep-interview read user YAML before project JSON (inverted precedence). This PR makes the settings surface single-source: whatgjc configwrites is what the runtimes read, with one documented precedence.Testing
check:types,generate-schemas/check:schemasclean; biome clean on the change set.check-visible-definitionsOK,verify-g002-gatespass,verify-gjc-skill-docs --fail0 drift,rebrand-inventory --strictclean,default-gjc-definitions28 pass.gjc ralplan --writewith project.gjc/config.ymlmaxIterations: 2): 3rd revision opener exits 3 withPLANNING-STUCK,max_iterations: 2,max_iterations_source: <project>/.gjc/config.yml— config.yml honored end-to-end.bun checkis run by CI on this head (not completed locally — 15-min timeout); the TS/schema/gate subset passed locally.GJC verdict
devbun checkpasses (full run in CI on this head; focused TS/schema/gate subset passed locally)