Skip to content

feat(config): experimental worktrunk.config.* git-config project-config source - #3609

Open
indexzero wants to merge 13 commits into
max-sixty:mainfrom
indexzero:feat/3454/claude-fable
Open

indexzero wants to merge 13 commits into
max-sixty:mainfrom
indexzero:feat/3454/claude-fable

Conversation

@indexzero

Copy link
Copy Markdown

What this adds

An experimental second source for project configuration: worktrunk.config.* keys in git config.

$ git config worktrunk.config.post-start 'pnpm install'
$ git config worktrunk.config.list.url 'http://localhost:3000'

One mechanical rule governs the mapping. Strip worktrunk.config.; the remainder is the exact key path from .config/wt.toml. worktrunk.config.post-start is the top-level post-start hook. worktrunk.config.list.url is [list] url. No renamed keys, no parallel schema.

Why

.git/config is never committed, never pushed, never cloned. Configuration written there is private to one machine by construction — and shared across every linked worktree, because the local scope lives in the common git dir. That combination is what issue 3454 asks for, and what several earlier issues (650, 1077, 2818) kept circling: private, repo-local config without a new file format and without repo identities leaking into public dotfiles.

How selection works

All-or-nothing. When any worktrunk.config.* key exists, those keys are the complete project config; .config/wt.toml is ignored. There is no key-level merging between sources.

Two consequences follow, both deliberate:

  • Supersession is loud. When a project config file would otherwise load, a once-per-invocation warning names it, and a hint gives the diagnostic command: git config --show-scope --show-origin --get-regexp '^worktrunk\.config\.'. wt config show and wt hook show name the active source.
  • Parse failures are fatal. An invalid value errors with the schema's own message. Falling back to the file would silently change which hooks run.

What stays the same

  • Approval. Hooks and aliases from git config pass through the same approval gate as file-based project config. Git config can carry remotely-authored content through include/includeIf (a cloned dotfiles repo, for instance), so source alone is not a trust signal.
  • Precedence belongs to git. Keys are read from the already-cached git config --list -z map — zero new subprocesses. Local overrides global; conditional includes work; worktrunk never sees scopes.
  • Existing interfaces. .config/wt.toml, [projects."…"] overrides, and WORKTRUNK_PROJECT_CONFIG_PATH are untouched.

Limits in this first version

Values are strings, one per key. Fields needing other TOML types (step.copy-ignored.exclude, an array) are not expressible and error clearly when attempted. Keys must be lowercase — git's key model makes middle segments case-sensitive, and the schema's own spelling is lowercase. No write commands: git config already is the management interface.

Testing

Unit tests cover the key mapping: nesting, value/table conflicts, empty segments, loud failure on non-string fields. Integration tests cover source selection, the supersession warning firing exactly when a file is superseded, local-over-global precedence, include.path resolution, approval gating, and wt config show in human and JSON forms. The config docs page gained a "Private project config in git config" section, and all generated doc mirrors and help snapshots are refreshed.

@indexzero
indexzero force-pushed the feat/3454/claude-fable branch 2 times, most recently from b2be351 to c2a50d0 Compare July 26, 2026 09:09

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.

Nicely structured — the mechanical key mapping, the all-or-nothing selection, and the consistent "name the source, keep the approval gate" treatment across every consumer (config show, hook show, diagnostic, approvals, create, alias) all hang together. The diagnostic path deliberately omitting values (only naming keys + the superseded file) is a good call given diagnose output gets pasted into public bug reports.

One functional gap in the wt step prune skip-hint, noted inline: the git_source_active → invoking_project_bytes = None change doesn't actually suppress the (different hooks on branch) annotation the way its comment claims — it inverts it for the supersession case. Details on the line.

Comment thread src/commands/step/prune.rs

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the new commit since the last review — mostly test coverage, plus one behavior change in the diagnose path worth a second look (inline).

Comment thread src/diagnostic.rs Outdated
@indexzero
indexzero force-pushed the feat/3454/claude-fable branch 3 times, most recently from f2f7d65 to f022b69 Compare August 4, 2026 15:38
@indexzero
indexzero marked this pull request as ready for review August 4, 2026 15:51
@indexzero

