Resolve one dependency version across the full graph - #134
Conversation
📝 WalkthroughWalkthroughChangesThe pull request adds migrated CLI, HTTPClient, SemVer, TOML, and testing packages. It adds cross-platform TLS support, fixed-point dependency resolution, immutable snapshots, lockfile identity metadata, transactional rollback, recovery handling, and extensive tests. CLI package
HTTPClient package
Pascal libraries
Dependency resolution
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
frostney
added
the
stack:managed
frostney
changed the title
frostney
added
ci:ready
frostney
added
ci:ready
frostney
added
ci:ready
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
lwpt.toml-19-23 (1)
19-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd the workspace include glob.
The root manifest ends with an empty
[workspaces]table. The comments state that one include glob replaces the four local dependencies, but noincludekey is present.lwpt installwill discover no workspace packages, so a clean checkout will not materializehttpclient,cli,semver,toml, ortesting.Proposed manifest fix
[workspaces] +include = ["packages/*"]This follows the workspace contract documented in
docs/architecture.mdLine 72 anddocs/packages.mdLine 11.🤖 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 `@lwpt.toml` around lines 19 - 23, Add the workspace include glob to the root [workspaces] table in lwpt.toml so lwpt install discovers and materializes all local packages, including httpclient, cli, semver, toml, and testing, as described by the surrounding comments..lwpt/modules/cli/source/CLI.Subcommands.pas-192-205 (1)
192-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
WantsHelptreats any argument as a help request.The scan tests every argv token from index 2 to
ParamCount. It matches--help,-h, and the literal wordhelpin any position, including option values and positionals.So
lwpt add somepkg --name helpprints theaddhelp text and exits 0 instead of adding the dependency. The same applies to any positional or option value equal tohelp,-h, or--help.Restrict the scan to the argument range the subcommand actually parses, and stop the scan at the first non-option token or at a
--separator.🐛 Proposed fix
-function WantsHelp: Boolean; +function WantsHelp(AStartArg: Integer): Boolean; var i: Integer; A: string; begin - for i := 2 to ParamCount do + Result := False; + for i := AStartArg + 1 to ParamCount do begin A := ParamStr(i); - if (A = '--help') or (A = '-h') or (LowerCase(A) = 'help') then + if A = '--' then Exit; + if (A = '--help') or (A = '-h') then Exit(True); end; - Result := False; end;Update the call site:
- if WantsHelp then + if WantsHelp(ParseStart) thenKeep the bare
helpword recognized only when it is the first argument after the subcommand name.Also applies to: 259-265
🤖 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 @.lwpt/modules/cli/source/CLI.Subcommands.pas around lines 192 - 205, Update WantsHelp and its call site to inspect only subcommand-level option arguments: recognize --help and -h before the first non-option token, stop at the first positional token or -- separator, and recognize bare help only as the first argument after the subcommand name. Ensure option values and positional arguments such as `help`, `-h`, or `--help` are not treated as help requests..lwpt/modules/cli/source/CLI.Help.pas-27-39 (1)
27-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
FindOrAddGroupwrites past the fixedGroupsarray.
Groupsis a fixed array ofMAX_GROUPS(32) entries.FindOrAddGroupassignsAGroups[Result]and incrementsACountwithout comparingACounttoHigh(AGroups). When the option table contains more than 32 distinct group headers, the function writes past the end of the caller's stack array.
Shared.incdisables range checks in production builds (Lines 3-6), so the write is silent there.Add a bounds check, or replace the fixed array with a dynamic array.
🐛 Proposed fix
function FindOrAddGroup(var AGroups: array of TGroupEntry; var ACount: Integer; const AHeader: string): Integer; var I: Integer; begin for I := 0 to ACount - 1 do if AGroups[I].Header = AHeader then Exit(I); + if ACount > High(AGroups) then + raise Exception.CreateFmt( + 'too many option groups: %d exceeds the maximum of %d', + [ACount + 1, System.Length(AGroups)]); Result := ACount; AGroups[Result].Header := AHeader; AGroups[Result].Lines := TStringList.Create; Inc(ACount); end;Also applies to: 44-48
🤖 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 @.lwpt/modules/cli/source/CLI.Help.pas around lines 27 - 39, Update FindOrAddGroup to check ACount against the available AGroups upper bound before assigning AGroups[Result] or incrementing ACount; when the fixed array is full, return a safe failure result and ensure callers handle it without writing past the array. Preserve existing lookup and group-creation behavior when capacity remains..lwpt/modules/cli/source/StringBuffer.pas-38-64 (1)
38-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle a zero-capacity buffer in
AppendandAppendChar.A
TStringBuffervariable that is not initialized byTStringBuffer.CreatehasFCap = 0and an emptyFData. Records have no implicit constructor, so this state is reachable for any consumer of this public record.In that state
Appendenterswhile NewCap < FLen + SLen do NewCap := NewCap * 2;withNewCap = 0, and the loop never terminates.AppendCharcomputesFCap := 0 * 2, callsSetLength(FData, 0), and then writesFData[1]out of bounds.Clamp the capacity to
DEFAULT_CAPACITYbefore growing.🐛 Proposed fix
procedure TStringBuffer.AppendChar(const C: AnsiChar); begin if FLen + 1 > FCap then begin - FCap := FCap * 2; + if FCap <= 0 then + FCap := DEFAULT_CAPACITY + else + FCap := FCap * 2; SetLength(FData, FCap); end; Inc(FLen); FData[FLen] := C; end; procedure TStringBuffer.Append(const S: AnsiString); var SLen, NewCap: Integer; begin SLen := System.Length(S); if SLen = 0 then Exit; if FLen + SLen > FCap then begin NewCap := FCap; + if NewCap <= 0 then NewCap := DEFAULT_CAPACITY; while NewCap < FLen + SLen do NewCap := NewCap * 2; FCap := NewCap; SetLength(FData, FCap); end; Move(S[1], FData[FLen + 1], SLen); Inc(FLen, SLen); end;🤖 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 @.lwpt/modules/cli/source/StringBuffer.pas around lines 38 - 64, Handle zero-capacity buffers in TStringBuffer.Append and TStringBuffer.AppendChar by initializing FCap to DEFAULT_CAPACITY before any growth calculation when FCap is zero. Ensure Append’s growth loop advances and AppendChar allocates storage before writing, while preserving existing behavior for initialized buffers..lwpt/modules/testing/source/TestingPascalLibrary.pas-300-311 (1)
300-311: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRun
AfterEacheven when the test body raises.
ASuite.AfterEach(Line 308) runs only whenASuite.BeforeEachandATest.Methodboth succeed. If the test raises,AfterEachis skipped, and any state or resource cleanup for that test never happens. This can pollute the state for the next test in the same suite, causing unrelated failures.Wrap the test body in
try...finallysoAfterEachalways runs.🐛 Proposed fix to guarantee `AfterEach` execution
StartTime := Now; try - ASuite.BeforeEach; - ATest.Method; - ASuite.AfterEach; + try + ASuite.BeforeEach; + ATest.Method; + finally + ASuite.AfterEach; + end; if not ASuite.FHasAssertions then raise ETestAssertionError.Create('Test has no assertions');🤖 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 @.lwpt/modules/testing/source/TestingPascalLibrary.pas around lines 300 - 311, Update the test execution flow around ASuite.BeforeEach and ATest.Method so ASuite.AfterEach runs in a finally block whenever setup succeeds, including when the test body raises; preserve the existing assertion validation and exception propagation behavior..lwpt/modules/testing/source/TestingPascalLibrary.pas-333-365 (1)
333-365: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestructure exception handling in
Runto avoid doubleAfterAllcalls and silent loss of results.
Suite.BeforeAll(Line 348) sits outside thetryblock. If it raises, the exception is not caught. The loop stops immediately, the remaining suites inFSuitesnever run, andPrintResults/PrintSummary(Lines 363-364) never execute. All already-collected results are lost from the report.
Suite.AfterAllruns on the success path at Line 356.RunTestalready catches every exception internally, so the only realistic path intoexceptisSuite.AfterAllitself failing. When that happens, the handler callsSuite.AfterAlla second time (Line 358) before re-raising. This runs teardown twice and, if the second call also raises, the new exception still escapesRun, again skipping the remaining suites and the result output.Wrap
BeforeAll/the test loop in atry...finallysoAfterAllruns exactly once, and catch per-suite exceptions so one suite's setup or teardown failure does not silently drop every other suite's results.🐛 Proposed fix for per-suite exception handling
for Suite in FSuites do begin WriteLn(' ', Suite.Name); - Suite.BeforeAll; try - for Test in Suite.Tests do - begin - Suite.FCurrentTestName := Test.Name; - RunTest(Suite, Test); - end; - - Suite.AfterAll; + Suite.BeforeAll; + try + for Test in Suite.Tests do + begin + Suite.FCurrentTestName := Test.Name; + RunTest(Suite, Test); + end; + finally + Suite.AfterAll; + end; except - Suite.AfterAll; - raise; + on E: Exception do + WriteLn(ErrOutput, 'Suite ', Suite.Name, + ' setup/teardown failed: ', E.Message); end; end; PrintResults; PrintSummary;🤖 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 @.lwpt/modules/testing/source/TestingPascalLibrary.pas around lines 333 - 365, Restructure TTestRunner.Run so each suite’s BeforeAll and test loop are enclosed in a per-suite try/finally, with Suite.AfterAll executed exactly once regardless of setup or test-loop failures. Add per-suite exception handling that records or reports the failure and continues to the next suite, ensuring PrintResults and PrintSummary always execute after all suites have been processed..lwpt/modules/testing/source/TestingPascalLibrary.pas-142-169 (1)
142-169: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse type-specific string pointers and read the full enum ordinal.
In FPC 3.2.2,
SysUtils.PStringresolves to anAnsiStringpointer. UsePShortString,PAnsiString,PUnicodeString, andPWideStringfor their respective RTTI kinds.
PByte(P)^truncates enums stored in two or four bytes. Read the ordinal usingGetTypeData(TypeInfo(T))^.OrdType.🤖 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 @.lwpt/modules/testing/source/TestingPascalLibrary.pas around lines 142 - 169, Update TExpect<T>.FormatValue to use the type-specific string pointer for each RTTI kind: PShortString, PAnsiString, PUnicodeString, and PWideString instead of PString. For tkEnumeration, read the ordinal using GetTypeData(TypeInfo(T))^.OrdType so two- and four-byte enum values are not truncated..lwpt/modules/semver/source/Semver.pas-1993-2000 (1)
1993-2000: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SortedVersionsis never sorted, and the dead loop marks where the sort belongs.Lines 1996-2000 form a loop that always runs once and always breaks:
while True do begin Result := ''; Break; end;It has no effect. It sits exactly where node-semver sorts the version list before scanning.
SortedVersionsreceives an unsorted copy ofAVersions. The rest of the function depends on ascending order:
- The contiguous-run scan at Lines 2005-2023 assumes satisfying versions appear consecutively. Unsorted input splits one run into several and emits a wrong
||union.- The
MinValue = SortedVersions[0]tests at Lines 2041 and 2045 decide between*,<=, and a hyphen range. They are only meaningful when element 0 is the lowest version.Sort
SortedVersionsascending withCompareSemverValuesand delete the dead loop.🐛 Proposed fix
SetLength(SortedVersions, Length(AVersions)); for I := Low(AVersions) to High(AVersions) do SortedVersions[I - Low(AVersions)] := AVersions[I]; - while True do - begin - Result := ''; - Break; - end; + SortVersionsAscending(SortedVersions, AOptions);Add an ascending sort helper that compares with
Compare(...)and skips entries that fail to parse.🤖 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 @.lwpt/modules/semver/source/Semver.pas around lines 1993 - 2000, In the version-range construction function, replace the dead while loop following the SortedVersions copy with an ascending sort of SortedVersions using CompareSemverValues. Ensure the comparator handles unparseable entries according to the existing semver comparison behavior, then leave the contiguous-run scan and minimum-value logic operating on the sorted array..lwpt/modules/httpclient/source/TransportSecurity.Test.pas-416-427 (1)
416-427: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnsafe
Copy(PAnsiChar(...))conversion of a non-terminated byte buffer at three sites. Each site converts@Buffer[0]toPAnsiCharand then copies. That conversion scans for a terminating#0byte. The buffers are plainarray of Byteand are never zero-initialized, so a full-length read has no terminator and the scan runs past the end of the stack buffer. Any embedded#0in the plaintext also truncates the result.SetString(Dest, PAnsiChar(@buffer[0]), Count)copies an exact byte count with no scan, and.lwpt/modules/httpclient/tests/e2e/TransportSecuritySocket.E2E.Test.pasalready uses that form at lines 265 and 431.
.lwpt/modules/httpclient/source/TransportSecurity.Test.pas#L416-L427: inReadRawClientPlaintext, replace line 426 withSetString(Result, PAnsiChar(@buffer[0]), ReadCount);..lwpt/modules/httpclient/source/TransportSecurity.Test.pas#L751-L755: add a localDecoded: string;, thenSetString(Decoded, PAnsiChar(@buffer[0]), ReadResult.BytesProcessed);and compareDecodedagainstCLIENT_REQUEST..lwpt/modules/httpclient/source/TransportSecurity.Test.pas#L892-L896: apply the sameSetStringsubstitution here and at line 917, whereClientReadis the byte count.🤖 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 @.lwpt/modules/httpclient/source/TransportSecurity.Test.pas around lines 416 - 427, Replace unsafe null-terminated conversions with exact-length SetString copies at ReadRawClientPlaintext (lines 416-427), the ReadResult.BytesProcessed site (lines 751-755), and both ClientRead sites (lines 892-896 and 917) in .lwpt/modules/httpclient/source/TransportSecurity.Test.pas. Add the Decoded local where required and compare it with CLIENT_REQUEST, preserving embedded null bytes and full-length reads..lwpt/modules/httpclient/tests/e2e/TransportSecuritySocket.E2E.Test.pas-144-155 (1)
144-155: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
WaitForso a failed construction does not hang the destructor.When a constructor raises, FreePascal calls the destructor on the partially built instance.
TLoopbackTLSServer.Createcan raise at line 117 (missing or unreadable PKCS#12) or at lines 122-133 (socket setup).Destroythen reachesWaitForat line 152 for a thread that was created suspended and never started.WaitForwaits on a handle that is never signaled, so the test process hangs instead of reporting the original error.Track whether
Executewas started and callWaitForonly in that case.🔒️ Proposed fix
Add a field and set it in the test:
private FContext: TTransportSecurityServerContext; FErrorMessage: string; FListenSocket: TSocket; FPeerGone: Boolean; FPort: Word; FRequest: string; FSawFragmentedInput: Boolean; FSawShortWrite: Boolean; + FStarted: Boolean;destructor TLoopbackTLSServer.Destroy; begin if FListenSocket >= 0 then begin FpShutdown(FListenSocket, 2); CloseSocket(FListenSocket); FListenSocket := -1; end; - WaitFor; + if FStarted then + WaitFor; CloseTransportSecurityServerContext(FContext); inherited Destroy; end;Set
FStarted := Trueat the top ofExecute, or expose aStartServermethod that sets the flag and callsStart.🤖 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 @.lwpt/modules/httpclient/tests/e2e/TransportSecuritySocket.E2E.Test.pas around lines 144 - 155, Update TLoopbackTLSServer to track whether its thread has started: add an FStarted state field, set it at the beginning of Execute (or in a start wrapper immediately before starting the thread), and guard the WaitFor call in Destroy with that state. Preserve socket and context cleanup for partially constructed instances so constructor errors propagate without hanging..lwpt/modules/httpclient/source/Tests.HTTPMockServer.pas-345-351 (1)
345-351: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDestroy the critical section after
inherited Destroy.
TThread.DestroycallsTerminateandWaitForwhen the thread still runs. Line 348 runsDoneCriticalSection(FCriticalSection)before that wait. IfExecutehas not returned yet, itsfinallyblock callsCloseOwnedSockets, which enters an already-destroyed critical section.
TMockHTTPServer.Destroycurrently callsStopandWaitForfirst, so the window is closed there. The thread class itself is still unsafe for any other caller.🔒️ Proposed fix
destructor TMockServerThread.Destroy; begin - CloseOwnedSockets; - DoneCriticalSection(FCriticalSection); - InterlockedDecrement(GMockLiveThreads); - inherited Destroy; + Stop; + inherited Destroy; { terminates and waits for Execute } + CloseOwnedSockets; + DoneCriticalSection(FCriticalSection); + InterlockedDecrement(GMockLiveThreads); end;🤖 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 @.lwpt/modules/httpclient/source/Tests.HTTPMockServer.pas around lines 345 - 351, Update TMockServerThread.Destroy to call inherited Destroy before DoneCriticalSection(FCriticalSection), ensuring TThread terminates and waits for Execute to finish before the critical section is destroyed; keep CloseOwnedSockets and the live-thread decrement in the destructor’s existing cleanup flow..lwpt/modules/httpclient/source/FileUtils.pas-79-99 (1)
79-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCall
FindCloseonly after a successfulFindFirst, and skip directory entries in the extension match.Two problems exist in this block:
- Line 99 calls
FindClose(SearchRec)even whenFindFirstat line 79 returned a non-zero result.SearchRecis a local record and is not initialized in that path, soFindClosereads an indeterminate handle value.- Line 95 evaluates
MatchesExtensionfor every entry, including directories. A directory namedsomething.pasis added to the result list as if it were a file.🐛 Proposed fix
if FindFirst(Dir + PathDelim + '*', faAnyFile, SearchRec) = 0 then begin - repeat - if (SearchRec.Attr and faDirectory) = faDirectory then - begin - if (SearchRec.Name <> '.') and (SearchRec.Name <> '..') then - begin - SubdirFiles := FindAllFiles(Dir + PathDelim + SearchRec.Name, AFileExtensions); - try - Files.AddStrings(SubdirFiles); - finally - SubdirFiles.Free; - end; - end; - end; - - if MatchesExtension(SearchRec.Name, AFileExtensions) then - Files.Add(Dir + PathDelim + SearchRec.Name); - until FindNext(SearchRec) <> 0; + try + repeat + if (SearchRec.Attr and faDirectory) = faDirectory then + begin + if (SearchRec.Name <> '.') and (SearchRec.Name <> '..') then + begin + SubdirFiles := FindAllFiles(Dir + PathDelim + SearchRec.Name, AFileExtensions); + try + Files.AddStrings(SubdirFiles); + finally + SubdirFiles.Free; + end; + end; + end + else if MatchesExtension(SearchRec.Name, AFileExtensions) then + Files.Add(Dir + PathDelim + SearchRec.Name); + until FindNext(SearchRec) <> 0; + finally + FindClose(SearchRec); + end; end; - FindClose(SearchRec); Files.Sort;🤖 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 @.lwpt/modules/httpclient/source/FileUtils.pas around lines 79 - 99, Update the FindAllFiles search block so FindClose(SearchRec) executes only when FindFirst succeeds, and evaluate MatchesExtension only for non-directory entries. Preserve recursive processing of valid subdirectories and add only matching files to Files.source/LWPT.Install.pas-693-719 (1)
693-719: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTest-only control flow is compiled unconditionally into the release binary. Every site below reads an
LWPT_TEST_*environment variable and then changes install behavior: it substitutes fetched data, forces a rollback failure, raises an injected error, or callsHalt. The shared root cause is the absence of a compile-time guard. Add one define (for exampleLWPT_TEST_HOOKS), enable it only for the test build entry points, and wrap every hook in{$IFDEF LWPT_TEST_HOOKS} ... {$ENDIF}. TheHaltsites are the most severe:Haltskips thefinallyblock that freesTInstallLock, so.lwpt/install.locksurvives and every later install fails withEConcurrencyErroruntil the user runslwpt repair.
source/LWPT.Install.pas#L693-L719: guard theLWPT_TEST_GIT_FIXTURE_DIRarchive branch andLoadTestFixtureArchiveso the release binary always performs the bounded HTTP fetch.source/LWPT.GitProtocol.pas#L309-L313: guard theLWPT_TEST_GIT_FIXTURE_DIRbranch inListRemoteRefsso the release binary always readsinfo/refsover HTTPS.source/LWPT.Core.pas#L1112-L1119: guard theLWPT_TEST_THROW_RESTORE_FORraise inAtomicRestorePathso rollback cannot be disabled at runtime.source/LWPT.Install.pas#L2221-L2224: guard theLWPT_TEST_HALT_AFTER_MODULE_RETAINHalt(87)call.source/LWPT.Install.pas#L2254-L2264: guard theLWPT_TEST_FAIL_PUBLISH_AFTERraise, theLWPT_TEST_HALT_PUBLISH_AFTERHalt(86)call, and theLWPT_TEST_STALE_LOCAL_SNAPSHOThash override at Line 2197.source/LWPT.Install.pas#L3022-L3045: guard theLWPT_TEST_FAIL_AFTER_LOCK_WRITEraise, theLWPT_TEST_CORRUPT_ROLLBACK_FORbackup corruption, and theLWPT_TEST_FAIL_AFTER_ORPHAN_RETAINraise at Line 3057.🤖 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 `@source/LWPT.Install.pas` around lines 693 - 719, Test-only environment-variable hooks are available in release builds and can alter installation behavior. Define a compile-time LWPT_TEST_HOOKS symbol enabled only by test build entry points, then wrap every listed hook with {$IFDEF LWPT_TEST_HOOKS} guards: the fixture archive branch in source/LWPT.Install.pas:693-719, the ListRemoteRefs fixture branch in source/LWPT.GitProtocol.pas:309-313, the AtomicRestorePath injected raise in source/LWPT.Core.pas:1112-1119, the retain-module Halt in source/LWPT.Install.pas:2221-2224, the publish failure/Halt and stale snapshot override in source/LWPT.Install.pas:2254-2264 (including the override at line 2197), and the lock-write failure, rollback corruption, and orphan-retain failure hooks in source/LWPT.Install.pas:3022-3045 (including line 3057). Ensure release builds always use normal HTTPS fetching and cannot execute injected failures or Halt calls.source/LWPT.Resolver.pas-13-31 (1)
13-31: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDerive
EResolverConflictfromELWPTErrorand use the project type prefixes.
EResolverConflictdescends fromException. Every other LWPT error type descends fromELWPTErrorinLWPT.Core, which carries theOperationandRecoveryfields the CLI error reporting uses. A caller that handlesELWPTErrorwill not catch this exception, so a resolver conflict escapes as an unclassified exception.LWPT.Install.SelectNoderewraps it intoEManifestError, butSelectHighestRefis a public function and other callers get no such treatment.The coding guidelines also require the
TLWPT...andELWPT...prefixes for project-owned types.♻️ Proposed change
uses SysUtils, + LWPT.Core, LWPT.GitProtocol, LWPT.Manifest; type - TResolverRequirement = record + TLWPTResolverRequirement = record Spec: string; Kind: TVersionKind; Requirer: string; end; - TResolverRequirementArray = array of TResolverRequirement; + TLWPTResolverRequirementArray = array of TLWPTResolverRequirement; - TResolverSelection = record + TLWPTResolverSelection = record RefName: string; CommitSHA: string; end; - EResolverConflict = class(Exception); + ELWPTResolverConflict = class(ELWPTError);Rename the references in
source/LWPT.Install.pasandsource/LWPT.Resolver.Test.pasaccordingly.
As per coding guidelines: "UseTLWPT...for types andELWPT...for exceptions".🤖 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 `@source/LWPT.Resolver.pas` around lines 13 - 31, Update the resolver declarations by renaming TResolverRequirement, TResolverRequirementArray, and TResolverSelection to TLWPT-prefixed names, and rename EResolverConflict to ELWPTResolverConflict while deriving it from ELWPTError. Adjust all references in SelectHighestRef and the resolver consumers in LWPT.Install.SelectNode and LWPT.Resolver.Test to use the new names.Source: Coding guidelines
.lwpt/modules/httpclient/source/TransportSecurity.pas-1016-1026 (1)
1016-1026: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBusy loop on every TLS want-state when
Deadline = 0. Each site guards the readiness wait withAConnection.Deadline <> 0. The no-deadline overloadStartTransportSecurity(AConnection, ASocket, AHost)at Line 67 setsDeadlineto0, and the socket is nonblocking, so the retry loop repeats with no wait and consumes a full CPU core. The shared root cause is that a zero deadline disables the wait instead of selecting an untimed wait. Add an untimed wait path inWaitForTransportSocketandRemainingTransportMilliseconds, then remove theDeadline <> 0conditions.
.lwpt/modules/httpclient/source/TransportSecurity.pas#L1016-L1026: callWaitForTransportSocketforSSL_ERROR_WANT_READandSSL_ERROR_WANT_WRITEwithout theDeadline <> 0condition..lwpt/modules/httpclient/source/TransportSecurity.pas#L479-L487: callWaitForTransportSocketonERR_SSL_WOULD_BLOCKwithout theDeadline <> 0condition..lwpt/modules/httpclient/source/TransportSecurity.pas#L1581-L1589: callWaitForTransportSocketinReadOpenSSLwithout theDeadline <> 0condition, and apply the same change toWriteOpenSSLat Line 1620.🤖 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 @.lwpt/modules/httpclient/source/TransportSecurity.pas around lines 1016 - 1026, The TLS retry paths busy-loop when Deadline is zero because they skip socket readiness waits. Update WaitForTransportSocket and RemainingTransportMilliseconds to support untimed waits, then remove Deadline <> 0 guards at .lwpt/modules/httpclient/source/TransportSecurity.pas:1016-1026, 479-487, and 1581-1589; apply the same unconditional wait change to WriteOpenSSL at 1620. Preserve timed waits when a deadline exists and use untimed waiting otherwise..lwpt/modules/httpclient/source/TransportSecurity.pas-769-794 (1)
769-794: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftThe restricted
LoadLibraryExWhandles do not constrain the library that gets bound.This code calls
LoadLibraryExWwithLOAD_LIBRARY_SEARCH_DEFAULT_DIRSandLOAD_LIBRARY_SEARCH_SYSTEM32, then callsInitSSLInterface, then releases both handles in thefinallyblock.
InitSSLInterfacein the FPCOpenSSLunit performs its own module load. That load does not inherit the restricted search flags from the earlierLoadLibraryExWcalls. TheLoadLibraryExWcalls therefore only prove that the DLLs are reachable through the restricted search path. They do not preventInitSSLInterfacefrom binding a DLL found through the ordinary search order, which includes the application directory andPATH.The
FreeLibrarycalls at Lines 792-793 then release the restricted references, so no restricted handle remains.Keep the restricted handles for the process lifetime, and confirm that the module bound by
InitSSLInterfaceis the module loaded under the restricted flags. Compare the module handle fromGetModuleHandleWagainst the handles obtained here, or resolve the required entry points directly from the restricted handles instead of relying onInitSSLInterface.Run the following script to inspect how the codebase depends on
InitSSLInterfaceand whether any Windows import check already covers this:#!/bin/bash # Description: Trace OpenSSL loading usage and the Windows TLS import checker. set -euo pipefail rg -n -C 5 'InitSSLInterface|LoadLibraryEx|SSLLibHandle|SSLUtilHandle|DLLSSLName|DLLUtilName' --iglob '*.pas' --iglob '*.inc' fd -i 'check-windows-tls-imports' --exec cat -n {}Based on the coding guideline "Load Windows server DLLs through restricted
LoadLibraryExsearch settings that exclude the current directory and ordinaryPATH."🤖 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 @.lwpt/modules/httpclient/source/TransportSecurity.pas around lines 769 - 794, The Windows OpenSSL loading path in the MSWINDOWS branch does not ensure that InitSSLInterface binds the restricted handles. Update the flow around ConfigureOpenSSLLoading and InitSSLInterface to retain the LoadLibraryExW handles for the process lifetime and verify the bound module handles with GetModuleHandleW, or initialize required entry points directly from those handles; remove the current finally-block FreeLibrary calls while preserving failure cleanup and secure-result reporting..lwpt/modules/httpclient/source/HTTPClient.pas-1075-1101 (1)
1075-1101: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRestrict redirects that downgrade the scheme or change the origin.
Two gaps exist in this redirect loop:
- A response from an
httpsURL can sendLocation: http://.... The loop follows it and the next request runs in cleartext. No check compares the new scheme with the previous scheme.- All caller-supplied headers in
AHeadersare re-sent on every redirect hop, including credentials such asAuthorization. A redirect to a different host therefore forwards those headers to that host.This client fetches package archives, so both gaps affect the integrity of downloaded content. Reject an
httpstohttptransition. Drop credential headers when the target host changes.🤖 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 @.lwpt/modules/httpclient/source/HTTPClient.pas around lines 1075 - 1101, Update the redirect handling around CurrentURL in the HTTP request loop to reject any redirect that downgrades from https to http. When the redirect target host differs from the previous host, remove caller-supplied credential headers, including Authorization, before continuing; preserve non-credential headers and same-host redirects..lwpt/modules/httpclient/source/HTTPClient.pas-403-415 (1)
403-415: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the socket if
WaitForSocketraises during connect.
WaitForSocketraisesEHTTPErrorwhen the request deadline expires. On this Unix path the exception escapesConnectSocketwhile the socket inResultis still open, so the descriptor leaks. The Windows implementation already wraps the same call intry..exceptand closes the socket at Line 485. Apply the same handling here.🔧 Proposed fix
if ConnectResult <> 0 then begin - WaitForSocket(Result, False, True, ADeadline, ATimeoutMilliseconds); - SocketError := 0; - SocketErrorLength := SizeOf(SocketError); - if (fpGetSockOpt(Result, SOL_SOCKET, SO_ERROR, `@SocketError`, - `@SocketErrorLength`) <> 0) or (SocketError <> 0) then - begin - CloseSocket(Result); - raise EHTTPError.CreateFmt('Failed to connect to %s:%d', - [AHost, APort]); - end; + try + WaitForSocket(Result, False, True, ADeadline, ATimeoutMilliseconds); + except + CloseSocket(Result); + raise; + end; + SocketError := 0; + SocketErrorLength := SizeOf(SocketError); + if (fpGetSockOpt(Result, SOL_SOCKET, SO_ERROR, `@SocketError`, + `@SocketErrorLength`) <> 0) or (SocketError <> 0) then + begin + CloseSocket(Result); + raise EHTTPError.CreateFmt('Failed to connect to %s:%d', + [AHost, APort]); + end; end;🤖 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 @.lwpt/modules/httpclient/source/HTTPClient.pas around lines 403 - 415, Update the Unix connect path in ConnectSocket so WaitForSocket is wrapped with exception handling that closes the socket in Result before re-raising EHTTPError. Preserve the existing socket-error validation and normal cleanup behavior after a successful wait, matching the Windows implementation’s handling..lwpt/modules/httpclient/source/StringBuffer.pas-38-64 (1)
38-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the growth logic against
FCap = 0.
TStringBufferis a record, so the compiler does not callCreate. A declared but not initializedTStringBuffervariable, or aTStringBufferfield inside a zero-filled class, hasFCap = 0. Two failures follow:
Append:NewCapstarts at0andNewCap * 2stays0, so thewhile NewCap < FLen + SLenloop on Line 58 never terminates. The process hangs.AppendChar:FCap * 2stays0,SetLength(FData, 0)runs, and Line 46 then writes toFData[1]outside the allocated string.Clamp the new capacity to a minimum before doubling.
🛡️ Proposed fix
procedure TStringBuffer.AppendChar(const C: AnsiChar); begin if FLen + 1 > FCap then begin - FCap := FCap * 2; + if FCap < DEFAULT_CAPACITY then + FCap := DEFAULT_CAPACITY + else + FCap := FCap * 2; SetLength(FData, FCap); end; Inc(FLen); FData[FLen] := C; end; procedure TStringBuffer.Append(const S: AnsiString); var SLen, NewCap: Integer; begin SLen := System.Length(S); if SLen = 0 then Exit; if FLen + SLen > FCap then begin - NewCap := FCap; + NewCap := FCap; + if NewCap < DEFAULT_CAPACITY then + NewCap := DEFAULT_CAPACITY; while NewCap < FLen + SLen do NewCap := NewCap * 2; FCap := NewCap; SetLength(FData, FCap); end;🤖 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 @.lwpt/modules/httpclient/source/StringBuffer.pas around lines 38 - 64, Update the growth logic in TStringBuffer.AppendChar and TStringBuffer.Append to ensure the capacity is initialized to a positive minimum before any doubling. Preserve the existing doubling behavior once capacity is nonzero, and ensure both methods allocate enough space for the requested character or string without looping indefinitely or writing beyond FData..lwpt/modules/httpclient/source/TransportSecurity.pas-2019-2021 (1)
2019-2021: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce a TLS 1.2 minimum in the SChannel backend.
grbitEnabledProtocols := 0delegates protocol selection to system policy, andSCH_USE_STRONG_CRYPTOdoes not itself disable TLS 1.0 or TLS 1.1. SetSP_PROT_TLS1_2_CLIENTforSCHANNEL_CRED, or migrate toSCH_CREDENTIALSwithTLS_PARAMETERSwhen TLS 1.3 support is required. This keeps the minimum consistent across backends.🤖 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 @.lwpt/modules/httpclient/source/TransportSecurity.pas around lines 2019 - 2021, Update the SChannel credential initialization around Credential and SCH_USE_STRONG_CRYPTO to explicitly set the enabled client protocol to SP_PROT_TLS1_2_CLIENT, ensuring TLS 1.0 and TLS 1.1 are disabled while preserving the existing strong-crypto flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a87648bb-4cde-443f-857b-9517d20751c7
⛔ Files ignored due to path filters (7)
.lwpt/modules/httpclient/source/fixtures/localhost-test-intermediate-cert.pemis excluded by!**/*.pem.lwpt/modules/httpclient/source/fixtures/localhost-test-intermediate-key.pemis excluded by!**/*.pem.lwpt/modules/httpclient/source/fixtures/localhost-test-leaf-cert.pemis excluded by!**/*.pem.lwpt/modules/httpclient/source/fixtures/localhost-test-leaf-key.pemis excluded by!**/*.pem.lwpt/modules/httpclient/source/fixtures/test-root-cert.pemis excluded by!**/*.pem.lwpt/modules/httpclient/source/fixtures/test-root-key.pemis excluded by!**/*.pemlwpt.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
.lwpt/modules/cli.lwpt/modules/cli/lwpt.toml.lwpt/modules/cli/source/CLI.Help.pas.lwpt/modules/cli/source/CLI.Options.pas.lwpt/modules/cli/source/CLI.Parser.Test.pas.lwpt/modules/cli/source/CLI.Parser.pas.lwpt/modules/cli/source/CLI.Prompts.pas.lwpt/modules/cli/source/CLI.Subcommands.Test.pas.lwpt/modules/cli/source/CLI.Subcommands.pas.lwpt/modules/cli/source/Shared.inc.lwpt/modules/cli/source/StringBuffer.pas.lwpt/modules/httpclient.lwpt/modules/httpclient/lwpt.toml.lwpt/modules/httpclient/scripts/check-windows-tls-imports.pas.lwpt/modules/httpclient/scripts/regenerate-utf8-pkcs12.pas.lwpt/modules/httpclient/source/FileUtils.pas.lwpt/modules/httpclient/source/HTTPClient.Test.pas.lwpt/modules/httpclient/source/HTTPClient.pas.lwpt/modules/httpclient/source/Shared.inc.lwpt/modules/httpclient/source/StringBuffer.pas.lwpt/modules/httpclient/source/Tests.HTTPMockServer.pas.lwpt/modules/httpclient/source/TransportSecurity.Test.pas.lwpt/modules/httpclient/source/TransportSecurity.pas.lwpt/modules/httpclient/source/fixtures/README.md.lwpt/modules/httpclient/source/fixtures/intermediate.cnf.lwpt/modules/httpclient/source/fixtures/leaf.cnf.lwpt/modules/httpclient/source/fixtures/localhost-empty-passphrase.p12.lwpt/modules/httpclient/source/fixtures/localhost-test-identity.p12.lwpt/modules/httpclient/source/fixtures/localhost-utf8-passphrase.p12.lwpt/modules/httpclient/source/fixtures/root.cnf.lwpt/modules/httpclient/tests/e2e/TransportSecuritySocket.E2E.Test.pas.lwpt/modules/semver.lwpt/modules/semver/lwpt.toml.lwpt/modules/semver/source/Semver.Test.pas.lwpt/modules/semver/source/Semver.pas.lwpt/modules/semver/source/Shared.inc.lwpt/modules/testing.lwpt/modules/testing/lwpt.toml.lwpt/modules/testing/source/Shared.inc.lwpt/modules/testing/source/TestingPascalLibrary.Test.pas.lwpt/modules/testing/source/TestingPascalLibrary.pas.lwpt/modules/toml.lwpt/modules/toml/lwpt.toml.lwpt/modules/toml/source/BaseMap.pas.lwpt/modules/toml/source/OrderedStringMap.pas.lwpt/modules/toml/source/Shared.inc.lwpt/modules/toml/source/TOML.pasdocs/README.mddocs/adr/0014-packages-extraction.mddocs/adr/0031-fixed-point-single-version-resolution.mddocs/architecture.mddocs/packages.mddocs/testing.mddocs/tooling.mdlwpt.tomlpackages/testing/lwpt.tomlsource/LWPT.Command.Repair.passource/LWPT.Core.Test.passource/LWPT.Core.passource/LWPT.GitProtocol.passource/LWPT.Install.passource/LWPT.Manifest.passource/LWPT.Resolver.Test.passource/LWPT.Resolver.pastests/integration/AddRemove.Test.pastests/integration/InstallGitGraph.Test.pastests/integration/InstallGraphConflict.Test.pastests/integration/InstallLocalDiamond.Test.pas
💤 Files with no reviewable changes (5)
- .lwpt/modules/cli
- .lwpt/modules/testing
- .lwpt/modules/semver
- .lwpt/modules/httpclient
- .lwpt/modules/toml
frostney
removed
ci:ready
frostney
added
ci:ready
|
@coderabbitai review |
✅ Action performedReview finished.
|
frostney
added
the
merge:ready
frostney
removed
ci:ready
frostney
removed
the
merge:ready
# Conflicts: # docs/testing.md
frostney
added
ci:ready
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
.lwpt/modules/toml/source/TOML.pas (7)
38-58: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRename the public Pascal types to the required LWPT prefixes.
This unit declares
TTOML...,TTemporal..., andETOMLParseError. The required prefixes areTLWPT...for types andELWPT...for exceptions. Rename the declarations and every reference before publishing this API. Apply the same rename topackages/toml/source/TOML.passo both parser copies expose the same interface.As per coding guidelines, compiled Pascal code must use
TLWPT...for types andELWPT...for exceptions.Also applies to: 63-97, 99-182
🤖 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 @.lwpt/modules/toml/source/TOML.pas around lines 38 - 58, Rename all public TOML types and exceptions in the parser, including ETOMLParseError, TTOMLNode and related TTOML/TTemporal declarations, to the required ELWPT and TLWPT prefixes, and update every reference throughout the unit. Apply the identical renaming to the corresponding TOML.pas copy under packages/toml so both parser interfaces match.Source: Coding guidelines
1758-1787: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject lowercase
zin offsets.
TryParseOffsetaccepts lowercasez, andTryParseDateTimeconverts it toZ. TOML 1.1 permits only uppercaseZor a numeric offset. Remove the lowercase check.🤖 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 @.lwpt/modules/toml/source/TOML.pas around lines 1758 - 1787, Update TTOMLParser.TryParseOffset to accept only uppercase 'Z' for the UTC designator, removing the lowercase 'z' condition while preserving numeric offset parsing.
491-506: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed UTF-8 before tokenization.
PrepareInputchecks control bytes but not UTF-8 validity. Invalid sequences such asFForC0 AFinside a quoted string pass throughParseBasicStringand enter the AST. Reject invalid UTF-8 input and add byte-level regression tests.🤖 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 @.lwpt/modules/toml/source/TOML.pas around lines 491 - 506, Update TTOMLParser.PrepareInput to validate the entire input as well-formed UTF-8, including bytes inside quoted strings, before tokenization proceeds; reject malformed sequences such as invalid leading bytes, truncated sequences, bad continuation bytes, and overlong encodings through the existing parse-error path. Add byte-level regression tests covering representative invalid UTF-8 inputs and confirm valid UTF-8 remains accepted.
366-395: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck integer bounds before
QWordmultiplication.The binary and octal parsers can wrap values such as
2^64to zero before the range check. Overflow checking can instead raise runtime error 215 instead ofETOMLParseError. Apply checked preconditions in both parsers andCanonicalizeIntegerToken. Add tests for2^63-1,2^63, and2^64in binary and octal forms.🤖 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 @.lwpt/modules/toml/source/TOML.pas around lines 366 - 395, Update CanonicalizeIntegerToken and the binary/octal integer parsing paths to validate each QWord multiplication and addition before applying it, preventing 2^64 and larger values from wrapping; convert any overflow into ETOMLParseError rather than runtime error 215. Preserve valid handling for 2^63-1 and 2^63, and add coverage for 2^63-1, 2^63, and 2^64 in both binary and octal forms.
1210-1223: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd exception-safe cleanup for temporary
TTOMLNodevalues.
ParseArray,ParseInlineTable, andParseKeyValuePaircan leave nodes unowned when parsing fails orAssignValuerejects a duplicate key.ParseDocumentthen frees onlyFRoot; inputs such asa = [1,and duplicateaassignments leak nodes. Free unowned nodes on failure and setValueNodetonilafter ownership transfers successfully.🤖 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 @.lwpt/modules/toml/source/TOML.pas around lines 1210 - 1223, Add exception-safe ownership cleanup to ParseArray, ParseInlineTable, and ParseKeyValuePair: initialize temporary TTOMLNode variables to nil, free them in exception paths when ownership has not transferred, and clear ValueNode after successful AssignValue ownership transfer. Ensure parse failures and duplicate-key rejection do not leak unowned nodes while preserving normal ownership behavior.
1289-1295: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict whitespace around inline-table
=separators.
SkipWhitespace(True)accepts newlines and comments before and after=, which violateskeyval-sep. UseSkipWhitespace(False)at both calls. KeepSkipWhitespace(True)between entries.🤖 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 @.lwpt/modules/toml/source/TOML.pas around lines 1289 - 1295, In the inline-table key/value parsing flow around ParseKeyPath and ParseValue, replace both SkipWhitespace(True) calls surrounding the '=' separator with SkipWhitespace(False) so newlines and comments are rejected there. Preserve SkipWhitespace(True) for whitespace between inline-table entries.
209-217: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftApply leap-second rules to
ASecond = 60.
IsValidTimeaccepts60for every hour and minute.TryParseDateTimedoes not use the parsed date or offset to validate it, so it accepts2024-01-01T00:00:60Z. Enforce the TOML rule thattime-second = 60is valid only for an actual leap second. Define the policy for local times, which have no date or offset.🤖 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 @.lwpt/modules/toml/source/TOML.pas around lines 209 - 217, Update IsValidTime and the TryParseDateTime validation flow so ASecond = 60 is accepted only when the parsed date and offset identify an actual leap second; reject it for ordinary timestamps. For local times without a date or offset, define the policy as rejecting 60 because leap-second validity cannot be verified, while preserving existing validation for seconds 0–59 and fractional components.source/LWPT.Core.Test.pas (1)
170-170: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale peel-suffix description.
The suite comment at Lines 159-162 still says the peel-suffix record is discarded. The renamed test now requires the record to populate
PeeledSHA. Update the comment to match the current contract.🤖 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 `@source/LWPT.Core.Test.pas` at line 170, Update the suite comment associated with TestPeelSuffixRecordsCommitIdentity to state that the peel-suffix record populates PeeledSHA rather than being discarded, matching the renamed test’s current contract.
🧹 Nitpick comments (3)
.lwpt/modules/httpclient/source/TransportSecurity.Test.pas (1)
851-863: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider feeding the remainder in a loop instead of asserting a single full accept.
Line 852 asserts that the whole remainder is accepted in one call. That holds only while the free space after the first read (
InputHighWatermark - InputBuffered) stays above the remaining ciphertext length. The margin depends on the negotiated cipher's per-record overhead and on the record-splitting behaviour of the OpenSSL runtime, so a different host runtime can shrink it. A feed loop that accumulates the accepted count keeps the counter assertions at lines 858-863 intact and removes that dependency.🤖 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 @.lwpt/modules/httpclient/source/TransportSecurity.Test.pas around lines 851 - 863, Update the ciphertext feeding logic before TransportSecurityServerRead to repeatedly call TransportSecurityFeedCiphertext until the entire remainder is accepted, advancing the buffer offset and remaining length by each call’s accepted count and accumulating the total. Keep the existing final flow counter and backpressure assertions unchanged, while avoiding any assumption that one feed accepts all remaining ciphertext.source/LWPT.Core.Test.pas (2)
28-29: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest the manifest boundary or move this parser test.
TestArrayCannotBecomeTablePathis registered underTLoadManifestValidation, but it callsTTOMLParser.ParseDocumentdirectly. It does not verify thatLoadManifestrejects the invalid path or propagatesEManifestError.Either rewrite both cases to use
LoadManifestandExpectManifestLoadError, or move the method, declaration, and registration to a co-locatedTOML.Test.pasunit.As per coding guidelines,
**/*.Test.pastests must be co-located with source files.Also applies to: 75-75, 865-913, 929-930
🤖 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 `@source/LWPT.Core.Test.pas` around lines 28 - 29, Move TestArrayCannotBecomeTablePath, its class declaration, and its registration from TLoadManifestValidation into the co-located TOML.Test.pas unit, keeping it as a direct TTOMLParser.ParseDocument test; update the relevant test unit references so the test remains registered and follows the co-location convention.Source: Coding guidelines
1665-1669: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify peeled-commit association with multiple tags.
The fixture contains one tag and checks only
PeeledSHA. A parser that attaches every^{}record to the last tag would pass this test.Add a second tag. Assert that only
v1.0.0receives the peeled SHA. Also assert that its original tag-object SHA remains thebbbb...value.Also applies to: 1681-1682
🤖 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 `@source/LWPT.Core.Test.pas` around lines 1665 - 1669, Update TGitProtocolParsing.TestPeelSuffixRecordsCommitIdentity to include a second tag in the fixture, then assert that only v1.0.0 receives the peeled commit SHA. Also verify v1.0.0 retains its original bbbb... tag-object SHA, ensuring peeled records are associated with the correct tag rather than the most recently parsed tag.
🤖 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.
Inline comments:
In @.lwpt/modules/httpclient/tests/e2e/TransportSecuritySocket.E2E.Test.pas:
- Around line 234-240: Update TLoopbackTLSServer.Stop to atomically exchange
FClientSocket with its invalid sentinel before calling FpShutdown, matching the
existing ownership pattern elsewhere in the class. Only invoke FpShutdown on the
exchanged descriptor when it was valid, preventing concurrent Execute cleanup
from reusing or closing the same descriptor.
In @.lwpt/modules/semver/source/Semver.pas:
- Around line 1556-1590: Update ExpandHyphenLowerBound to append a “-0”
prerelease to the generated lower-bound comparator when IncludePrerelease is
enabled and Partial.PrereleaseText is empty; preserve explicit prerelease and
build suffix handling. Add regression coverage for complete and partial hyphen
ranges, including acceptance of prerelease versions at the lower bound.
In `@packages/semver/source/Semver.pas`:
- Around line 1556-1591: Update ExpandHyphenLowerBound to append “-0” to the
generated lower-bound comparator when IncludePrerelease is enabled and the
parsed left endpoint has no prerelease tag. Update ExpandHyphenUpperBound so a
complete right endpoint without a prerelease emits an exclusive next-patch “-0”
bound under the same option, while preserving current behavior otherwise. Add
coverage for partial and complete hyphen ranges with IncludePrerelease set to
true.
---
Outside diff comments:
In @.lwpt/modules/toml/source/TOML.pas:
- Around line 38-58: Rename all public TOML types and exceptions in the parser,
including ETOMLParseError, TTOMLNode and related TTOML/TTemporal declarations,
to the required ELWPT and TLWPT prefixes, and update every reference throughout
the unit. Apply the identical renaming to the corresponding TOML.pas copy under
packages/toml so both parser interfaces match.
- Around line 1758-1787: Update TTOMLParser.TryParseOffset to accept only
uppercase 'Z' for the UTC designator, removing the lowercase 'z' condition while
preserving numeric offset parsing.
- Around line 491-506: Update TTOMLParser.PrepareInput to validate the entire
input as well-formed UTF-8, including bytes inside quoted strings, before
tokenization proceeds; reject malformed sequences such as invalid leading bytes,
truncated sequences, bad continuation bytes, and overlong encodings through the
existing parse-error path. Add byte-level regression tests covering
representative invalid UTF-8 inputs and confirm valid UTF-8 remains accepted.
- Around line 366-395: Update CanonicalizeIntegerToken and the binary/octal
integer parsing paths to validate each QWord multiplication and addition before
applying it, preventing 2^64 and larger values from wrapping; convert any
overflow into ETOMLParseError rather than runtime error 215. Preserve valid
handling for 2^63-1 and 2^63, and add coverage for 2^63-1, 2^63, and 2^64 in
both binary and octal forms.
- Around line 1210-1223: Add exception-safe ownership cleanup to ParseArray,
ParseInlineTable, and ParseKeyValuePair: initialize temporary TTOMLNode
variables to nil, free them in exception paths when ownership has not
transferred, and clear ValueNode after successful AssignValue ownership
transfer. Ensure parse failures and duplicate-key rejection do not leak unowned
nodes while preserving normal ownership behavior.
- Around line 1289-1295: In the inline-table key/value parsing flow around
ParseKeyPath and ParseValue, replace both SkipWhitespace(True) calls surrounding
the '=' separator with SkipWhitespace(False) so newlines and comments are
rejected there. Preserve SkipWhitespace(True) for whitespace between
inline-table entries.
- Around line 209-217: Update IsValidTime and the TryParseDateTime validation
flow so ASecond = 60 is accepted only when the parsed date and offset identify
an actual leap second; reject it for ordinary timestamps. For local times
without a date or offset, define the policy as rejecting 60 because leap-second
validity cannot be verified, while preserving existing validation for seconds
0–59 and fractional components.
In `@source/LWPT.Core.Test.pas`:
- Line 170: Update the suite comment associated with
TestPeelSuffixRecordsCommitIdentity to state that the peel-suffix record
populates PeeledSHA rather than being discarded, matching the renamed test’s
current contract.
---
Nitpick comments:
In @.lwpt/modules/httpclient/source/TransportSecurity.Test.pas:
- Around line 851-863: Update the ciphertext feeding logic before
TransportSecurityServerRead to repeatedly call TransportSecurityFeedCiphertext
until the entire remainder is accepted, advancing the buffer offset and
remaining length by each call’s accepted count and accumulating the total. Keep
the existing final flow counter and backpressure assertions unchanged, while
avoiding any assumption that one feed accepts all remaining ciphertext.
In `@source/LWPT.Core.Test.pas`:
- Around line 28-29: Move TestArrayCannotBecomeTablePath, its class declaration,
and its registration from TLoadManifestValidation into the co-located
TOML.Test.pas unit, keeping it as a direct TTOMLParser.ParseDocument test;
update the relevant test unit references so the test remains registered and
follows the co-location convention.
- Around line 1665-1669: Update
TGitProtocolParsing.TestPeelSuffixRecordsCommitIdentity to include a second tag
in the fixture, then assert that only v1.0.0 receives the peeled commit SHA.
Also verify v1.0.0 retains its original bbbb... tag-object SHA, ensuring peeled
records are associated with the correct tag rather than the most recently parsed
tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cfd69a1f-9c4a-4571-935b-67906240913d
⛔ Files ignored due to path filters (1)
lwpt.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.lwpt/modules/httpclient/lwpt.toml.lwpt/modules/httpclient/source/TransportSecurity.Test.pas.lwpt/modules/httpclient/source/TransportSecurity.pas.lwpt/modules/httpclient/tests/e2e/TransportSecuritySocket.E2E.Test.pas.lwpt/modules/semver/lwpt.toml.lwpt/modules/semver/source/Semver.Test.pas.lwpt/modules/semver/source/Semver.pas.lwpt/modules/toml/lwpt.toml.lwpt/modules/toml/source/TOML.pasdocs/architecture.mddocs/packages.mddocs/testing.mdpackages/semver/lwpt.tomlpackages/semver/source/Semver.Test.paspackages/semver/source/Semver.paspackages/toml/lwpt.tomlpackages/toml/source/TOML.passource/LWPT.Core.Test.passource/LWPT.Manifest.pas
🚧 Files skipped from review as they are similar to previous changes (5)
- .lwpt/modules/semver/lwpt.toml
- docs/packages.md
- .lwpt/modules/toml/lwpt.toml
- source/LWPT.Manifest.pas
- .lwpt/modules/semver/source/Semver.Test.pas
frostney
removed
ci:ready
frostney
added
ci:ready
|
@coderabbitai review |
✅ Action performedReview finished.
|
frostney
added
the
merge:ready
1 participant
Summary
Stack role
This is the independent resolver lane for the 0.5.0 rush. It does not depend on the compiler, analysis, HTTP, TLS, or observability stacks.
Validation
Closes #36
Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by CodeRabbit
New Features
Bug Fixes
Documentation