[codex] Push-and-green functional closure finalization#89
Conversation
|
Unable to trigger custom agent "Code Reviewer". You have run out of credits 😔 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces deterministic native host bootstrap, multi-payload promoted action matrix semantics (notably Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Push-and-green closure evidence update (2026-03-01) Deterministic gate:
Live evidence bundles (strict schema validated):
Live matrix summary:
Promoted matrix contract note:
|
Codacy's Analysis Summary0 new issue (≤ 0 issue) ✨ AI Reviewer: run the reviewer on demand. As new changes are pushed, run a review below. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
native/SwfocExtender.Plugins/src/GlobalTogglePlugin.cpp (1)
173-193:⚠️ Potential issue | 🟠 MajorAtomic state updated before write verification - may cause state inconsistency.
Lines 174-180 update the atomic state variables (
freezeTimerEnabled_,fogRevealEnabled_,aiEnabled_) before the address parsing and process write occur (Lines 182-191). If the write fails, the internal state will reflect the intended value rather than the actual state.Consider moving the atomic store after the successful write:
Proposed fix
- const bool nextValue = request.boolValue; - if (request.featureId == "freeze_timer") { - freezeTimerEnabled_.store(nextValue); - } else if (request.featureId == "toggle_fog_reveal") { - fogRevealEnabled_.store(nextValue); - } else { - aiEnabled_.store(nextValue); - } - std::uintptr_t targetAddress = 0; if (!process_mutation::TryParseAddress(resolvedAnchor->second, targetAddress)) { return BuildInvalidAnchorResult(request, *resolvedAnchor); } std::string writeError; + const bool nextValue = request.boolValue; const auto encoded = static_cast<std::uint8_t>(nextValue ? 1 : 0); if (!process_mutation::TryWriteValue<std::uint8_t>(request.processId, targetAddress, encoded, writeError)) { return BuildWriteFailureResult(request, *resolvedAnchor, nextValue, writeError); } + if (request.featureId == "freeze_timer") { + freezeTimerEnabled_.store(nextValue); + } else if (request.featureId == "toggle_fog_reveal") { + fogRevealEnabled_.store(nextValue); + } else { + aiEnabled_.store(nextValue); + } + return BuildMutationSuccessResult(request, *resolvedAnchor, nextValue);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/SwfocExtender.Plugins/src/GlobalTogglePlugin.cpp` around lines 173 - 193, The atomic flags (freezeTimerEnabled_, fogRevealEnabled_, aiEnabled_) are being stored before verifying the memory write, causing internal state to diverge if TryParseAddress or process_mutation::TryWriteValue fails; move the stores so they occur only after process_mutation::TryWriteValue succeeds (i.e., after the writeError check and before returning BuildMutationSuccessResult), using the same nextValue computed from request.boolValue and keeping all existing error returns (BuildInvalidAnchorResult, BuildWriteFailureResult) unchanged.
🧹 Nitpick comments (5)
src/SwfocTrainer.Runtime/Services/NamedPipeExtenderBackend.cs (1)
151-160: Make probe-anchor merge tolerant to non-string JSON values.
GetValue<string>()can throw and drop the full merge path if one value is non-string.ToString()keeps merging resilient.♻️ Proposed change
private static void AppendNonEmptyAnchorValues(JsonObject sourceAnchors, JsonObject destinationAnchors) { foreach (var kv in sourceAnchors) { - var normalized = kv.Value?.GetValue<string>(); + var normalized = kv.Value?.ToString(); if (!string.IsNullOrWhiteSpace(normalized)) { destinationAnchors[kv.Key] = normalized; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SwfocTrainer.Runtime/Services/NamedPipeExtenderBackend.cs` around lines 151 - 160, In AppendNonEmptyAnchorValues, using kv.Value?.GetValue<string>() can throw for non-string JSON nodes and abort the merge; change it to serialize the value safely by using kv.Value?.ToString(), trim it, and only assign destinationAnchors[kv.Key] when the resulting string is not null/whitespace (e.g., var normalized = kv.Value?.ToString()?.Trim(); if (!string.IsNullOrWhiteSpace(normalized)) destinationAnchors[kv.Key] = normalized;). This makes the probe-anchor merge tolerant to non-string JSON values.native/SwfocExtender.Plugins/include/swfoc_extender/plugins/ProcessMutationHelpers.hpp (1)
72-73: Clearerroron successful writes to avoid stale diagnostics.Callers can otherwise observe an old error message even when the current write succeeded.
🧹 Proposed tweak
- return true; + error.clear(); + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/SwfocExtender.Plugins/include/swfoc_extender/plugins/ProcessMutationHelpers.hpp` around lines 72 - 73, In the success path that currently just does "return true;" in ProcessMutationHelpers (the branch shown before the `#else`), clear/reset the error diagnostic so callers don't see stale messages—specifically clear the variable named "error" (e.g., call error.clear() or reset the optional/error-holder used in this code) just before returning true so successful writes remove any prior error state.tests/SwfocTrainer.Tests/Runtime/ProcessLocatorForcedContextTests.cs (1)
71-107: Reflection-based test helpers are fragile but acceptable.Using reflection to invoke private static methods (
ResolveForcedContext,ResolveOptionsFromEnvironment) makes tests fragile to internal refactoring. However, this is a reasonable tradeoff for testing critical internal behavior without exposing it publicly.Consider adding a comment explaining why reflection is used and which methods are being tested:
// Tests invoke private ProcessLocator methods via reflection to verify // forced-context resolution logic without exposing internal APIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SwfocTrainer.Tests/Runtime/ProcessLocatorForcedContextTests.cs` around lines 71 - 107, Add a brief explanatory comment above the reflection-based helpers describing why private static methods are invoked via reflection (to test internal forced-context logic without making APIs public) and list the target methods being tested (ResolveForcedContext and ResolveOptionsFromEnvironment); place the comment near the helper methods InvokeResolveForcedContext and InvokeResolveOptionsFromEnvironment so future maintainers understand the tradeoff and intent.tools/collect-mod-repro-bundle.ps1 (1)
209-214: LASTEXITCODE check may not reliably capture the exit code.Using
Get-Variable -Name LASTEXITCODE -Scope Global -ErrorAction SilentlyContinuecan miss the exit code if the native command doesn't set it, or may return a stale value from a previous command. The2>&1redirection captures stderr into stdout, which changes how PowerShell handles errors.Consider using
$LASTEXITCODEdirectly after the call:Proposed fix
try { $output = & $Command[0] `@invocationArgs` 2>&1 + $exitCode = if ($null -ne $LASTEXITCODE) { [int]$LASTEXITCODE } else { 0 } return [PSCustomObject]@{ - ExitCode = if (Get-Variable -Name LASTEXITCODE -Scope Global -ErrorAction SilentlyContinue) { [int]$global:LASTEXITCODE } else { 0 } + ExitCode = $exitCode Output = ($output | Out-String) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/collect-mod-repro-bundle.ps1` around lines 209 - 214, The current try block captures command output into $output and attempts to read LASTEXITCODE via Get-Variable which can be stale; instead, immediately after invoking the external command (& $Command[0] `@invocationArgs` 2>&1) read the automatic $LASTEXITCODE into the ExitCode field (e.g. assign $ExitCode = [int]($LASTEXITCODE -or 0)) to reliably capture the native process exit code; update the PSCustomObject creation that sets ExitCode to use that direct $LASTEXITCODE value and keep Output = ($output | Out-String) so $output and $LASTEXITCODE are captured in the same scope for functions using $Command, $invocationArgs and $output.native/SwfocExtender.Plugins/src/BuildPatchPlugin.cpp (1)
230-241: Potential state inconsistency on write failure.
ApplyUnitCapState/ApplyInstantBuildStateare called at lines 230-232 before the actual process mutation is attempted. If the subsequentTryWriteValuefails (lines 252-253 or 260-261), the internal atomic state (unitCapPatchEnabled_,instantBuildPatchEnabled_) will indicate "enabled" even though the process wasn't actually patched.Consider either:
- Moving the state updates after successful writes, or
- Rolling back state on write failure
♻️ Suggested reorder: update state only on success
if (request.featureId == "set_unit_cap") { if (IsUnitCapOutOfBounds(request, enablePatch)) { return BuildInvalidUnitCapResult(request); } - - ApplyUnitCapState(enablePatch, request.intValue); - } else { - ApplyInstantBuildState(enablePatch); } if (!enablePatch) { + if (request.featureId == "set_unit_cap") { + ApplyUnitCapState(false, request.intValue); + } else { + ApplyInstantBuildState(false); + } return BuildDisableNoOpSuccessResult(request, enablePatch, resolvedAnchor); } // ... validation and write logic ... if (request.featureId == "set_unit_cap") { // ... write logic ... + ApplyUnitCapState(true, clamped); return BuildMutationSuccessResult(request, *resolvedAnchor, enablePatch, clamped); } // ... instant_build write logic ... + ApplyInstantBuildState(true); return BuildMutationSuccessResult(request, *resolvedAnchor, enablePatch, 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/SwfocExtender.Plugins/src/BuildPatchPlugin.cpp` around lines 230 - 241, The code updates internal flags before attempting the external patch; move the state mutation to occur only after a successful TryWriteValue (or, if you prefer to keep the current order, perform a rollback by reverting unitCapPatchEnabled_ or instantBuildPatchEnabled_ when TryWriteValue fails). Concretely: in the patch path that currently calls ApplyUnitCapState(...) or ApplyInstantBuildState(...), defer those calls until after the corresponding TryWriteValue(...) succeeds, or add explicit reversal calls to ApplyUnitCapState(false, ...) / ApplyInstantBuildState(false) in the error handling branch that handles TryWriteValue failures so unitCapPatchEnabled_ and instantBuildPatchEnabled_ always reflect actual write success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@native/SwfocExtender.Plugins/include/swfoc_extender/plugins/ProcessMutationHelpers.hpp`:
- Around line 31-40: The parsed 64-bit value "parsed" may exceed the width of
std::uintptr_t and get silently truncated when assigned to "address"; before
casting in the code inside ProcessMutationHelpers.hpp (the block using
std::from_chars and assigning to address), add a bounds check that the parsed
value fits in std::uintptr_t (compare parsed <=
std::numeric_limits<std::uintptr_t>::max()), return false if it does not, then
perform the static_cast to std::uintptr_t; include <limits> if not present so
the check can be performed.
In `@native/SwfocExtender.Plugins/src/BuildPatchPlugin.cpp`:
- Around line 249-257: In the set_unit_cap handling (when request.featureId ==
"set_unit_cap") the write clamps to 0..255 but validation and capability
reporting use kMinUnitCap=1 and kMaxUnitCap=100000; change the
validation/constants so the allowed range matches the 8-bit memory target (set
kMaxUnitCap to 255 and ensure kMinUnitCap is 1), update the clamp to use those
constants instead of hardcoded 0/255, and ensure the capability snapshot and any
validation logic reference kMinUnitCap/kMaxUnitCap so requests rejected/accepted
and reported bounds are consistent with process_mutation::TryWriteValue and the
value passed to BuildMutationSuccessResult.
In `@tools/collect-mod-repro-bundle.ps1`:
- Around line 74-82: The current fallback unconditionally returns the WSL
command ($wsl.Source + "-e python3") even if it was already tested and failed;
change the logic in the block after the foreach so that you only return the WSL
fallback if it actually works: call Test-PythonInterpreter with the WSL
invocation (or ensure $wsl.Source wasn't already in $candidates) and only return
@($wsl.Source, "-e", "python3") when that test succeeds; if it fails, return an
empty array (@()) instead. This modifies the post-loop behavior around
$candidates, Test-PythonInterpreter and $wsl to avoid returning a non-functional
interpreter.
In `@tools/native/build-native.ps1`:
- Around line 36-38: The throw in the Path validation if-block (the check using
[string]::IsNullOrWhiteSpace($Path)) must use a stable machine-readable
classification prefix; replace the bare throw message ("Path cannot be empty.")
with a prefixed error code (e.g. "ERR_PATH_EMPTY: Path cannot be empty.") and
apply the same pattern to the other new throw sites referenced in the review
(the throws around the other validation blocks noted). Ensure the unique prefix
is consistent, descriptive, and preserved in all three throw messages so
callers/telemetry can reliably classify the failure.
- Around line 77-80: The fallback selection of $recursiveArtifact from
$recursiveHits is nondeterministic because it takes the first hit returned by
Get-ChildItem without ordering; change the selection to sort $recursiveHits
deterministically (for example by LastWriteTime desc or FullName asc) and then
pick the first item so $recursiveArtifact is stable across runs—i.e., replace
the current Select-Object -ExpandProperty FullName -First 1 usage with a sorted
selection (Sort-Object on LastWriteTime or FullName followed by Select-Object
-First 1) to ensure reproducible artifact choice.
In `@tools/run-live-validation.ps1`:
- Around line 292-303: Update the three throw statements in the WSL path
conversion block to include stable classification codes so automation can parse
failures: change the "wsl executable not available" throw (near $wsl.Source
check) to include a code like "ERR-WSL-NOTFOUND:"; change the "Failed to convert
... using wslpath." throw (after checking $exitCode from Get-LastExitCodeOrZero)
to include a code like "ERR-WSL-WSLPATH:" and include the exitCode variable
where available; and change the "WSL path conversion produced no output" throw
(after checking $text) to include a code like "ERR-WSL-NOOUTPUT:". Ensure each
modified throw keeps the original human-readable message after the code for
context.
- Around line 328-337: The script currently throws on a non-zero $buildExitCode
before replaying $buildOutput, losing valuable build diagnostics; modify the
control flow in the block that runs & $buildScript (using $buildOutput and
Get-LastExitCodeOrZero) so that you always iterate over $buildOutput and call
Write-Information for non-empty lines (the foreach over @($buildOutput) with the
IsNullOrWhiteSpace check) before throwing the ATTACH_NATIVE_HOST_PRECHECK_FAILED
error when $buildExitCode -ne 0; keep the same diagnostic formatting and only
throw after replaying the captured output.
---
Outside diff comments:
In `@native/SwfocExtender.Plugins/src/GlobalTogglePlugin.cpp`:
- Around line 173-193: The atomic flags (freezeTimerEnabled_, fogRevealEnabled_,
aiEnabled_) are being stored before verifying the memory write, causing internal
state to diverge if TryParseAddress or process_mutation::TryWriteValue fails;
move the stores so they occur only after process_mutation::TryWriteValue
succeeds (i.e., after the writeError check and before returning
BuildMutationSuccessResult), using the same nextValue computed from
request.boolValue and keeping all existing error returns
(BuildInvalidAnchorResult, BuildWriteFailureResult) unchanged.
---
Nitpick comments:
In
`@native/SwfocExtender.Plugins/include/swfoc_extender/plugins/ProcessMutationHelpers.hpp`:
- Around line 72-73: In the success path that currently just does "return true;"
in ProcessMutationHelpers (the branch shown before the `#else`), clear/reset the
error diagnostic so callers don't see stale messages—specifically clear the
variable named "error" (e.g., call error.clear() or reset the
optional/error-holder used in this code) just before returning true so
successful writes remove any prior error state.
In `@native/SwfocExtender.Plugins/src/BuildPatchPlugin.cpp`:
- Around line 230-241: The code updates internal flags before attempting the
external patch; move the state mutation to occur only after a successful
TryWriteValue (or, if you prefer to keep the current order, perform a rollback
by reverting unitCapPatchEnabled_ or instantBuildPatchEnabled_ when
TryWriteValue fails). Concretely: in the patch path that currently calls
ApplyUnitCapState(...) or ApplyInstantBuildState(...), defer those calls until
after the corresponding TryWriteValue(...) succeeds, or add explicit reversal
calls to ApplyUnitCapState(false, ...) / ApplyInstantBuildState(false) in the
error handling branch that handles TryWriteValue failures so
unitCapPatchEnabled_ and instantBuildPatchEnabled_ always reflect actual write
success.
In `@src/SwfocTrainer.Runtime/Services/NamedPipeExtenderBackend.cs`:
- Around line 151-160: In AppendNonEmptyAnchorValues, using
kv.Value?.GetValue<string>() can throw for non-string JSON nodes and abort the
merge; change it to serialize the value safely by using kv.Value?.ToString(),
trim it, and only assign destinationAnchors[kv.Key] when the resulting string is
not null/whitespace (e.g., var normalized = kv.Value?.ToString()?.Trim(); if
(!string.IsNullOrWhiteSpace(normalized)) destinationAnchors[kv.Key] =
normalized;). This makes the probe-anchor merge tolerant to non-string JSON
values.
In `@tests/SwfocTrainer.Tests/Runtime/ProcessLocatorForcedContextTests.cs`:
- Around line 71-107: Add a brief explanatory comment above the reflection-based
helpers describing why private static methods are invoked via reflection (to
test internal forced-context logic without making APIs public) and list the
target methods being tested (ResolveForcedContext and
ResolveOptionsFromEnvironment); place the comment near the helper methods
InvokeResolveForcedContext and InvokeResolveOptionsFromEnvironment so future
maintainers understand the tradeoff and intent.
In `@tools/collect-mod-repro-bundle.ps1`:
- Around line 209-214: The current try block captures command output into
$output and attempts to read LASTEXITCODE via Get-Variable which can be stale;
instead, immediately after invoking the external command (& $Command[0]
`@invocationArgs` 2>&1) read the automatic $LASTEXITCODE into the ExitCode field
(e.g. assign $ExitCode = [int]($LASTEXITCODE -or 0)) to reliably capture the
native process exit code; update the PSCustomObject creation that sets ExitCode
to use that direct $LASTEXITCODE value and keep Output = ($output | Out-String)
so $output and $LASTEXITCODE are captured in the same scope for functions using
$Command, $invocationArgs and $output.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
TODO.mddocs/LIVE_VALIDATION_RUNBOOK.mdnative/SwfocExtender.Plugins/include/swfoc_extender/plugins/ProcessMutationHelpers.hppnative/SwfocExtender.Plugins/src/BuildPatchPlugin.cppnative/SwfocExtender.Plugins/src/EconomyPlugin.cppnative/SwfocExtender.Plugins/src/GlobalTogglePlugin.cppsrc/SwfocTrainer.Runtime/Services/NamedPipeExtenderBackend.cssrc/SwfocTrainer.Runtime/Services/RuntimeAdapter.cstests/SwfocTrainer.Tests/Profiles/LiveHeroHelperWorkflowTests.cstests/SwfocTrainer.Tests/Profiles/LivePromotedActionMatrixTests.cstests/SwfocTrainer.Tests/Runtime/NamedPipeExtenderBackendTests.cstests/SwfocTrainer.Tests/Runtime/ProcessLocatorForcedContextTests.cstools/collect-mod-repro-bundle.ps1tools/native/build-native.ps1tools/run-live-validation.ps1
| unsigned long long parsed = 0; | ||
| const auto* begin = normalized.data(); | ||
| const auto* end = begin + normalized.size(); | ||
| const auto [ptr, ec] = std::from_chars(begin, end, parsed, 16); | ||
| if (ec != std::errc() || ptr != end) { | ||
| return false; | ||
| } | ||
|
|
||
| address = static_cast<std::uintptr_t>(parsed); | ||
| return true; |
There was a problem hiding this comment.
Guard against address truncation before casting to std::uintptr_t.
A parsed 64-bit value can be silently truncated on narrower std::uintptr_t targets, which can redirect writes to the wrong address.
🛠️ Proposed fix
`#include` <charconv>
`#include` <cstdint>
+#include <limits>
`#include` <string>
`#include` <string_view>
@@
const auto [ptr, ec] = std::from_chars(begin, end, parsed, 16);
if (ec != std::errc() || ptr != end) {
return false;
}
+ if (parsed > static_cast<unsigned long long>(std::numeric_limits<std::uintptr_t>::max())) {
+ return false;
+ }
+
address = static_cast<std::uintptr_t>(parsed);
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsigned long long parsed = 0; | |
| const auto* begin = normalized.data(); | |
| const auto* end = begin + normalized.size(); | |
| const auto [ptr, ec] = std::from_chars(begin, end, parsed, 16); | |
| if (ec != std::errc() || ptr != end) { | |
| return false; | |
| } | |
| address = static_cast<std::uintptr_t>(parsed); | |
| return true; | |
| unsigned long long parsed = 0; | |
| const auto* begin = normalized.data(); | |
| const auto* end = begin + normalized.size(); | |
| const auto [ptr, ec] = std::from_chars(begin, end, parsed, 16); | |
| if (ec != std::errc() || ptr != end) { | |
| return false; | |
| } | |
| if (parsed > static_cast<unsigned long long>(std::numeric_limits<std::uintptr_t>::max())) { | |
| return false; | |
| } | |
| address = static_cast<std::uintptr_t>(parsed); | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@native/SwfocExtender.Plugins/include/swfoc_extender/plugins/ProcessMutationHelpers.hpp`
around lines 31 - 40, The parsed 64-bit value "parsed" may exceed the width of
std::uintptr_t and get silently truncated when assigned to "address"; before
casting in the code inside ProcessMutationHelpers.hpp (the block using
std::from_chars and assigning to address), add a bounds check that the parsed
value fits in std::uintptr_t (compare parsed <=
std::numeric_limits<std::uintptr_t>::max()), return false if it does not, then
perform the static_cast to std::uintptr_t; include <limits> if not present so
the check can be performed.
| if (request.featureId == "set_unit_cap") { | ||
| const auto clamped = std::clamp(request.intValue, 0, 255); | ||
| const auto encoded = static_cast<std::uint8_t>(clamped); | ||
| if (!process_mutation::TryWriteValue<std::uint8_t>(request.processId, targetAddress, encoded, writeError)) { | ||
| return BuildWriteFailureResult(request, *resolvedAnchor, enablePatch, writeError); | ||
| } | ||
|
|
||
| return BuildMutationSuccessResult(request, *resolvedAnchor, enablePatch, clamped); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's get the file and check its size
wc -l native/SwfocExtender.Plugins/src/BuildPatchPlugin.cppRepository: Prekzursil/SWFOC-Mod-Menu
Length of output: 126
🏁 Script executed:
# Search for kMaxUnitCap and kMinUnitCap definitions and usages
rg -n "kMaxUnitCap|kMinUnitCap" --type cppRepository: Prekzursil/SWFOC-Mod-Menu
Length of output: 642
🏁 Script executed:
# Look for unit_cap related code and validation paths
rg -n "unit_cap|unitCap" --type cpp -A 2 -B 2Repository: Prekzursil/SWFOC-Mod-Menu
Length of output: 7400
🏁 Script executed:
# Check the specific lines mentioned in the review (25-26, 115, 226, 249-257)
sed -n '20,30p' native/SwfocExtender.Plugins/src/BuildPatchPlugin.cppRepository: Prekzursil/SWFOC-Mod-Menu
Length of output: 563
🏁 Script executed:
# Check the validation function around line 115
sed -n '110,125p' native/SwfocExtender.Plugins/src/BuildPatchPlugin.cppRepository: Prekzursil/SWFOC-Mod-Menu
Length of output: 621
🏁 Script executed:
# Check the entire mutation flow from line 220-260
sed -n '220,265p' native/SwfocExtender.Plugins/src/BuildPatchPlugin.cppRepository: Prekzursil/SWFOC-Mod-Menu
Length of output: 1801
Bounds mismatch between validation (1–100000) and memory write clamping (0–255).
The validation at line 226 accepts intValue in range [kMinUnitCap=1, kMaxUnitCap=100000], and the capability snapshot reports these bounds to clients (lines 84–85). However, the actual memory write at line 250 clamps to [0, 255]:
const auto clamped = std::clamp(request.intValue, 0, 255);This creates two problems:
- Silent clamping for large values: A request with
intValue=50000passes validation but gets silently clamped to255when written to memory. The success result reportsclamped(255), not the original request. - Lower bound inconsistency: Validation rejects
intValue=0(belowkMinUnitCap), but the clamp allows it.
If the 8-bit encoding is intentional (target memory is a single byte), align the validation bounds to [1, 255] to match the actual write constraints and prevent misleading capability snapshots.
🔧 Suggested fix to align validation and write bounds
-constexpr std::int32_t kMinUnitCap = 1;
-constexpr std::int32_t kMaxUnitCap = 100000;
+constexpr std::int32_t kMinUnitCap = 1;
+constexpr std::int32_t kMaxUnitCap = 255;And use the constants in the clamp:
- const auto clamped = std::clamp(request.intValue, 0, 255);
+ const auto clamped = std::clamp(request.intValue, kMinUnitCap, kMaxUnitCap);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/SwfocExtender.Plugins/src/BuildPatchPlugin.cpp` around lines 249 -
257, In the set_unit_cap handling (when request.featureId == "set_unit_cap") the
write clamps to 0..255 but validation and capability reporting use kMinUnitCap=1
and kMaxUnitCap=100000; change the validation/constants so the allowed range
matches the 8-bit memory target (set kMaxUnitCap to 255 and ensure kMinUnitCap
is 1), update the clamp to use those constants instead of hardcoded 0/255, and
ensure the capability snapshot and any validation logic reference
kMinUnitCap/kMaxUnitCap so requests rejected/accepted and reported bounds are
consistent with process_mutation::TryWriteValue and the value passed to
BuildMutationSuccessResult.
| foreach ($candidate in $candidates) { | ||
| if (Test-PythonInterpreter -Command $candidate) { | ||
| return $candidate | ||
| } | ||
| } | ||
|
|
||
| if ($null -ne $wsl) { | ||
| return @($wsl.Source, "-e", "python3") | ||
| } |
There was a problem hiding this comment.
WSL fallback returned unconditionally after all candidate tests fail.
The loop at Lines 74-78 tests all candidates via Test-PythonInterpreter and returns the first valid one. However, if none pass, Line 80-82 returns the WSL command anyway as a fallback, even though it was already tested and failed (if WSL was in the candidates list at Line 71). This could result in returning a non-functional interpreter.
Consider returning an empty array if all candidates fail, including WSL:
Proposed fix
foreach ($candidate in $candidates) {
if (Test-PythonInterpreter -Command $candidate) {
return $candidate
}
}
- if ($null -ne $wsl) {
- return @($wsl.Source, "-e", "python3")
- }
-
return @()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/collect-mod-repro-bundle.ps1` around lines 74 - 82, The current
fallback unconditionally returns the WSL command ($wsl.Source + "-e python3")
even if it was already tested and failed; change the logic in the block after
the foreach so that you only return the WSL fallback if it actually works: call
Test-PythonInterpreter with the WSL invocation (or ensure $wsl.Source wasn't
already in $candidates) and only return @($wsl.Source, "-e", "python3") when
that test succeeds; if it fails, return an empty array (@()) instead. This
modifies the post-loop behavior around $candidates, Test-PythonInterpreter and
$wsl to avoid returning a non-functional interpreter.
| if ([string]::IsNullOrWhiteSpace($Path)) { | ||
| throw "Path cannot be empty." | ||
| } |
There was a problem hiding this comment.
Use explicit classification codes for new failure throws.
These newly introduced failure paths should carry stable machine-readable prefixes.
🏷️ Proposed classification prefixes
- throw "Path cannot be empty."
+ throw "NATIVE_PATH_INVALID: Path cannot be empty."
@@
- throw "native fallback build failed for Windows mode."
+ throw "NATIVE_BUILD_FAILED: native fallback build failed for Windows mode."
@@
- throw "native build completed but SwfocExtender.Host.exe artifact was not found under '$resolvedOutDir'."
+ throw "NATIVE_ARTIFACT_MISSING: native build completed but SwfocExtender.Host.exe artifact was not found under '$resolvedOutDir'."As per coding guidelines "Failure conditions must use explicit classification codes".
Also applies to: 203-204, 210-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/native/build-native.ps1` around lines 36 - 38, The throw in the Path
validation if-block (the check using [string]::IsNullOrWhiteSpace($Path)) must
use a stable machine-readable classification prefix; replace the bare throw
message ("Path cannot be empty.") with a prefixed error code (e.g.
"ERR_PATH_EMPTY: Path cannot be empty.") and apply the same pattern to the other
new throw sites referenced in the review (the throws around the other validation
blocks noted). Ensure the unique prefix is consistent, descriptive, and
preserved in all three throw messages so callers/telemetry can reliably classify
the failure.
| $recursiveHits = @(Get-ChildItem -Path $OutDir -Filter "SwfocExtender.Host.exe" -File -Recurse -ErrorAction SilentlyContinue) | ||
| $searchCount += $recursiveHits.Count | ||
| $recursiveArtifact = $recursiveHits | Select-Object -ExpandProperty FullName -First 1 | ||
| if (-not [string]::IsNullOrWhiteSpace($recursiveArtifact)) { |
There was a problem hiding this comment.
Recursive artifact fallback is currently nondeterministic.
Selecting the first recursive hit without ordering can pick stale artifacts unpredictably between runs.
🛠️ Proposed deterministic selection
- $recursiveHits = @(Get-ChildItem -Path $OutDir -Filter "SwfocExtender.Host.exe" -File -Recurse -ErrorAction SilentlyContinue)
+ $recursiveHits = @(
+ Get-ChildItem -Path $OutDir -Filter "SwfocExtender.Host.exe" -File -Recurse -ErrorAction SilentlyContinue |
+ Sort-Object `
+ @{ Expression = { $_.LastWriteTimeUtc }; Descending = $true }, `
+ @{ Expression = { $_.FullName }; Descending = $false }
+ )
$searchCount += $recursiveHits.Count
$recursiveArtifact = $recursiveHits | Select-Object -ExpandProperty FullName -First 1As per coding guidelines "Script outputs must be stable and schema-backed where practical".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/native/build-native.ps1` around lines 77 - 80, The fallback selection
of $recursiveArtifact from $recursiveHits is nondeterministic because it takes
the first hit returned by Get-ChildItem without ordering; change the selection
to sort $recursiveHits deterministically (for example by LastWriteTime desc or
FullName asc) and then pick the first item so $recursiveArtifact is stable
across runs—i.e., replace the current Select-Object -ExpandProperty FullName
-First 1 usage with a sorted selection (Sort-Object on LastWriteTime or FullName
followed by Select-Object -First 1) to ensure reproducible artifact choice.
| throw "WSL command was requested for python fallback but wsl executable is not available." | ||
| } | ||
|
|
||
| $converted = & $wsl.Source -e wslpath -a $Path 2>$null | ||
| $exitCode = Get-LastExitCodeOrZero | ||
| if ($exitCode -ne 0) { | ||
| throw "Failed to convert '$Path' into a WSL path using wslpath." | ||
| } | ||
|
|
||
| $text = ($converted | Out-String).Trim() | ||
| if ([string]::IsNullOrWhiteSpace($text)) { | ||
| throw "WSL path conversion produced no output for '$Path'." |
There was a problem hiding this comment.
Add classification codes to new WSL conversion failure throws.
These failure branches should expose stable reason prefixes for automation and repro triage.
🏷️ Proposed error-code prefixes
- throw "WSL command was requested for python fallback but wsl executable is not available."
+ throw "PYTHON_WSL_UNAVAILABLE: WSL command was requested for python fallback but wsl executable is not available."
@@
- throw "Failed to convert '$Path' into a WSL path using wslpath."
+ throw "PYTHON_WSLPATH_FAILED: Failed to convert '$Path' into a WSL path using wslpath."
@@
- throw "WSL path conversion produced no output for '$Path'."
+ throw "PYTHON_WSLPATH_EMPTY: WSL path conversion produced no output for '$Path'."As per coding guidelines "Failure conditions must use explicit classification codes".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw "WSL command was requested for python fallback but wsl executable is not available." | |
| } | |
| $converted = & $wsl.Source -e wslpath -a $Path 2>$null | |
| $exitCode = Get-LastExitCodeOrZero | |
| if ($exitCode -ne 0) { | |
| throw "Failed to convert '$Path' into a WSL path using wslpath." | |
| } | |
| $text = ($converted | Out-String).Trim() | |
| if ([string]::IsNullOrWhiteSpace($text)) { | |
| throw "WSL path conversion produced no output for '$Path'." | |
| throw "PYTHON_WSL_UNAVAILABLE: WSL command was requested for python fallback but wsl executable is not available." | |
| } | |
| $converted = & $wsl.Source -e wslpath -a $Path 2>$null | |
| $exitCode = Get-LastExitCodeOrZero | |
| if ($exitCode -ne 0) { | |
| throw "PYTHON_WSLPATH_FAILED: Failed to convert '$Path' into a WSL path using wslpath." | |
| } | |
| $text = ($converted | Out-String).Trim() | |
| if ([string]::IsNullOrWhiteSpace($text)) { | |
| throw "PYTHON_WSLPATH_EMPTY: WSL path conversion produced no output for '$Path'." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/run-live-validation.ps1` around lines 292 - 303, Update the three throw
statements in the WSL path conversion block to include stable classification
codes so automation can parse failures: change the "wsl executable not
available" throw (near $wsl.Source check) to include a code like
"ERR-WSL-NOTFOUND:"; change the "Failed to convert ... using wslpath." throw
(after checking $exitCode from Get-LastExitCodeOrZero) to include a code like
"ERR-WSL-WSLPATH:" and include the exitCode variable where available; and change
the "WSL path conversion produced no output" throw (after checking $text) to
include a code like "ERR-WSL-NOOUTPUT:". Ensure each modified throw keeps the
original human-readable message after the code for context.
| $buildOutput = & $buildScript -Mode Windows -Configuration $Config | ||
| $buildExitCode = Get-LastExitCodeOrZero | ||
| if ($buildExitCode -ne 0) { | ||
| throw "ATTACH_NATIVE_HOST_PRECHECK_FAILED: native host build exited with code $buildExitCode." | ||
| } | ||
| foreach ($line in @($buildOutput)) { | ||
| if (-not [string]::IsNullOrWhiteSpace([string]$line)) { | ||
| Write-Information ([string]$line) -InformationAction Continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Emit captured native-build output before failing preflight.
At Line 331 the function throws before replaying $buildOutput, which removes the exact diagnostics needed for failure triage.
🛠️ Proposed ordering fix
$buildOutput = & $buildScript -Mode Windows -Configuration $Config
$buildExitCode = Get-LastExitCodeOrZero
- if ($buildExitCode -ne 0) {
- throw "ATTACH_NATIVE_HOST_PRECHECK_FAILED: native host build exited with code $buildExitCode."
- }
foreach ($line in @($buildOutput)) {
if (-not [string]::IsNullOrWhiteSpace([string]$line)) {
Write-Information ([string]$line) -InformationAction Continue
}
}
+ if ($buildExitCode -ne 0) {
+ throw "ATTACH_NATIVE_HOST_PRECHECK_FAILED: native host build exited with code $buildExitCode."
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $buildOutput = & $buildScript -Mode Windows -Configuration $Config | |
| $buildExitCode = Get-LastExitCodeOrZero | |
| if ($buildExitCode -ne 0) { | |
| throw "ATTACH_NATIVE_HOST_PRECHECK_FAILED: native host build exited with code $buildExitCode." | |
| } | |
| foreach ($line in @($buildOutput)) { | |
| if (-not [string]::IsNullOrWhiteSpace([string]$line)) { | |
| Write-Information ([string]$line) -InformationAction Continue | |
| } | |
| } | |
| $buildOutput = & $buildScript -Mode Windows -Configuration $Config | |
| $buildExitCode = Get-LastExitCodeOrZero | |
| foreach ($line in @($buildOutput)) { | |
| if (-not [string]::IsNullOrWhiteSpace([string]$line)) { | |
| Write-Information ([string]$line) -InformationAction Continue | |
| } | |
| } | |
| if ($buildExitCode -ne 0) { | |
| throw "ATTACH_NATIVE_HOST_PRECHECK_FAILED: native host build exited with code $buildExitCode." | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/run-live-validation.ps1` around lines 328 - 337, The script currently
throws on a non-zero $buildExitCode before replaying $buildOutput, losing
valuable build diagnostics; modify the control flow in the block that runs &
$buildScript (using $buildOutput and Get-LastExitCodeOrZero) so that you always
iterate over $buildOutput and call Write-Information for non-empty lines (the
foreach over @($buildOutput) with the IsNullOrWhiteSpace check) before throwing
the ATTACH_NATIVE_HOST_PRECHECK_FAILED error when $buildExitCode -ne 0; keep the
same diagnostic formatting and only throw after replaying the captured output.
Prekzursil
force-pushed
the
codex/m1-epic7-closure-phase
branch
from
cc5f7e7 to
1620cc4
Compare
Harden native host bootstrap and live runner host-path wiring, align promoted matrix set_unit_cap sequencing with fail-closed restore semantics, add forced-context ProcessLocator unit coverage, and update runbook/TODO evidence references for final live matrix runs. Co-authored-by: Codex <noreply@openai.com>
Prekzursil
force-pushed
the
codex/m1-epic7-closure-phase
branch
from
1620cc4 to
6ed7199
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/run-live-validation.ps1 (1)
85-165: Consider extracting shared helpers to a common module.
Invoke-CapturedCommand,Test-IsWslPythonCommand, andConvert-ToWslPathare duplicated between this file andtools/collect-mod-repro-bundle.ps1. Extracting to a shared module (e.g.,tools/common/wsl-helpers.ps1) would reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/run-live-validation.ps1` around lines 85 - 165, Multiple functions (Invoke-CapturedCommand, Test-IsWslPythonCommand, Convert-ToWslPath) are duplicated; extract them into a shared module (for example tools/common/wsl-helpers.ps1) and update both scripts to load that module. Move the full implementations of Invoke-CapturedCommand, Test-IsWslPythonCommand, and Convert-ToWslPath into the new file, ensure they are declared as functions exported or remain available for dot-sourcing, then replace the duplicated definitions in tools/run-live-validation.ps1 and tools/collect-mod-repro-bundle.ps1 with a single module import (dot-source or Import-Module) and run the scripts to verify behavior and tests still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/run-live-validation.ps1`:
- Around line 85-165: Multiple functions (Invoke-CapturedCommand,
Test-IsWslPythonCommand, Convert-ToWslPath) are duplicated; extract them into a
shared module (for example tools/common/wsl-helpers.ps1) and update both scripts
to load that module. Move the full implementations of Invoke-CapturedCommand,
Test-IsWslPythonCommand, and Convert-ToWslPath into the new file, ensure they
are declared as functions exported or remain available for dot-sourcing, then
replace the duplicated definitions in tools/run-live-validation.ps1 and
tools/collect-mod-repro-bundle.ps1 with a single module import (dot-source or
Import-Module) and run the scripts to verify behavior and tests still pass.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
TODO.mddocs/LIVE_VALIDATION_RUNBOOK.mdtests/SwfocTrainer.Tests/Profiles/LiveHeroHelperWorkflowTests.cstests/SwfocTrainer.Tests/Profiles/LivePromotedActionMatrixTests.cstests/SwfocTrainer.Tests/Runtime/ProcessLocatorForcedContextTests.cstools/collect-mod-repro-bundle.ps1tools/native/build-native.ps1tools/run-live-validation.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/SwfocTrainer.Tests/Runtime/ProcessLocatorForcedContextTests.cs
|
|
Finalization update on commit
Evidence bundle references remain:
|
|
Finalization update on commit :\n\n- Fresh local gate rerun passed (MSBuild version 17.8.3+195e7f5a3 for .NET Build succeeded. Time Elapsed 00:00:03.73 Starting test execution, please wait... Passed! - Failed: 0, Passed: 218, Skipped: 5, Total: 223, Duration: 12 s - SwfocTrainer.Tests.dll (net8.0) |
1 participant
Summary
This PR finalizes the push-and-green closure wave on
codex/m1-epic7-closure-phasewith a single commit and updated evidence references.Key delivered changes:
tools/native/build-native.ps1(absolute path normalization, deterministic re-scan, second-pass target build, explicit diagnostics).SWFOC_EXTENDER_HOST_PATHdeterministically intools/run-live-validation.ps1for each live test invocation.run-live-validation.ps1,collect-mod-repro-bundle.ps1) with captured execution flow.set_unit_capto explicit two-step fail-closed contract (enable=true, thenenable=false).Why
Latest closure effort had non-deterministic host/bootstrap and context/tooling edge behavior during live evidence runs. This PR makes those behaviors deterministic and aligns live matrix expectations with fail-closed runtime safety semantics.
Risk
risk:high(runtime/tooling integration path touching native host and live validation harness).main).Evidence (Deterministic)
dotnet build SwfocTrainer.sln -c Release --no-restore✅dotnet test tests/SwfocTrainer.Tests/SwfocTrainer.Tests.csproj -c Release --no-build --filter "FullyQualifiedName!~SwfocTrainer.Tests.Profiles.Live&FullyQualifiedName!~RuntimeAttachSmokeTests"✅ (Passed: 192, Failed: 0)powershell.exe -NoLogo -NoProfile -ExecutionPolicy Bypass -File ./tools/validate-workshop-topmods.ps1 -Path tools/fixtures/workshop_topmods_sample.json -Strict✅powershell.exe -NoLogo -NoProfile -ExecutionPolicy Bypass -File ./tools/validate-generated-profile-seed.ps1 -Path tools/fixtures/generated_profile_seeds_sample.json -Strict✅Evidence (Live / Bundles)
TestResults/runs/LIVE-EAW-20260228-204540/eaw-process-snapshot.jsonTestResults/runs/LIVE-TACTICAL-20260228-211256/repro-bundle.jsonTestResults/runs/LIVE-AOTR-20260228-211521/repro-bundle.jsonTestResults/runs/LIVE-ROE-20260228-211757/repro-bundle.jsonTestResults/runs/LIVE-MATRIX-20260228-211757.mdBundle strict schema validation commands all passed:
... LIVE-TACTICAL-20260228-211256/repro-bundle.json... LIVE-AOTR-20260228-211521/repro-bundle.json... LIVE-ROE-20260228-211757/repro-bundle.jsonBehavioral Notes
set_unit_cap[1/2]andset_unit_cap[2/2]per profile when context is available.Summary by CodeRabbit
Documentation
Tests
Chores
Reliability Evidence Contract
Repro bundle json: TestResults/runs/LIVE-ROE-20260228-211757/repro-bundle.json
Classification: passed