Fix review feedback: shared type aliases, Z timestamp, vocabulary validation, ValueError masking#320
Conversation
|
Also be sure to fix markdownlint issues |
…bulary validation, ValueError masking, broken path Co-authored-by: ahouseholder <2594236+ahouseholder@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback from PR #319 by consolidating non-empty string validation into shared type aliases, fixing ISO-8601 Z parsing for timestamps, enforcing tag vocabulary membership in CaseReference, eliminating a silent ValueError-masking pattern, and fixing a broken file path in a prompt.
Changes:
- New
NonEmptyString/OptionalNonEmptyStringtype aliases invultron/as_vocab/base/types.py(per CS-08-002), replacing per-field validators inVulnerabilityRecordandEmbargoPolicy CaseEventnow handles ISO-8601ZUTC suffix inreceived_atstring parsing;CaseReference.validate_tags()now enforces membership inCASE_REFERENCE_TAG_VOCABULARY;CreateInitialVendorParticipantreplacestry/except ValueErrorwith explicitdatalayer.read()pre-check- Path correction in
prompts/IDEATION_prompt.md:plans/IDEAS.md→plan/IDEAS.md
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
vultron/as_vocab/base/types.py |
New module defining shared NonEmptyString and OptionalNonEmptyString Pydantic type aliases per CS-08-002 |
vultron/as_vocab/objects/vulnerability_record.py |
Replaces per-field validators with NonEmptyString/list[NonEmptyString] type aliases |
vultron/as_vocab/objects/embargo_policy.py |
Replaces per-field validators with NonEmptyString/OptionalNonEmptyString type aliases |
vultron/as_vocab/objects/case_event.py |
Fixes Z-suffix ISO-8601 parsing by normalizing before fromisoformat() |
vultron/as_vocab/objects/case_reference.py |
Enforces tag vocabulary membership in the existing validate_tags() validator |
vultron/behaviors/case/nodes.py |
Replaces silent ValueError swallowing with explicit pre-check via datalayer.read() |
test/as_vocab/test_vulnerability_record.py |
Updates error message assertions to match the new shared validator message |
test/as_vocab/test_embargo_policy.py |
Updates error message assertions to match the new shared validator message |
test/as_vocab/test_case_reference.py |
Adds test for invalid tag vocabulary rejection |
test/as_vocab/test_case_event.py |
Adds test for Z-suffix ISO-8601 parsing |
prompts/IDEATION_prompt.md |
Fixes two broken path references plans/IDEAS.md → plan/IDEAS.md |
| def parse_received_at(cls, v) -> datetime: | ||
| if isinstance(v, str): | ||
| return datetime.fromisoformat(v) | ||
| return datetime.fromisoformat(v.replace("Z", "+00:00")) |
There was a problem hiding this comment.
The str.replace("Z", "+00:00") call replaces ALL occurrences of "Z" in the input string, not just a trailing one. While a well-formed ISO 8601 timestamp only has "Z" as a trailing UTC designator (so in practice this is harmless), a more precise and defensive implementation would only replace a trailing "Z". For example, checking v.endswith("Z") before replacing, or using a targeted replacement like (v[:-1] + "+00:00") if v.endswith("Z") else v.
| return datetime.fromisoformat(v.replace("Z", "+00:00")) | |
| if v.endswith("Z"): | |
| v = v[:-1] + "+00:00" | |
| return datetime.fromisoformat(v) |
Addresses all review comments from PR #319. Consolidates non-empty string validation into shared type aliases, fixes ISO-8601
Zparsing, enforces tag vocabulary membership, and eliminates a silent error-masking pattern.Changes
Shared
NonEmptyString/OptionalNonEmptyStringtype aliases (vultron/as_vocab/base/types.py)NonEmptyString = Annotated[str, AfterValidator(_non_empty)]andOptionalNonEmptyString = Optional[NonEmptyString]VulnerabilityRecordandEmbargoPolicyrefactored@field_validatorstubs with shared type aliases onname,aliases,actor_id,inbox,notes"must be a non-empty string"(Pydantic includes field path in full error output)CaseEvent— ISO-8601Zsuffix supportparse_received_at()now normalizesZ→+00:00beforefromisoformat(), works across all Python versionsCreateInitialVendorParticipant— eliminate ValueError maskingtry/except ValueErrorarounddatalayer.create()with an explicitdatalayer.read()pre-checkValueErrorfor wrong record type or missingid_; now only skips creation when the record already existsCaseReference— enforce tag vocabularyvalidate_tags()now validates each tag againstCASE_REFERENCE_TAG_VOCABULARY; previously the constant was defined but never usedValueErrorwith the full list of valid optionsprompts/IDEATION_prompt.mdplans/IDEAS.md→plan/IDEAS.md(two occurrences)Please note: Pull request submissions are subject to our
Contribution Instructions
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.