Copy link
Copy Markdown
Author

@max-sixty after reviewing this a few times, I have started using this as my default for wt locally as of today

Will run this from now on to shake loose any other bugs, but it feels solid

Curious to hear your thoughts! 🙏

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full pass now that it's out of draft. The core is in good shape — mechanical key mapping, all-or-nothing selection, and the "name the source, keep the approval gate" treatment across every consumer are consistent, and both draft-review findings are properly fixed (the differs guard in prune is now on the computation, and the diagnostic keeps its explicit Err arm). Test coverage is unusually thorough for a config source: scope precedence, include.path, includeIf, the override kill switch, declined-approval non-execution, and alias discovery matching dispatch.

Not approving, on repo policy rather than on the code: the diff edits src/commands/step/prune.rs, which is wt step prune's removal driver (branch deletion via delete_branch_if_safe, worktree removal). The project's review rules hold any change reaching that surface for a human. Requesting @max-sixty.

Three things worth a look, plus inline suggestions.

--global × all-or-nothing is the sharp edge, and neither doc paragraph says it. The docs note that --global "puts a key in every repository" and, separately, that selection is all-or-nothing — but not the product: a single git config --global worktrunk.config.list.url … set for convenience silently switches every repository on the machine off its committed .config/wt.toml, dropping all project hooks. The supersession warning does fire, which is the right backstop, but a user reading only the docs won't see it coming. Suggestion inline.

config.worktree is not uniformly ignored. Both the module spec and the docs say a config.worktree value is "never consumed" because the bulk read runs from the common git dir. That holds for a linked worktree, but the main worktree's config.worktree lives at $GIT_COMMON_DIR/config.worktree, so the common-dir read does pick it up — this repo's own prewarm_git_config_from_common_dir docstring relies on exactly that ("the common dir … sees the full merged set", vs. a linked-worktree read "missing values set on the main worktree's config.worktree"). Under extensions.worktreeConfig, then, a key placed in the main worktree's worktree-scoped file supplies project config repository-wide — the opposite of what the reader is told to expect. Two inline suggestions.

The diagnostic's value omission is defeated inside the same artifact. config_show_output deliberately prints key names without values because "diagnose output is routinely pasted into public bug reports" — but the report's own trace section embeds the raw git config --list -z output, values included, as the integration test concedes in its scoping comment. That matters more than it would for an ordinary git key, because this source's stated purpose is private configuration and the triage path (running-tend) tells reporters to run wt -vv and gh gist create the bundle. Calling it pre-existing is fair for git config in general, but the values only get there because this feature put them there. Worth deciding between the two consistent endpoints: redact worktrunk.config.* values in the trace layer too, or drop the value omission and let the report say plainly that git config values are included (matching the module's Privacy section, which already lists "config files"). The current middle reads as a guarantee it doesn't deliver.

Smaller notes:

  • Git lowercases the final key component, so user-chosen names get silently lowercased: worktrunk.config.aliases.Deploy lands as deploy (verified against git). Fine as behavior, but it isn't covered by "keys must be written in lowercase — exactly how the schema spells them", since alias names aren't schema-spelled. Folded into the docs suggestion.
  • warn_superseded_project_file's "SET it only on emit" comment justifies itself with wt config create --project's born-superseded warning, but that warning is an independent eprintln! in create.rs and never consults this latch. The behavior is right; the reason given for it isn't. Inline.

Comment thread src/cli/mod.rs
Comment thread src/cli/mod.rs Outdated
Comment thread src/config/git_source.rs Outdated
Comment thread src/config/git_source.rs Outdated
@worktrunk-bot
worktrunk-bot requested a review from max-sixty August 5, 2026 00:42
@indexzero
indexzero force-pushed the feat/3454/claude-fable branch from f022b69 to 690dcbd Compare August 13, 2026 05:25
indexzero added a commit to indexzero/worktrunk that referenced this pull request Aug 13, 2026
… trace redaction

Responds to the full-pass review on max-sixty#3609:

