Skip to content

Conversation

@zanesq
Copy link
Collaborator

@zanesq zanesq commented Oct 28, 2025

Summary

add clippy warning for string_slice
closes #3476

@zanesq
Copy link
Collaborator Author

zanesq commented Oct 30, 2025

was hoping to just add warnings for now but its killing the ci linting, looking into it

@zanesq
Copy link
Collaborator Author

zanesq commented Oct 30, 2025

Goose says

The -D warnings flag in the CI script is promoting all warnings to errors, including your new string_slice warning.

When you set string_slice = "warn" in Cargo.toml, it enables the lint as a warning. But then the CI script runs with -D warnings, which says "deny (treat as error) ALL warnings" - including your new string_slice warnings.

So you have a few options:

Option 1: Fix the actual string slicing issues (the safest approach)

  • This is what we started doing earlier with the playback.rs fix

Option 2: Add the lint at the "deny" level instead of "warn"

  • Change string_slice = "warn" to string_slice = "deny" in Cargo.toml
  • This makes it explicit that it's an error, not just a warning being promoted

Option 3: Suppress existing violations with #[allow(clippy::string_slice)]

  • This is what we did with "Option B" earlier
  • Allows existing code to pass while catching new violations

Option 4: Modify the CI script to allow this specific warning

  • Add #[allow(clippy::string_slice)] at the crate level temporarily

Given that your PR intent is to enable the lint going forward, I'd recommend Option 1 (fix the issues) or Option 3 (suppress existing ones).

@DOsinga
Copy link
Collaborator

DOsinga commented Oct 31, 2025

yeah, I guess we need to fix all those occurrences

@DOsinga
Copy link
Collaborator

DOsinga commented Nov 4, 2025

I'll take this over

@jamadeo
Copy link
Collaborator

jamadeo commented Nov 4, 2025

@DOsinga you could use the baseline thing that @lifeizhou-ap set up if we want to merge this for future code without fixing all of the existing

@DOsinga
Copy link
Collaborator

DOsinga commented Nov 4, 2025

yeah we could, but I'll just have goose do it

