Tamper-sealed Cargo orders: You Will (Not) Deliver#4764
Conversation
6c17f8a to
e2795e8
Compare
|
@coderabbitai Re-review |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Content.Server/Cargo/Systems/CargoSystem.Orders.cs (1)
681-693: 💤 Low valueNice work on the tamper seal integration! The conditional sealing logic is clean and the component wiring is well-structured.
Minor: Consider defensively copying the HashSet to avoid shared mutable state.
Line 693 assigns the
DestroyToolQualitiesHashSet by reference. If multiple entities are spawned from the same prototype, they'll share the same HashSet instance. While the downstream code only reads this set (never mutates it), a defensive copy would prevent subtle bugs if future changes introduce mutations.🛡️ Suggested defensive copy
- seal.DestroyToolQualities = tamperSealable.DestroyToolQualities; + seal.DestroyToolQualities = new HashSet<ProtoId<ToolQualityPrototype>>(tamperSealable.DestroyToolQualities);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Content.Server/Cargo/Systems/CargoSystem.Orders.cs` around lines 681 - 693, In the TamperSealComponent initialization block, the DestroyToolQualities property is being assigned by reference from the tamperSealable component. To prevent shared mutable state issues across multiple entities, apply a defensive copy pattern similar to what is already done for seal.Accesses. Replace the direct assignment of seal.DestroyToolQualities with a new HashSet instance that copies the contents from tamperSealable.DestroyToolQualities, ensuring each entity has its own independent copy of the collection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Content.Server/Cargo/Systems/CargoSystem.Orders.cs`:
- Around line 681-693: In the TamperSealComponent initialization block, the
DestroyToolQualities property is being assigned by reference from the
tamperSealable component. To prevent shared mutable state issues across multiple
entities, apply a defensive copy pattern similar to what is already done for
seal.Accesses. Replace the direct assignment of seal.DestroyToolQualities with a
new HashSet instance that copies the contents from
tamperSealable.DestroyToolQualities, ensuring each entity has its own
independent copy of the collection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 35e2cd0b-f6c4-48bb-a02f-9f3cb74b2e17
📒 Files selected for processing (10)
Content.Server/Cargo/Systems/CargoSystem.Orders.csContent.Shared/_Starlight/CCVar/StarlightCCVars.Cargo.csContent.Shared/_Starlight/Cargo/TamperSeal/Components/TamperSealComponent.csContent.Shared/_Starlight/Cargo/TamperSeal/Components/TamperSealValueComponent.csContent.Shared/_Starlight/Cargo/TamperSeal/Components/TamperSealableComponent.csContent.Shared/_Starlight/Cargo/TamperSeal/SharedTamperSealSystem.csResources/Locale/en-US/_Starlight/cargo/tamper-seal.ftlResources/Prototypes/Entities/Structures/Storage/Canisters/gas_canisters.ymlResources/Prototypes/Entities/Structures/Storage/Crates/base_structurecrates.ymlResources/Prototypes/Entities/Structures/Storage/Crates/crates.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- Content.Shared/_Starlight/CCVar/StarlightCCVars.Cargo.cs
- Content.Shared/_Starlight/Cargo/TamperSeal/Components/TamperSealValueComponent.cs
- Resources/Prototypes/Entities/Structures/Storage/Crates/base_structurecrates.yml
- Resources/Locale/en-US/_Starlight/cargo/tamper-seal.ftl
- Resources/Prototypes/Entities/Structures/Storage/Crates/crates.yml
- Content.Shared/_Starlight/Cargo/TamperSeal/Components/TamperSealComponent.cs
- Content.Shared/_Starlight/Cargo/TamperSeal/SharedTamperSealSystem.cs
|
@Rinary1 Need your review on this one since I made it |
Short description
Makes almost every type of order you can submit though cargo locked to the department that ordered it.
The only types of orders that aren't covered are large Science artifacts and large instruments.
Why we need to add this
TL;DR: Apparently it's a very common problem that Cargo fails to deliver, or even steals orders for themself (e.g.: Salvage taking advanced medkits that Medical ordered).
Media (Video/Screenshots)
Tamper-sealed containers
Three kinds of containers are supported, and will be equipped with a departmental tamper seal upon summoning:
Opening seals
2026-06-19.14-26-47.mp4
Examine text
Currently only when unsealed.
Integrity tracking
If >30% of orders have their tamper seals destroyed, the ITG informs the station.
Admin logging
Todos
DelivererAccount == RecipientAccountCount deconstruction of an entity as the seal being destroyed.Prevent deconstruction while tamper-sealed.Ensure cyborgs can only open crates belonging to their department?Opted not to do anything to this end for now.Checks
Changelog
🆑 redmushie