- Redact worktrunk.config.* values in the diagnostic trace/subprocess
  layer, not just the config section. `Cmd::redact_logged_stdout` applies
  a transform to a command's *logged* stdout (never the bytes the caller
  parses), and the bulk `git config --list -z` read passes
  redact_worktrunk_config_z, which replaces this namespace's values with
  [REDACTED] while leaving every other key intact. The -vv bundle no
  longer carries private values in any section; the config section still
  names keys and omits values.
- Correct the config.worktree claim: only a *linked* worktree's
  config.worktree is unread. The main worktree's lives in the common dir,
  which all_config() reads, so a key there supplies project config
  repo-wide. Fixed in the module spec and the docs.
- Spell out the --global product in the docs: one global key supersedes
  every repository's committed project config at once.
- Note that git lowercases the final key component, so a chosen name is
  lowercased with it (aliases.Deploy runs as `wt deploy`).
- Fix the warn_superseded latch comment: it justified emit-only latching
  via create.rs's born-superseded warning, which never consults this
  latch; the real reason is a later same-invocation call with a
  superseded file to report.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rebase plus 690dcbd — all four open threads from the last pass read as properly addressed, and the fixes are accurate rather than cosmetic: the config.worktree correction now matches what prewarm_git_config_from_common_dir actually does, the --global product is spelled out with a workable mitigation (includeIf), and the latch comment states the reason it really has. I'm resolving those threads.

The trace redaction is the substantive addition and it's well-placed — a Cmd-level redact_logged_stdout that only touches the copy log_output writes, gated behind the existing log_enabled!(Debug) check so a normal run pays nothing, applied at all three git config --list -z sites. The -z framing (first \n per NUL-delimited entry is the key/value split) is the right parse, and leaving valueless and non-matching entries verbatim keeps it scoped to this namespace.

Two things on the new commit, both inline.

pipe_into honors the redactor on the stage that can't leak and drops it on the stage that can. The source's stdout is routed into the pipe, so its redactor is a no-op there — but the sink's stdout is captured (second_child.wait_with_output()) and logged, and it's passed None unconditionally. Every other field pipe_into can't support (stdin_data, timeout, external_label, directive_*_file) is guarded by an assert; this one is silently ignored. For a field whose entire job is keeping private bytes out of the bundle, silent-drop is the wrong failure direction.

The bundle-wide leak assertion can pass without proving anything. The loop's let … else { continue } skips a missing log silently, so if trace.log/subprocess.log were ever absent — or the config read stopped going through the traced path — the test still passes on diagnostic.md alone, which the earlier assertions already cover. The comment claims "the key name survives, and other config is intact," but nothing asserts either. A positive check that the redacted listing actually landed in the uncapped sink turns absence-of-value into evidence of redaction.

Not approving, same reason as last time rather than anything new in this commit: the diff still reaches src/commands/step/prune.rs, and the repo's review rules hold that surface for a human. @max-sixty is already requested.

Comment thread src/shell_exec.rs Outdated
Comment thread tests/integration_tests/git_config_source.rs Outdated
@indexzero
indexzero force-pushed the feat/3454/claude-fable branch from 690dcbd to 7a00d43 Compare September 15, 2026 15:44
indexzero added a commit to indexzero/worktrunk that referenced this pull request Sep 15, 2026
… trace redaction

Responds to the full-pass review on max-sixty#3609:

- Redact worktrunk.config.* values in the diagnostic trace/subprocess
  layer, not just the config section. `Cmd::redact_logged_stdout` applies
  a transform to a command's *logged* stdout (never the bytes the caller
  parses), and the bulk `git config --list -z` read passes
  redact_worktrunk_config_z, which replaces this namespace's values with
  [REDACTED] while leaving every other key intact. The -vv bundle no
  longer carries private values in any section; the config section still
  names keys and omits values.
- Correct the config.worktree claim: only a *linked* worktree's
  config.worktree is unread. The main worktree's lives in the common dir,
  which all_config() reads, so a key there supplies project config
  repo-wide. Fixed in the module spec and the docs.