@DOsinga DOsinga merged commit 89f7384 into main Nov 4, 2025
14 checks passed
wpfleger96 added a commit that referenced this pull request Nov 5, 2025
* main: (54 commits)
  add clippy warning for string_slice (#5422)
  improve linux tray icon support (#5425)
  feat: log rotation (#5561)
  use app.isPackaged instead of checking for node env development (#5465)
  disable RPM build-ID generation to prevent package conflicts (#5563)
  Add Diagnostics Info to Q&A and Bug Report Templates (#5565)
  fix: improve server error messages to include HTTP status code (#5532)
  improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300)
  fix: unblock acp via databricks (#5562)
  feat: add --output-format json flag to goose run command (#5525)
  Sessions required (#5548)
  feat: add grouped extension loading notification (#5529)
  we should run this on main and also test open models at least via ope… (#5556)
  info: print location of sessions.db via goose info (#5557)
  chore: remove yarn usage from documentation (#5555)
  cli: adjust default theme to address #1905 (#5552)
  Manual compaction counting fix + cli cleanup (#5480)
  chore(deps): bump prismjs and react-syntax-highlighter in /ui/desktop (#5549)
  fix: remove qwen3-coder from provider/mcp smoke tests (#5551)
  fix: do not build unsigned desktop app bundles on every PR in ci. add manual option. (#5550)
  ...
katzdave added a commit that referenced this pull request Nov 5, 2025
…est-and-fix

* 'main' of github.com:block/goose:
  Document missing recipe param types (#5584)
  docs: description required for "Add Extension" in cli (#5573)
  fix: Add schema-aware numeric coercion for MCP tool arguments (#5478)
  Add uv for uvx in Justfile (#5581)
  Keep llm logs in place (#5577)
  bump to 1.12.0 (#5580)
  automate more of the release process (#5409)
  add clippy warning for string_slice (#5422)
michaelneale added a commit that referenced this pull request Nov 6, 2025
* main: (41 commits)
  Add pending extension indicator to extension panel (#5493)
  Add environment subsition for auth blocks (#5439)
  acp: ToolCallLocations and working cancellation (#5588)
  feat(providers): add Mistral AI provider (#5009)
  Listen for ctrl-c during provider request (#5585)
  Also accept null as description, not just missing (#5589)
  Document missing recipe param types (#5584)
  docs: description required for "Add Extension" in cli (#5573)
  fix: Add schema-aware numeric coercion for MCP tool arguments (#5478)
  Add uv for uvx in Justfile (#5581)
  Keep llm logs in place (#5577)
  bump to 1.12.0 (#5580)
  automate more of the release process (#5409)
  add clippy warning for string_slice (#5422)
  improve linux tray icon support (#5425)
  feat: log rotation (#5561)
  use app.isPackaged instead of checking for node env development (#5465)
  disable RPM build-ID generation to prevent package conflicts (#5563)
  Add Diagnostics Info to Q&A and Bug Report Templates (#5565)
  fix: improve server error messages to include HTTP status code (#5532)
  ...
michaelneale added a commit that referenced this pull request Nov 6, 2025
* main: (53 commits)
  acp: ToolCallLocations and working cancellation (#5588)
  feat(providers): add Mistral AI provider (#5009)
  Listen for ctrl-c during provider request (#5585)
  Also accept null as description, not just missing (#5589)
  Document missing recipe param types (#5584)
  docs: description required for "Add Extension" in cli (#5573)
  fix: Add schema-aware numeric coercion for MCP tool arguments (#5478)
  Add uv for uvx in Justfile (#5581)
  Keep llm logs in place (#5577)
  bump to 1.12.0 (#5580)
  automate more of the release process (#5409)
  add clippy warning for string_slice (#5422)
  improve linux tray icon support (#5425)
  feat: log rotation (#5561)
  use app.isPackaged instead of checking for node env development (#5465)
  disable RPM build-ID generation to prevent package conflicts (#5563)
  Add Diagnostics Info to Q&A and Bug Report Templates (#5565)
  fix: improve server error messages to include HTTP status code (#5532)
  improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300)
  fix: unblock acp via databricks (#5562)
  ...
aharvard added a commit that referenced this pull request Nov 6, 2025
* origin/main: (75 commits)
  fix: customised recipe to yaml string to avoid minininjia parsing error (#5494)
  Add pending extension indicator to extension panel (#5493)
  Add environment subsition for auth blocks (#5439)
  acp: ToolCallLocations and working cancellation (#5588)
  feat(providers): add Mistral AI provider (#5009)
  Listen for ctrl-c during provider request (#5585)
  Also accept null as description, not just missing (#5589)
  Document missing recipe param types (#5584)
  docs: description required for "Add Extension" in cli (#5573)
  fix: Add schema-aware numeric coercion for MCP tool arguments (#5478)
  Add uv for uvx in Justfile (#5581)
  Keep llm logs in place (#5577)
  bump to 1.12.0 (#5580)
  automate more of the release process (#5409)
  add clippy warning for string_slice (#5422)
  improve linux tray icon support (#5425)
  feat: log rotation (#5561)
  use app.isPackaged instead of checking for node env development (#5465)
  disable RPM build-ID generation to prevent package conflicts (#5563)
  Add Diagnostics Info to Q&A and Bug Report Templates (#5565)
  ...
fbalicchia pushed a commit to fbalicchia/goose that referenced this pull request Nov 7, 2025
Co-authored-by: Douwe Osinga <[email protected]>
Signed-off-by: fbalicchia <[email protected]>
BlairAllan pushed a commit to BlairAllan/goose that referenced this pull request Nov 29, 2025
Co-authored-by: Douwe Osinga <[email protected]>
Signed-off-by: Blair Allan <[email protected]>
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.

clippy shuld check clippy::string_slice

6 participants