feat(#4313): apply connector config defaults at resolve time - #4314
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4314 +/- ##
=======================================
Coverage 61.49% 61.49%
=======================================
Files 2534 2534
Lines 101630 101642 +12
Branches 28464 28479 +15
=======================================
+ Hits 62496 62509 +13
+ Misses 38567 38566 -1
Partials 567 567
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/fs-fix changeset and all known issues so far (like CI) |
|
🤖 Finished Fix · ✅ Success · Started 6:51 AM UTC · Completed 7:10 AM UTC Commit: |
- Add changeset for connector config defaults feature (minor bump) - Remove stale changeset from previous release (already consumed on main) - Regenerate frontend report.api.md to match API Extractor output, fixing the CI "check api reports" step failure Addresses review feedback on #4314
🔧 Fix agent — iteration 1 (human-triggered)Addressed both issues from /fs-fix instruction: (1) added proper changeset for the connector config defaults feature and removed the stale changeset from a previous release, (2) fixed CI by regenerating the frontend API report to match API Extractor output. Fixed (2):
Tests: passed Updated by fullsend fix agent |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:32 AM UTC · Completed 7:49 AM UTC Commit: |
ReviewReason: stale-head The review agent reviewed commit Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:27 AM UTC · Completed 11:45 AM UTC Commit: |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
workspaces/boost/plugins/boost-backend/src/config/RuntimeConfigResolver.ts:96: [medium] behavioral-contract-change
resolve() now returns a field default value instead of undefined when neither DB override nor YAML baseline is set. This changes the observable contract of a public method. Consumers checking for undefined to distinguish 'not configured' from 'has a value' will see different behavior. The method's return type did not change (Promise<unknown | undefined>), so consumers have no compile-time signal.
Suggested fix: Verify no external consumer relies on undefined to distinguish 'not configured.' Consider offering a separate method or source indicator (see issue #4066).
workspaces/boost/plugins/boost-backend/src/config/RuntimeConfigResolver.ts(file-level): Line 61 · [low] JSDoc consistency
The class-level JSDoc still says 'Two-layer configuration resolver' but the PR adds a third precedence layer. Method-level JSDoc was correctly updated.
workspaces/boost/plugins/boost-backend/src/config/RuntimeConfigResolver.ts(file-level): Line 46 · [low] resolve() vs resolveAll() consistency
resolve() uses value !== undefined to decide fallback while resolveAll() uses effective.has(key). These differ semantically when a Map key exists with value undefined. Cannot happen today but would cause disagreement if a future code path sets a key to undefined.
workspaces/boost/plugins/boost-backend/src/config/RuntimeConfigResolver.ts:95: [low] architectural-coherence
resolve() applies defaults per-call via getFieldDefault() while resolveAll() iterates all keys to layer defaults. The asymmetry is functionally correct but worth noting for future maintainers.
workspaces/boost/plugins/boost-backend/src/config/RuntimeConfigResolver.ts:118: [low] behavioral-contract-change
resolveAll() now includes entries for keys with field defaults even when not set in DB or YAML. Consumers using map.has(key) to check explicit configuration will see a behavioral difference.
workspaces/boost/plugins/boost-backend/src/config/schemas.ts:190: [low] version-constant-change
BOOST_CONFIG_SCHEMA_VERSION bumped from 4 to 5. Cross-repo consumers or migration scripts asserting version == 4 will need updating.
workspaces/boost/plugins/boost-backend/src/config/schemas.ts:558: [low] Accessor function pattern consistency
getFieldDefault() casts boostConfigFields[key] as ConfigFieldMeta without | undefined, whereas isSensitiveField() uses ConfigFieldMeta | undefined with optional chaining.
workspaces/boost/plugins/boost-backend/src/config/schemas.ts:170: [low] JSDoc punctuation consistency
The defaultValue field JSDoc is a run-on sentence. Existing field JSDoc in the same interface uses short single-sentence descriptions.
|
Response on #4314 (comment) fullsend-ai's review: Medium — This is the intended behavior from #4313, not a regression. Optional numeric fields must resolve to a stable effective value ( There are no production Source tagging (“using default” vs “explicitly set”) is out of scope here; Medium — OpenSpec “two-layer” → “three-layer” Please don’t mass-replace that wording. The one OpenSpec tweak worth doing (from #4313 item 5) is a one-liner on Default Values: “schema provides default” means Low
None of these should block merge. |
|
you've now got some conflicts @mareklibra given the merges yesterday; but as I mentioned in slack, per you request yesterday, I hope to provide a review sometime today my time - thanks |
There was a problem hiding this comment.
yeah a overall I like what is here and addressing nuance and devil in the details wrt config precedence @mareklibra
aside from needing to rebase (as of this comment) my only ask is not with the code changes themselves, but that you update https://github.com/redhat-developer/rhdh-plugins/blob/main/workspaces/boost/specifications/JIRA-analysis/rhdhplan1505-1-staged-issues.md and
- add references to #4313 around the 4 of 29 issue #4042 write up
- including explaining whatever staging or ordering is required between that and minimally #4042
- and confirm that what remains in #4042 is what's left after presumably #4223 and #4220 are done , as I see the description has less content than the original description as listed in the markdown file ... I'm fine replacing the original desription listed in the markdown with the new one you have
So yeah, even though this PR is only for #4223 I'd like you new issues added in whichever tier and spot in the existing staging you think then need to go ...maybe use a "4.1 of 29" , "4.2 of 29", "6.1 of 29", etc.
If claude and I parsed it correctly, we are talking about #4220 , #4313 , #4286 , #4285
also, what is up with the older #3734 ..... is that still valid ? and if so, again, where does it fit in the staging plan
thanks
Add optional `defaultValue` field to `ConfigFieldMeta` and apply it as a third precedence layer in `RuntimeConfigResolver`: DB override → YAML baseline → field default → undefined. Default values per OpenSpec task 1.8: - schedule.intervalMs: 300000 (5 min) — jira, github, gitlab - batchSize: 100 — jira, github, gitlab - timeout.connectionMs: 30000 (30 s) — jira only Defaults are NOT added via Zod `.default()` to preserve "unset" semantics: `validateConfigValue(key, undefined)` still returns undefined, and resolver precedence is respected. Instead, the `defaultValue` property on `ConfigFieldMeta` is read by a new `getFieldDefault()` helper, which the resolver calls when neither DB nor YAML provides a value. Fields without defaults (`enabled`, `endpoint`, `schedule.cron`) continue to return `undefined` when absent. Bumps BOOST_CONFIG_SCHEMA_VERSION from 4 to 5. Closes #4313
- Add changeset for connector config defaults feature (minor bump) - Remove stale changeset from previous release (already consumed on main) - Regenerate frontend report.api.md to match API Extractor output, fixing the CI "check api reports" step failure Addresses review feedback on #4314
- Document that RuntimeConfigResolver.resolveAll() returns a fresh per-call snapshot Map, safe to mutate/retain without affecting subsequent resolver calls or the underlying cache - Note that resolveAll() cannot distinguish synthesized field defaults from real DB/YAML values; flagged for the admin UI work in #4066 - Change getFieldDefault() to a generic signature keyed on BoostConfigKey so callers get the field's typed schema output instead of `unknown` Signed-off-by: Marek Libra <marek.libra@gmail.com>
3d124d1 to
4b24250
Compare
|
Rebased |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 12:19 PM UTC · Completed 12:38 PM UTC Commit: |
Document that field defaults are a read-time fallback (not Zod .default()), align resolve() with resolveAll() on Map.has(), and drop "this PR" from the 6.1 staging slot. Signed-off-by: Marek Libra <marek.libra@gmail.com>
|
|
@gabemontero PTAL The JIRA-analysis/rhdhplan1505-1-staged-issues.md is updated. Working on #3734 , I will send comments/changes there. #3734 is still valid, lives under 1509 / RHIDP-15440, is the UsageTab slice of Issue 24 tasks 3.1–3.2 and is not a 4.x under #4042. Implemented by #3746. |
|
/fs-review |
gabemontero
left a comment
gabemontero
left a comment
There was a problem hiding this comment.
awesome / thanks @mareklibra
at least for me, the 6.1 etc. nomenclature helps me organize / understand the staging impact
|
hit the merge button at your discretion |
|
🤖 Finished Retro · ✅ Success · Started 1:47 PM UTC · Completed 2:04 PM UTC Commit: |
Retro: PR #4314 — apply connector config defaults at resolve timeWorkflow: Issue #4313 (manual Timeline
Review Agent AssessmentOf 12 findings in the substantive review: 5 were acknowledged true positives (accessor pattern inconsistency, JSDoc nits, code consistency observations), 4 were true observations but zero-risk (intentional behavioral changes per issue requirements), and 3 were false positives where the agent recommended mass-replacing "two-layer" with "three-layer" in documentation. The human correctly explained the cache merge IS two-layer — field defaults are a read-time fallback, not a third cache layer. The agent's recommendation would have introduced architecturally incorrect terminology. All inline review comments failed with 422 errors and were posted as body text instead. Human vs. Agent Review GapThe review agent focused exclusively on code-level findings (behavioral contracts, documentation consistency, code style). The human reviewer ( Autonomy ReadinessFor this class of change (schema/config additions in the boost workspace), the review agent provides informational value but is not ready for autonomous approval. The 25% false positive rate on documentation consistency findings (3 of 12) is too high — the "three-layer" recommendation would have been actively harmful if auto-applied. The agent's true positives were all non-blocking observations that didn't require changes before merge. Evidence for Existing Issues (no new proposals needed)
Overall AssessmentThe code agent produced a strong implementation on the first attempt — the architectural design (defaults as read-time fallback, not cache-layer), the test coverage (100% new code), and the schema versioning were all correct. The main rework was mechanical: missing changeset and API report, both attributable to missing workspace-level guidance. The review agent provided some useful observations but no blocking findings. The 5-day merge timeline was driven by human scheduling, not agent deficiencies. |
2 participants
Add optional
defaultValuefield toConfigFieldMetaand apply it as a third precedence layer inRuntimeConfigResolver: DB override → YAML baseline → field default → undefined.Default values per OpenSpec task 1.8:
Defaults are NOT added via Zod
.default()to preserve "unset" semantics:validateConfigValue(key, undefined)still returns undefined, and resolver precedence is respected. Instead, thedefaultValueproperty onConfigFieldMetais read by a newgetFieldDefault()helper, which the resolver calls when neither DB nor YAML provides a value.Fields without defaults (
enabled,endpoint,schedule.cron) continue to returnundefinedwhen absent.Bumps BOOST_CONFIG_SCHEMA_VERSION from 4 to 5.
Closes #4313
Post-script verification
agent/4313-connector-config-defaults)e6c850f7dd03d646d614fb0c74f80e7e4b9d0026..HEAD)