- Spell out the --global product in the docs: one global key supersedes
  every repository's committed project config at once.
- Note that git lowercases the final key component, so a chosen name is
  lowercased with it (aliases.Deploy runs as `wt deploy`).
- Fix the warn_superseded latch comment: it justified emit-only latching
  via create.rs's born-superseded warning, which never consults this
  latch; the real reason is a later same-invocation call with a
  superseded file to report.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full pass over the rebased head. Both findings from the last round are properly
closed: pipe_into now refuses a redactor on either stage rather than dropping
the sink's, and the bundle-wide leak assertion requires all three logs to exist
and positively proves the redacted listing reached subprocess.log, with a
non-worktrunk value still visible beside it.

Two new things, both inline.

wt config show --format json swallows the git-config read failure the other
two surfaces are careful to report.
render_project_config propagates it with
? and says why — "swallowing it would render the file as active while actual
execution errors on the same read, and diagnostics must not disagree with
execution" — and config_show_output prints (git config read failed: …) for
the same reason. The JSON renderer reads it through unwrap_or_default(), so a
failed bulk read collapses into "no keys", drops into the file branch, and
reports "source": "file" with the file's config and invalid unset — while
ProjectConfig::load hard-errors on that same read, so wt merge in that state
fails. repo.project_config_path()? two lines below already propagates, so ?
is the adjacent convention here.

The parse error on the all-or-nothing hard fail points into a document the
user never wrote.
TOML parse error at line 2, column 11 (from
git_config_source_invalid_value_fails_loudly.snap) is a location in the TOML
render_git_source_toml synthesizes; what the user set was a git config worktrunk.config.… key. Since selection is all-or-nothing and a parse failure
is deliberately fatal, this message is what stands between the user and every
wt command working again, so it should name the key rather than a synthetic
line number.

Not approving, for the standing reason rather than anything in this diff: the
change edits src/commands/step/prune.rs, prune's removal driver, which the
repo's review rules hold for a human. @max-sixty is already requested.

Comment thread src/commands/config/show.rs Outdated
Comment thread src/config/git_source.rs
indexzero and others added 4 commits September 15, 2026 13:20
…ig source

