Repository navigation
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
I read the change but did not get to run it: the dependency install did not finish in time, so the reproduction from #54057, the tests and the type check were not run. From reading the code, the approach looks right. Theme names are mapped to hex at the load boundary and during v1 migration, the hex-only v2 schema stays unchanged, and the hex values match the default opencode theme's dark palette.
Public API: the config schema is unchanged. Agent color values that were already valid still are, and v1 theme names now load as default-theme hex values instead of being dropped (native files) or turned into #aaaaaa (legacy files). This is backwards compatible.
Things to be aware of:
- This reverses the choice made in #38414, which mapped named colors to
#aaaaaaon purpose. With this change, names resolve against the default theme's dark palette rather than the user's active theme, as v1 did. Maintainers should confirm that trade-off is acceptable. - Any other
colorthat is not a six-digit hex still drops the whole agent with nothing logged. That part of #54057 is not addressed here. resolveThemeColorusescolor in ThemeColorHex, which also matches inherited keys likeconstructor. It has no practical effect today, butObject.hasOwnwould be the correct check.
Related: #38414
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
372fb70 to
4ec2ef5
Compare
|
Thanks for the review, addressed below:
Tests: |
V1 accepted theme color names for an agent color (primary, �ccent, ...) but V2 only accepts six-digit hex, so migration replaced every theme name with the grey fallback \#aaaaaa\. Resolve the known names to the default theme's hex instead, leaving hex values and unknown colors untouched.
4ec2ef5 to
c61f8b0
Compare
|
Rebased onto |
There was a problem hiding this comment.
I ran the #54057 reproduction from source. On dev, a legacy agent file (with a v1 permission block) and color: warning migrates to #aaaaaa. With this PR it gets #f5a742. The agent config tests and the packages/core type check pass. The in check from my last review now uses Object.hasOwn, and #57910 has fixed the silent drop.
Public API: the config schema is unchanged. The v1 theme color names on migrated agents now become default-theme hex values instead of #aaaaaa. This is backwards compatible.
One thing for maintainers to decide: #57910 chose to ignore theme-name colors on native agent files, so they get no color and the TUI picks one from its rotating palette. With this PR, the same color: accent resolves to #9d7cd8 only when the file also has a v1-only key. Which color a user sees then depends on unrelated keys in the file. Either both paths should map names to hex, or both should drop them. Mapping is also a reversal of the #aaaaaa fallback chosen in #38414, and it uses the default theme's dark palette rather than the user's active theme.
Issue for this PR
Related to #54057 -- the "agent is silently dropped" half is already fixed upstream by #57910, so this PR now only covers the part that is still broken: V1 migration.
Type of change
What does this PR do?
V2 only accepts a six-digit hex for an agent
color, but V1 also accepted theme color names (primary,secondary,accent,success,warning,error,info). The V1 migration replaced every named color with the grey fallback#aaaaaa, so a migrated legacy agent lost its intended color even though the name is still present in the V1 schema.migrateAgentnow resolves the known theme names to the default theme's hex and leaves hex values untouched:packages/core/src/v1/config/agent.ts-- expose the theme color names plus the name -> hex table andresolveThemeColor.packages/core/src/v1/config/migrate.ts-- use it instead of the hard-coded#aaaaaa.packages/core/test/config/agent.test.ts-- the existing legacy file-based agent now usescolor: accentand asserts#9d7cd8.Native agent files are intentionally unchanged: #57910 made the V2 schema ignore unsupported colors, and this PR does not re-open that decision.
How did you verify your code works?
bun test test/config/agent.test.ts-> 36 pass / 0 fail.bun run typecheckinpackages/core-> clean.migrate.tshunk while keeping the new assertion fails with#aaaaaa, so the regression is covered.bun test test/confighas the same 6 failures on unmodifieddevin this environment (subagent background runs + global config discovery); they are unrelated to this change.Screenshots / recordings
N/A -- covered by the test above.
Checklist
Added by the opencode bot (AI-generated).
Related: #54057
Public API: v1 theme color names on migrated agents become default-theme hex instead of #aaaaaa (backwards compatible; schema unchanged)