2404 stack 4 model definition accuracy - #2447
Merged
correct-horse-battery-bench merged 11 commits intoAug 21, 2026
Merged
Conversation
correct-horse-battery-bench
force-pushed
the
2404-stack-4-model-definition-accuracy
branch
from
August 18, 2026 07:44
08ed599 to
0d32bf3
Compare
correct-horse-battery-bench
force-pushed
the
2404-stack-4-model-definition-accuracy
branch
from
August 18, 2026 14:07
07a3496 to
9f3543b
Compare
Every optional key of a column definition was defaulted where it was read, so "subtype", "protected", "private", "alias", "public" and "dba_mapping" each repeated an array_key_exists check inside the string that builds the feature dict, and the nullability of a column was spelled as a double negation twice. Merge the defaults onto the column once before the loop, so the feature dict is built from keys that are known to be there, and state the rule the double negation encoded: a column without a "null" key is nullable, "null" => False makes it non-nullable. A choice key may be a string as well as an integer, which the emitted choice list now quotes accordingly. The generated models are unchanged; running the generator produces the same files.
Twelve columns hold one of a fixed set of values, decided by a define in src/inc/defines/ or by the notification registry, and said so nowhere: the spec described them as a plain integer or string, so a client had to read the server source to learn that an agent runs on one of three operating systems, and the API accepted any number for them. Attach the choices to the columns that have them, taken from the define that decides the value: the operating system of an agent, the type of an agent statistic, the state of a chunk, the type of a file, the status of a file download, the status and mode of a health check, the status of a health check agent, the issuer and level of a log entry, and the two columns of a notification setting. The two notification columns are the ones the spec got wrong until now. 'action' names the event that triggers a notification, which NotificationUtils validates against DNotificationType, and 'notification' names the delivery method, which it validates against the registered HashtopolisNotification instances. The choices of the second are derived from that registry rather than listed a second time, so a newly registered delivery method appears in the spec by being registered. Each choice becomes a oneOf branch of const and title in the spec, which is how a generated client learns the name of a value, and validateData rejects a value that is not one of them.
Eight columns disagreed with the schema they are stored in, so the spec promised an attribute that can be missing and the API rejected a value the database accepts, or the other way round. Four are nullable in the database and were declared as never null: the chunk of an agent error (an error is not necessarily tied to a chunk), the chunk of a cracked hash in both hash tables, and the object a notification setting refers to (an agent-wide event refers to none). The spec described them as an integer, so a client following it broke on the null the API answers with, and PATCHing the attribute back to null was refused. Four are NOT NULL in the database and were declared as nullable: the benchmark of an assignment, the revoked flag of an API key, the permission map of a permission group and the valid flag of a user. Those become required on create, which is what the database enforces anyway, and stop accepting a PATCH to null. See src/migrations/*/20260619090219_initial.sql for the column definitions the declarations now match.
TaskUtils::getStatus() and TaskWrapperDisplayAPI::getAggregateStatus() decided the status of a task with bare numbers, explained by a comment that listed three of the four values the functions return. DTaskStatus names the five values, and both functions use the constants. The values are unchanged; what the reader gains is that "skipped" is visible as such at the two places that return it, for an archived task and for a task that found no crack. The define is also what the generated spec will name the values of the computed status attribute with, so the code producing them and the document describing them stay in step.
Every config item was described as a plain string, although the value an item takes is decided by its type: a select item accepts exactly the keys of its selection, and DConfig knows both. A client editing a config had no way to learn which values are valid other than trying one. ConfigAPI answers the attributes schema of its resource itself, through getOpenAPIAttributesSchemaOverride(): a hook this adds to AbstractBaseAPI for an attributes object no feature type can describe, which the generator reads in place of the schema it derives. The override carries one branch per select item, naming the item as a const and its selection as a oneOf of const and title, plus one generic branch for every item whose value is a free string. The branches are titled after the item, so a generated client names the union members after the config they belong to.
The benchmark column is NOT NULL, so declaring it as such made it mandatory on POST /ui/agentassignments. The endpoint accepted the attribute and then dropped it: AgentUtils::assign() overwrote whatever came in with a hardcoded 0, so the spec demanded a value the API refused to keep. Let assign() take the benchmark to record and hand it the value the request carried. The other five callers - unassigning an agent, the assignAgent helper, the two user_api entry points and the legacy agent handler - know no benchmark and keep the "0" the function used to hardcode, as its default.
The isRevoked column is NOT NULL, so declaring it as such made it mandatory on POST /ui/apiTokens. The endpoint accepted the attribute and then dropped it: JwtTokenUtils::createKey() wrote a hardcoded 0, so a client that asked for a revoked key got a live one back and the spec demanded a value the API refused to keep. Let createKey() take the flag and hand it the value the request carried. The default is the "not revoked" the function used to hardcode, which is also the default the column carries.
isValid is NOT NULL, so declaring it as such made it mandatory on POST /ui/users, next to the sessionLifetime that was mandatory already. Both were hidden from the spec: getAllPostParameters() unset them, so the create body documented three attributes while the API insisted on five and answered any request that followed the document with 400. Drop the override and let the two attributes appear as the mandatory attributes they are. createUser() has received both from the request all along, so the "?? false" and "?? 3600" fallbacks stood for a case validateParameters() does not let through and go with it.
correct-horse-battery-bench
force-pushed
the
2404-stack-4-model-definition-accuracy
branch
from
August 18, 2026 14:37
9f3543b to
be22079
Compare
correct-horse-battery-bench
marked this pull request as ready for review
August 18, 2026 14:38
s3inlc
requested changes
Aug 19, 2026
| $dict['notification'] = ['read_only' => False, "type" => "str(50)", "subtype" => "unset", "choices" => "unset", "null" => False, "pk" => False, "protected" => False, "private" => False, "alias" => "notification", "public" => False, "dba_mapping" => False]; | ||
| $dict['action'] = ['read_only' => False, "type" => "str(50)", "subtype" => "unset", "choices" => ["taskComplete" => "Task completed", "agentError" => "Agent error", "ownAgentError" => "Own agent error", "logError" => "Log error", "newTask" => "New task", "newHashlist" => "New hashlist", "hashlistAllCracked" => "Hashlist all cracked", "hashlistCrackedHash" => "Hashlist cracked hash", "userCreated" => "User created", "userDeleted" => "User deleted", "userLoginFailed" => "User login failed", "logWarn" => "Log warning", "logFatal" => "Log fatal", "newAgent" => "New agent", "deleteTask" => "Delete task", "deleteHashlist" => "Delete hashlist", "deleteAgent" => "Delete agent", ], "null" => False, "pk" => False, "protected" => False, "private" => False, "alias" => "action", "public" => False, "dba_mapping" => False]; | ||
| $dict['objectId'] = ['read_only' => True, "type" => "int", "subtype" => "unset", "choices" => "unset", "null" => True, "pk" => False, "protected" => True, "private" => False, "alias" => "objectId", "public" => False, "dba_mapping" => False]; | ||
| $dict['notification'] = ['read_only' => False, "type" => "str(50)", "subtype" => "unset", "choices" => ["ChatBot" => "ChatBot", "Discord Webhook" => "Discord Webhook", "Email" => "Email", "Example" => "Example", "Slack" => "Slack", "Telegram" => "Telegram", ], "null" => False, "pk" => False, "protected" => False, "private" => False, "alias" => "notification", "public" => False, "dba_mapping" => False]; |
Member
There was a problem hiding this comment.
Baking the existing Notifications that strongly into the model is probably not ideal. Users can put in their own notification classes and use them. With this change, this would require to change DBA model files
| * @throws Exception | ||
| */ | ||
| public static function assign(int $agentId, int $taskId, User $user): ?Assignment { | ||
| public static function assign(int $agentId, int $taskId, User $user, string $benchmark = "0"): ?Assignment { |
Member
There was a problem hiding this comment.
I think it would be better to defaul to empty string than 0.
s3inlc
approved these changes
Aug 21, 2026
correct-horse-battery-bench
deleted the
2404-stack-4-model-definition-accuracy
branch
August 21, 2026 09:49
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update model definitions so the openapi spec becomes more accurate.