Any key under the worktrunk.config. prefix in git config now supplies
project configuration (max-sixty#3454). Strip the prefix; the remainder is the
exact .config/wt.toml key path. Selection is all-or-nothing: when any
key exists, the merged effective git config is the complete project
config, the file is ignored, and a warning names the superseded file.
Values are strings only; schema violations fail loudly with no fallback
to the file. Commands from this source pass through the same approval
gate as file-based project config.

Git owns precedence: keys come from the cached `git config --list -z`
map, so system/global/local scopes and includes resolve before worktrunk
sees them, at zero additional subprocess cost.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Findings from adversarial reviews of the two competing implementations,
applied per the reviewed merge-fix plan:

- Alias discovery routes through ProjectConfig::load, so git-config
  aliases appear in listings exactly when dispatch would run them.
- A WORKTRUNK_PROJECT_CONFIG_PATH override — any value, including the
  empty kill switch — disables the git source, enforced in the sole
  accessor so every consumer inherits the deferral by construction.
- wt config show propagates a failed git-config read instead of
  rendering the file as active while execution errors.
- wt --diagnose names the git-config source, its key names, and the
  superseded file, but never the values: diagnose output is routinely
  pasted into public bug reports and this source exists for private
  configuration.
- The supersession-warning latch is peeked before label resolution and
  set only on emit, so a no-file call cannot suppress a later
  born-superseded warning from wt config create --project.
- Provenance docstring softened: include/includeIf can carry
  remotely-sourced content, which is why the approval gate applies
  unchanged.
- Docs now state what the git source skips (file migration and its
  deprecation messaging — deprecated spellings may still deserialize),
  that per-worktree git config is unsupported, and describe the
  diagnostic command as listing matching keys rather than consumed
  values.
- wt config create --project warns when the new file is born
  superseded; wt step prune suppresses the branch-hooks annotation
  under the git source (git config is branch-independent); project
  config resolves from a bare root via the git source, with
  wt config approvals framing its answers from it.
- New tests: behavioral approval decline (hook must not run),
  includeIf resolution, both override-precedence forms, alias-listing
  consistency, born-superseded create, bare-root approvals, and
  diagnose value redaction.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- wt config show snapshots for the two error renderings: a git-source
  value that violates the schema (Invalid config + gutter) and colliding
  key paths (conflict error), matching the file branch's treatment.
- wt step prune --dry-run under the git source exercises the suppressed
  branch-hooks baseline.
- A bare root without keys exercises the no-project-config guard and its
  path-free error.
- A repeat-call unit test exercises the supersession-warning latch peek.
- The diagnose Err arm collapses into the file fallback (best-effort
  surface; the file section already reports read failures) and the
  infallible-in-practice TOML render error map shrinks to a passthrough,
  removing two unreachable specialized handlers instead of pretending to
  test them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r arm

Two review findings on the follow-up commits:

- wt step prune suppressed the "(different hooks on branch)" annotation
  by nulling the comparison baseline, which inverts it: a candidate with
  a committed .config/wt.toml compares Some(_) != None and flags the
  exact case where git-config hooks are identical across branches. The
  suppression now lives on the differs computation itself, pinned by a
  test with a committed file and an unapproved git-config pre-remove.

- The diagnose Err arm returns: a failed bulk-config read is reported
  as such instead of falling through and printing .config/wt.toml as
  the active source — the file section's "(read failed)" covers a
  different read, so the fall-through misattributed, in the report
  meant to diagnose exactly that failure. Its uncovered line is an
  honest, effectively untestable gap, and is documented as one.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
indexzero and others added 9 commits September 15, 2026 13:20
test_git_config_scope_precedence_local_over_global wrote its "global"
worktrunk.config.* keys with `git config --global`, which lands in the
process-shared test gitconfig (test_gitconfig_path) that every parallel
test points GIT_CONFIG_GLOBAL at. Those keys then bled into other tests'
merged `git config --list`, non-deterministically shifting, e.g., the
TOML line number in test_git_config_source_invalid_value_fails_loudly
(line 2 → line 8) whenever the two tests overlapped.

Give the global tier its own GIT_CONFIG_GLOBAL file so the global scope
stays private to this test. No other test writes --global.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the codecov/patch gaps on the worktrunk.config.* changes:

- git_source: the supersession-warning latch drops its untestable
  race-loser guard for a race-tolerant `let _ = WARNED.set(())`; the
  top peek still suppresses the common re-entry.
- config show: the superseded-file warning and the diagnostic-command
  hint move from writeln!(…)? to push_str — writing into a String is
  infallible, so `?` left an uncoverable error region.
- diagnostic: three in-process unit tests exercise config_show_output's
  git-source branch (names source + keys, omits values), the file
  fallback, and the failed-read arm; the superseded-file note builds
  via map/unwrap_or_default so its region records under coverage.
- config.rs: project_config reverts to upstream's form. Upstream now
  has current_worktree().root() fall back to the discovery path instead
  of erroring, so load() is always reached and handles the git-config
  source itself — the earlier in_worktree/git-keys guard (and its dead
  else) is unnecessary.
- config create: an error-path test (`.config` is a regular file, so
  create_dir_all fails) covers the create call's `?`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… trace redaction

Responds to the full-pass review on max-sixty#3609:

- Redact worktrunk.config.* values in the diagnostic trace/subprocess
  layer, not just the config section. `Cmd::redact_logged_stdout` applies
  a transform to a command's *logged* stdout (never the bytes the caller
  parses), and the bulk `git config --list -z` read passes
  redact_worktrunk_config_z, which replaces this namespace's values with
  [REDACTED] while leaving every other key intact. The -vv bundle no
  longer carries private values in any section; the config section still
  names keys and omits values.
- Correct the config.worktree claim: only a *linked* worktree's
  config.worktree is unread. The main worktree's lives in the common dir,
  which all_config() reads, so a key there supplies project config
  repo-wide. Fixed in the module spec and the docs.
- Spell out the --global product in the docs: one global key supersedes
  every repository's committed project config at once.
