Add architect-reviewer skill, lenses, and agent#427
Add architect-reviewer skill, lenses, and agent#427moumighosh wants to merge 3 commits intomasterfrom
Conversation
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
Added |
| 2. **[Gap: Threat Model]** Question text | ||
| ... | ||
|
|
||
| ### 5. Recommendation |
There was a problem hiding this comment.
Any possible examples we can include in "references"?
Examples/templates, etc help significantly in replicating the output you are looking for. Perhaps we can test this agent/skill on some example, agree if we like the output and then add that as a reference
| --- | ||
| name: architect-reviewer | ||
| description: > | ||
| Produce a structured Architect Review Brief from an Android Identity Platform developer design document. |
There was a problem hiding this comment.
Could you shared (maybe on Teams) a run of this against the telemetry spec?
There was a problem hiding this comment.
It's fairly hard to tell if these definitions are good or need changes without throwing them at some test cases, so I consider this to be almost like an acceptance test for the merge.
|
|
||
| - Do NOT summarize the document — produce only the structured brief | ||
| - Do NOT skip the lens-loading step — the lenses encode the platform-specific rules | ||
| - Absent threat model in the design → automatic 🔴 in the Risk Register |
There was a problem hiding this comment.
This seems a little extreme, but that's just my personal opinion. I expect some docs are going to discuss things that don't have security impact.
| | Rename public method/class | 🔴 Breaking | | ||
| | Change method signature (add required param) | 🔴 Breaking | | ||
| | Add new required field to Parcelable/Serializable | 🔴 Breaking | | ||
| | Change enum values | 🔴 Breaking | |
There was a problem hiding this comment.
Another huge one here -- adding new error code strings without checking protocol version.
This is one of the biggest sources of problem for our dashboards. New error codes get added for existing cases, then all the existing clients start to go from some error that the OneAuth version they shipped with understood like 'ServerTemporarilyUnavailable' which we can account for, to an unknown value we classify as Unexpected.
Maybe we can't enforce this just yet without protocol changes. But I would like to start marking error enum values as introduced in some particular broker protocol SDK version and require the broker to map errors backwards to less-precise but better-than-Unexpected errors at the IPC boundary when they realize the client won't know how to handle it.
I'd love to get a feature on the books for this, we've had several of these recently.
No description provided.