- Note that git lowercases the final key component, so a chosen name is
  lowercased with it (aliases.Deploy runs as `wt deploy`).
- Fix the warn_superseded latch comment: it justified emit-only latching
  via create.rs's born-superseded warning, which never consults this
  latch; the real reason is a later same-invocation call with a
  superseded file to report.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mechanical only: config-show snapshots pick up upstream's new APPROVALS
section and hermetic-env floor; the doc mirrors regenerate into the
relocated docs/src/content/docs path; project.rs is a rustfmt line-wrap
of the rebase conflict resolution.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce redaction

Two findings on the trace-redaction commit:

- pipe_into honored a redactor on the source stage (a no-op, since the
  source's stdout is routed into the pipe) and silently dropped it on the
  sink stage, whose output is captured and logged. A redactor's whole job
  is keeping private bytes out of the bundle, so refuse it with an assert —
  matching every other field pipe_into can't support — rather than
  silently ignore it. No config read uses pipe_into.

- The bundle-wide leak test skipped a missing log via `let … else
  { continue }`, so it could pass on diagnostic.md alone. It now requires
  subprocess.log (the uncapped sink) and positively asserts the redacted
  listing landed there — key name and [REDACTED] present, value absent —
  plus a non-worktrunk key whose value stays visible, so absence-of-value
  is evidence of redaction rather than of the listing missing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `built-site` route-contract test pins the config page's table-of-
contents heading count. The new "Private project config in git config"
section adds one H2, so the count moves 33 → 34. Verified against a
fresh `astro build`: the route-contract test passes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Worktrunk Bot <w@worktrunk.dev>
Co-authored-by: Worktrunk Bot <w@worktrunk.dev>
The applied review suggestion makes the all-or-nothing parse failure name
the offending git-config key ("From: worktrunk.config.…") instead of only
a synthetic TOML line. Regenerate the snapshot to match.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@indexzero
indexzero force-pushed the feat/3454/claude-fable branch from e1eccfc to e2a317b Compare September 15, 2026 17:25

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both suggestions from the last pass are applied as written and hold up. The JSON
report now propagates the bulk-read failure the way config_show_output and
ProjectConfig::load already do, so all three agree on that read and only
diagnostic.rs keeps its deliberate best-effort note. The parse error names the
worktrunk.config.* keys it was rendered from, keys only.

One thing the applied suggestion left behind, inline: the comment directly above
the changed line still describes that read as tolerant, so it now contradicts its
own sibling comment on the identical read.

Comment on lines +147 to +151
// Experimental git-config source (#3454): when any `worktrunk.config.*`
// key exists it is the whole project config, with no on-disk path.
// Parse it tolerantly like the file path below — a broken source
// sets `invalid` and reports null config rather than aborting the
// report — and name it as the active source.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap_or_default() is gone from the line below, but the comment above it still
says the read is tolerant — "a broken source sets invalid and reports null
config rather than aborting the report". That now describes only the parse
(render_git_source_toml + parse_json_config); the read itself aborts.

The sibling comment on the identical call in config_show_output gives the
opposite rationale — "A failed read propagates rather than defaulting to empty
— swallowing it would render the file as active while actual execution errors on
the same read, and diagnostics must not disagree with execution." Two comments in
one file disagreeing about one accessor is what invites the revert: the next
reader reaching for a report that always renders has this comment's authority for
putting unwrap_or_default() back.

Suggested change
// Experimental git-config source (#3454): when any `worktrunk.config.*`
// key exists it is the whole project config, with no on-disk path.
// Parse it tolerantly like the file path below — a broken source
// sets `invalid` and reports null config rather than aborting the
// report — and name it as the active source.
// Experimental git-config source (#3454): when any `worktrunk.config.*`
// key exists it is the whole project config, with no on-disk path.
// A failed read propagates, matching `config_show_output` and
// `ProjectConfig::load` — swallowing it would report the file as
// active while execution errors on the same read. An unparseable
// source is still tolerated like the file path below: `invalid` is
// set and the config reported null rather than aborting the report.

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.

2 participants